Skip to content

Commit 73484f6

Browse files
rheniumrhenium
committed
asn1: use ASN1_TIME_to_tm() to decode UTCTime and GeneralizedTime
The current logic relies on sscanf() and error checks are almost entirely missing. It also assumes that ASN1_STRING contents are NUL terminated, which is undocumented and not guaranteed for all valid ASN1_TIME objects. Switch to using ASN1_TIME_to_tm() added in OpenSSL 1.1.1. It is also supported by LibreSSL and AWS-LC. In the long term, we may want to replace ASN1_TIME_to_tm() with a hand-rolled decoder, since the function is intended for a specific use-case. It is too permissive for strict DER, yet still does not support all valid DER inputs and silently drops information such as fractional seconds. However, it handles everything that the current sscanf() code could handle.
1 parent 6c0ef87 commit 73484f6

File tree

2 files changed

+71
-60
lines changed

2 files changed

+71
-60
lines changed

‎ext/openssl/ossl_asn1.c‎

Lines changed: 25 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -55,57 +55,35 @@ static ID id_each;
5555
/*
5656
* DATE conversion
5757
*/
58+
static VALUE
59+
time_utc_new(VALUE args)
60+
{
61+
return rb_funcallv(rb_cTime, rb_intern("utc"), 6, (VALUE *)args);
62+
}
63+
64+
static VALUE
65+
time_utc_new_rescue(VALUE args, VALUE exc)
66+
{
67+
rb_raise(eASN1Error, "invalid time");
68+
}
69+
5870
VALUE
5971
asn1time_to_time(const ASN1_TIME *time)
6072
{
6173
struct tm tm;
62-
VALUE argv[6];
63-
int count;
64-
65-
memset(&tm, 0, sizeof(struct tm));
66-
67-
switch (time->type) {
68-
case V_ASN1_UTCTIME:
69-
count = sscanf((const char *)time->data, "%2d%2d%2d%2d%2d%2dZ",
70-
&tm.tm_year, &tm.tm_mon, &tm.tm_mday, &tm.tm_hour, &tm.tm_min,
71-
&tm.tm_sec);
72-
73-
if (count == 5) {
74-
tm.tm_sec = 0;
75-
} else if (count != 6) {
76-
ossl_raise(rb_eTypeError, "bad UTCTIME format: \"%s\"",
77-
time->data);
78-
}
79-
if (tm.tm_year < 50) {
80-
tm.tm_year += 2000;
81-
} else {
82-
tm.tm_year += 1900;
83-
}
84-
break;
85-
case V_ASN1_GENERALIZEDTIME:
86-
count = sscanf((const char *)time->data, "%4d%2d%2d%2d%2d%2dZ",
87-
&tm.tm_year, &tm.tm_mon, &tm.tm_mday, &tm.tm_hour, &tm.tm_min,
88-
&tm.tm_sec);
89-
if (count == 5) {
90-
tm.tm_sec = 0;
91-
}
92-
else if (count != 6) {
93-
ossl_raise(rb_eTypeError, "bad GENERALIZEDTIME format: \"%s\"",
94-
time->data);
95-
}
96-
break;
97-
default:
98-
rb_warning("unknown time format");
99-
return Qnil;
100-
}
101-
argv[0] = INT2NUM(tm.tm_year);
102-
argv[1] = INT2NUM(tm.tm_mon);
103-
argv[2] = INT2NUM(tm.tm_mday);
104-
argv[3] = INT2NUM(tm.tm_hour);
105-
argv[4] = INT2NUM(tm.tm_min);
106-
argv[5] = INT2NUM(tm.tm_sec);
107-
108-
return rb_funcall2(rb_cTime, rb_intern("utc"), 6, argv);
74+
if (!ASN1_TIME_to_tm(time, &tm))
75+
ossl_raise(eASN1Error, "ASN1_TIME_to_tm");
76+
77+
VALUE args[] = {
78+
INT2NUM(tm.tm_year + 1900),
79+
INT2NUM(tm.tm_mon + 1),
80+
INT2NUM(tm.tm_mday),
81+
INT2NUM(tm.tm_hour),
82+
INT2NUM(tm.tm_min),
83+
INT2NUM(tm.tm_sec),
84+
};
85+
return rb_rescue2(time_utc_new, (VALUE)args, time_utc_new_rescue, Qnil,
86+
rb_eArgError, 0);
10987
}
11088

11189
static VALUE

‎test/openssl/test_asn1.rb‎

Lines changed: 46 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -426,42 +426,75 @@ def test_utctime
426426
OpenSSL::ASN1::UTCTime.new(Time.new(2049, 12, 31, 23, 0, 0, "-04:00")).to_der
427427
}
428428

429-
# not implemented
429+
# UTC offset (BER): ASN1_TIME_to_tm() may or may not support it
430430
# decode_test B(%w{ 17 11 }) + "500908234339+0930".b,
431431
# OpenSSL::ASN1::UTCTime.new(Time.new(1950, 9, 8, 23, 43, 39, "+09:30"))
432432
# decode_test B(%w{ 17 0F }) + "5009082343-0930".b,
433433
# OpenSSL::ASN1::UTCTime.new(Time.new(1950, 9, 8, 23, 43, 0, "-09:30"))
434-
# assert_raise(OpenSSL::ASN1::ASN1Error) {
435-
# OpenSSL::ASN1.decode(B(%w{ 17 0C }) + "500908234339".b)
436-
# }
437-
# assert_raise(OpenSSL::ASN1::ASN1Error) {
438-
# OpenSSL::ASN1.decode(B(%w{ 17 0D }) + "500908234339Y".b)
439-
# }
434+
435+
# Seconds is omitted (BER)
436+
# decode_test B(%w{ 18 0D }) + "201612081934Z".b,
437+
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 0))
438+
439+
# Fractional seconds is not allowed in UTCTime
440+
assert_raise(OpenSSL::ASN1::ASN1Error) {
441+
OpenSSL::ASN1.decode(B(%w{ 17 0F }) + "160908234339.5Z".b)
442+
}
443+
444+
# Missing "Z"
445+
assert_raise(OpenSSL::ASN1::ASN1Error) {
446+
OpenSSL::ASN1.decode(B(%w{ 17 0C }) + "500908234339".b)
447+
}
448+
assert_raise(OpenSSL::ASN1::ASN1Error) {
449+
OpenSSL::ASN1.decode(B(%w{ 17 0D }) + "500908234339Y".b)
450+
}
440451
end
441452

442453
def test_generalizedtime
443454
encode_decode_test B(%w{ 18 0F }) + "20161208193429Z".b,
444455
OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 29))
445456
encode_decode_test B(%w{ 18 0F }) + "99990908234339Z".b,
446457
OpenSSL::ASN1::GeneralizedTime.new(Time.utc(9999, 9, 8, 23, 43, 39))
447-
# not implemented
458+
459+
# Fractional seconds (DER). Not supported by ASN1_TIME_to_tm()
460+
# because struct tm cannot store it.
461+
# encode_decode_test B(%w{ 18 11 }) + "20161208193439.5Z".b,
462+
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 39.5))
463+
464+
# UTC offset (BER): ASN1_TIME_to_tm() may or may not support it
448465
# decode_test B(%w{ 18 13 }) + "20161208193439+0930".b,
449466
# OpenSSL::ASN1::GeneralizedTime.new(Time.new(2016, 12, 8, 19, 34, 39, "+09:30"))
450467
# decode_test B(%w{ 18 11 }) + "201612081934-0930".b,
451468
# OpenSSL::ASN1::GeneralizedTime.new(Time.new(2016, 12, 8, 19, 34, 0, "-09:30"))
452469
# decode_test B(%w{ 18 11 }) + "201612081934-09".b,
453470
# OpenSSL::ASN1::GeneralizedTime.new(Time.new(2016, 12, 8, 19, 34, 0, "-09:00"))
471+
472+
# Minutes and seconds are omitted (BER)
473+
# decode_test B(%w{ 18 0B }) + "2016120819Z".b,
474+
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 0, 0))
475+
# Fractional hours (BER)
454476
# decode_test B(%w{ 18 0D }) + "2016120819.5Z".b,
455477
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 30, 0))
478+
# Fractional hours with "," as the decimal separator (BER)
456479
# decode_test B(%w{ 18 0D }) + "2016120819,5Z".b,
457480
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 30, 0))
481+
482+
# Seconds is omitted (BER)
483+
# decode_test B(%w{ 18 0D }) + "201612081934Z".b,
484+
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 0))
485+
# Fractional minutes (BER)
458486
# decode_test B(%w{ 18 0F }) + "201612081934.5Z".b,
459487
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 30))
460-
# decode_test B(%w{ 18 11 }) + "20161208193439.5Z".b,
461-
# OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 39.5))
462-
# assert_raise(OpenSSL::ASN1::ASN1Error) {
463-
# OpenSSL::ASN1.decode(B(%w{ 18 0D }) + "201612081934Y".b)
464-
# }
488+
489+
# Missing "Z"
490+
assert_raise(OpenSSL::ASN1::ASN1Error) {
491+
OpenSSL::ASN1.decode(B(%w{ 18 0F }) + "20161208193429Y".b)
492+
}
493+
494+
# Encoding year out of range
495+
assert_raise(OpenSSL::ASN1::ASN1Error) {
496+
OpenSSL::ASN1::GeneralizedTime.new(Time.utc(10000, 9, 8, 23, 43, 39)).to_der
497+
}
465498
end
466499

467500
def test_basic_asn1data

0 commit comments

Comments
 (0)