Skip to content

OpenSSL version 3.3.0-dev: OpenSSL::ASN1::ASN1Error: utctime/generalizedtime is too short #724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
junaruga opened this issue Mar 11, 2024 · 7 comments · Fixed by #728
Closed

Comments

@junaruga
Copy link
Member

I got the following errors only in the CI openssl-head case when running the latest master commit 1e8e246 on openssl-head on my forked repository. The used OpenSSL is the latest master branch openssl/openssl@1f03d33 according to the log.

https://github.com/junaruga/ruby-openssl/actions/runs/8233851472/job/22514261811#step:12:613

1) Error: test_generalizedtime(OpenSSL::TestASN1): OpenSSL::ASN1::ASN1Error: generalizedtime is too short
/home/runner/work/ruby-openssl/ruby-openssl/test/openssl/test_asn1.rb:698:in `decode'
/home/runner/work/ruby-openssl/ruby-openssl/test/openssl/test_asn1.rb:698:in `decode_test'
/home/runner/work/ruby-openssl/ruby-openssl/test/openssl/test_asn1.rb:433:in `test_generalizedtime'
     430:       OpenSSL::ASN1::GeneralizedTime.new(Time.utc(9999, 9, 8, 23, 43, 39))
     431:     # LibreSSL 3.6.0 requires the seconds element
     432:     return if libressl?
  => 433:     decode_test B(%w{ 18 0D }) + "201612081934Z".b,
     434:       OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 0))
     435:     # not implemented
     436:     # decode_test B(%w{ 18 13 }) + "20161208193439+0930".b,

2) Error: test_utctime(OpenSSL::TestASN1): OpenSSL::ASN1::ASN1Error: utctime is too short
/home/runner/work/ruby-openssl/ruby-openssl/test/openssl/test_asn1.rb:698:in `decode'
/home/runner/work/ruby-openssl/ruby-openssl/test/openssl/test_asn1.rb:698:in `decode_test'
/home/runner/work/ruby-openssl/ruby-openssl/test/openssl/test_asn1.rb:411:in `test_utctime'
     408:     end
     409:     # Seconds is omitted. LibreSSL 3.6.0 requires it
     410:     return if libressl?
  => 411:     decode_test B(%w{ 17 0B }) + "1609082343Z".b,
     412:       OpenSSL::ASN1::UTCTime.new(Time.utc(2016, 9, 8, 23, 43, 0))
     413:     # not implemented
     414:     # decode_test B(%w{ 17 11 }) + "500908234339+0930".b,
@rhenium
Copy link
Member

rhenium commented Mar 11, 2024

Grep with "generalizedtime is too short" lead me to this OpenSSL PR: openssl/openssl#23483.

I think we can remove these assertions.

@junaruga
Copy link
Member Author

junaruga commented Mar 11, 2024

Thanks for finding the commit. Did you mean that we remove the decode_test lines?

For the 2nd error test, I debugged. Below is a minimal Ruby reproducer.

$ ruby -I ./lib -r openssl -e 'der = [%w{ 17 0B }.join].pack("H*") + "1609082343Z".b; OpenSSL::ASN1.decode(der)'
-e:1:in `decode': utctime is too short (OpenSSL::ASN1::ASN1Error)
  from -e:1:in `<main>'

Below is the gdb debugging and backtrace.

What do you think about that we adjust the value of the der for the OpenSSL::ASN1.decode(der) to pass the tests? I just don't understand the meaning of the 2 assertions that caused the errors.

$ gdb --args ruby -I ./lib -r openssl -e 'der = [%w{ 17 0B }.join].pack("H*") + "1609082343Z".b; OpenSSL::ASN1.decode(der)'
...
(gdb) l
436	    VALUE ret;
437	    int status = 0;
438	
439	    p = der;
440	    if(!(time = d2i_ASN1_TIME(NULL, &p, length)))
441		ossl_raise(eASN1Error, NULL);
442	    ret = rb_protect(asn1time_to_time_i,
443			     (VALUE)time, &status);
444	    ASN1_TIME_free(time);
445	    if(status) rb_jump_tag(status);
(gdb) f
#0  decode_time (der=0x7fffe86ac520 "\027\v1609082343Z", length=13)
    at ../../../../ext/openssl/ossl_asn1.c:441
441		ossl_raise(eASN1Error, NULL);
(gdb) p time
$7 = (ASN1_TIME *) 0x0
(gdb) p length
$8 = 13
(gdb) bt
#0  decode_time (der=0x7fffe86ac520 "\027\v1609082343Z", length=13)
    at ../../../../ext/openssl/ossl_asn1.c:441
#1  0x00007fffe8744716 in int_ossl_asn1_decode0_prim (pp=0x7fffffffc648, length=11, 
    hlen=2, tag=23, tc=20667148, num_read=0x7fffffffc5b0)
    at ../../../../ext/openssl/ossl_asn1.c:778
#2  0x00007fffe8744de9 in ossl_asn1_decode0 (pp=0x7fffffffc648, length=13, 
    offset=0x7fffffffc630, depth=0, yield=0, num_read=0x7fffffffc638)
    at ../../../../ext/openssl/ossl_asn1.c:919
#3  0x00007fffe874501f in ossl_asn1_decode (self=140737093809160, obj=140737092699520)
    at ../../../../ext/openssl/ossl_asn1.c:1006
...

@junaruga
Copy link
Member Author

By the way, there are 2 kinds of the errors: OpenSSL::ASN1::ASN1Error: generalizedtime is too short and OpenSSL::ASN1::ASN1Error: utctime is too short.

@junaruga
Copy link
Member Author

junaruga commented Mar 11, 2024

I debugged the 2nd error test again into the asn1_ex_c2i function changing the logic.

$ gdb --args ruby -I ./lib -r openssl -e 'der = [%w{ 17 0B }.join].pack("H*") + "1609082343Z".b; OpenSSL::ASN1.decode(der)'
...
(gdb) f
#0  asn1_ex_c2i (pval=0x7fffffff9558, cont=0x7fffcfbe5a2a "1609082343Z", len=11, 
    utype=23, free_cont=0x7fffffff9365 "", it=0x7fffce3a4bc0 <local_it>)
    at crypto/asn1/tasn_dec.c:929
929	            ERR_raise(ERR_LIB_ASN1, ASN1_R_UTCTIME_IS_TOO_SHORT);

(gdb) l
924	        if (utype == V_ASN1_GENERALIZEDTIME && (len < 15)) {
925	            ERR_raise(ERR_LIB_ASN1, ASN1_R_GENERALIZEDTIME_IS_TOO_SHORT);
926	            goto err;
927	        }
928	        if (utype == V_ASN1_UTCTIME && (len < 13)) {
929	            ERR_raise(ERR_LIB_ASN1, ASN1_R_UTCTIME_IS_TOO_SHORT);
930	            goto err;
931	        }
932	        /* All based on ASN1_STRING and handled the same */
933	        if (*pval == NULL) {
(gdb) p len
$2 = 11
(gdb) bt
#0  asn1_ex_c2i (pval=0x7fffffff9558, cont=0x7fffcfbe5a2a "1609082343Z", len=11, 
    utype=23, free_cont=0x7fffffff9365 "", it=0x7fffce3a4bc0 <local_it>)
    at crypto/asn1/tasn_dec.c:929
#1  0x00007fffcdeec2a9 in asn1_d2i_ex_primitive (pval=0x7fffffff9558, 
    in=0x7fffffff9608, inlen=13, it=0x7fffce3a4bc0 <local_it>, tag=23, aclass=0, 
    opt=0 '\000', ctx=0x7fffffff9560) at crypto/asn1/tasn_dec.c:818
#2  0x00007fffcdeea82b in asn1_item_embed_d2i (pval=0x7fffffff9558, in=0x7fffffff9608, 
    len=13, it=0x7fffce3a4bc0 <local_it>, tag=-1, aclass=0, opt=0 '\000', 
    ctx=0x7fffffff9560, depth=1, libctx=0x0, propq=0x0) at crypto/asn1/tasn_dec.c:256
#3  0x00007fffcdeea238 in asn1_item_ex_d2i_intern (pval=0x7fffffff9558, 
    in=0x7fffffff9608, len=13, it=0x7fffce3a4bc0 <local_it>, tag=-1, aclass=0, 
    opt=0 '\000', ctx=0x7fffffff9560, libctx=0x0, propq=0x0)
    at crypto/asn1/tasn_dec.c:118
#4  0x00007fffcdeea321 in ASN1_item_d2i_ex (pval=0x7fffffff9558, in=0x7fffffff9608, 
    len=13, it=0x7fffce3a4bc0 <local_it>, libctx=0x0, propq=0x0)
    at crypto/asn1/tasn_dec.c:144
#5  0x00007fffcdeea375 in ASN1_item_d2i (pval=0x0, in=0x7fffffff9608, len=13, 
    it=0x7fffce3a4bc0 <local_it>) at crypto/asn1/tasn_dec.c:154
#6  0x00007fffcded8edc in d2i_ASN1_TIME (a=0x0, in=0x7fffffff9608, len=13)
    at crypto/asn1/a_time.c:27
#7  0x00007fffe8743cdd in decode_time (der=0x7fffcfbe5a28 "\027\v1609082343Z", 
    length=13) at ../../../../ext/openssl/ossl_asn1.c:440
#8  0x00007fffe8744716 in int_ossl_asn1_decode0_prim (pp=0x7fffffff9798, length=11, 
    hlen=2, tag=23, tc=18135820, num_read=0x7fffffff9700)
    at ../../../../ext/openssl/ossl_asn1.c:778
#9  0x00007fffe8744de9 in ossl_asn1_decode0 (pp=0x7fffffff9798, length=13, 
    offset=0x7fffffff9780, depth=0, yield=0, num_read=0x7fffffff9788)
    at ../../../../ext/openssl/ossl_asn1.c:919
#10 0x00007fffe874501f in ossl_asn1_decode (self=140737093953200, obj=140736678747360)
    at ../../../../ext/openssl/ossl_asn1.c:1006

@junaruga junaruga changed the title OpenSSL version 3.3.0-dev: OpenSSL::ASN1::ASN1Error: generalizedtime is too short OpenSSL version 3.3.0-dev: OpenSSL::ASN1::ASN1Error: utctime/generalizedtime is too short Mar 12, 2024
@junaruga
Copy link
Member Author

OK. It seems that the assertions getting the errors are testing the cases of returning the String format without the seconds.

According to the PDF file: ITU-T X.690 / ISO/IEC 8825-1 mentioned at openssl/openssl#23483 (comment), it seems that the document and/or OpenSSL project thinks that the 000000Z (HHMISSZ) is the minimal valid string, and thinks that the 0000Z (HHMIZ) is invalid.

  • section 11.7: GeneralizedTime - 11.7.5 - YYYYMMDD000000Z
  • section 11.8: UTCTime - 11.8.3 - YYMMDD000000Z

I think we can remove these assertions.

So, as rhenium mentioned, just dropping the assertions makes sense to me.

@junaruga
Copy link
Member Author

I sent the PR #728.

@rhenium
Copy link
Member

rhenium commented Mar 13, 2024

Yes, seconds in GeneralizedTime and UTCTime is required in DER, but is optional in BER, according to the document.

OpenSSL's parser hasn't been very consistent; it supports DER and partially BER. It still is more liberal than strictly DER by allowing timezone string, but I think the change itself is OK.

Either way, I think the current assertions were overly specific for the ruby/openssl test suite. The PR looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants