diff options
author | David Benjamin <davidben@chromium.org> | 2015-06-16 18:51:15 +0300 |
---|---|---|
committer | Adam Langley <agl@google.com> | 2015-06-16 22:07:15 +0300 |
commit | d87021d2467fca449d98f1251ddc3c87b9b97f30 (patch) | |
tree | 620d3b72640a66ed8d53ac5a55170bb4b946619d /crypto/x509 | |
parent | 184494dfccbf854212a82e230465b98ea1e86afe (diff) |
Fix length checks in X509_cmp_time to avoid out-of-bounds reads.
Also tighten X509_cmp_time to reject more than three fractional
seconds in the time; and to reject trailing garbage after the offset.
CVE-2015-1789
(Imported from upstream's 9bc3665ac9e3c36f7762acd3691e1115d250b030)
Change-Id: I2091b2d1b691c177d58dc7960e2e7eb4c97b1f69
Reviewed-on: https://boringssl-review.googlesource.com/5124
Reviewed-by: Adam Langley <agl@google.com>
Diffstat (limited to 'crypto/x509')
-rw-r--r-- | crypto/x509/x509_vfy.c | 54 |
1 files changed, 47 insertions, 7 deletions
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 2ba9c84a..f53f279d 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -1829,49 +1829,89 @@ int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time) ASN1_TIME atm; long offset; char buff1[24],buff2[24],*p; - int i,j; + int i, j, remaining; p=buff1; - i=ctm->length; + remaining = ctm->length; str=(char *)ctm->data; + /* Note that the following (historical) code allows much more slack in + * the time format than RFC5280. In RFC5280, the representation is + * fixed: + * UTCTime: YYMMDDHHMMSSZ + * GeneralizedTime: YYYYMMDDHHMMSSZ */ if (ctm->type == V_ASN1_UTCTIME) { - if ((i < 11) || (i > 17)) return 0; + /* YYMMDDHHMM[SS]Z or YYMMDDHHMM[SS](+-)hhmm */ + int min_length = sizeof("YYMMDDHHMMZ") - 1; + int max_length = sizeof("YYMMDDHHMMSS+hhmm") - 1; + if (remaining < min_length || remaining > max_length) + return 0; memcpy(p,str,10); p+=10; str+=10; + remaining -= 10; } else { - if (i < 13) return 0; + /* YYYYMMDDHHMM[SS[.fff]]Z or YYYYMMDDHHMM[SS[.f[f[f]]]](+-)hhmm */ + int min_length = sizeof("YYYYMMDDHHMMZ") - 1; + int max_length = sizeof("YYYYMMDDHHMMSS.fff+hhmm") - 1; + if (remaining < min_length || remaining > max_length) + return 0; memcpy(p,str,12); p+=12; str+=12; + remaining -= 12; } if ((*str == 'Z') || (*str == '-') || (*str == '+')) { *(p++)='0'; *(p++)='0'; } else { + /* SS (seconds) */ + if (remaining < 2) + return 0; *(p++)= *(str++); *(p++)= *(str++); - /* Skip any fractional seconds... */ - if (*str == '.') + remaining -= 2; + /* Skip any (up to three) fractional seconds... + * TODO(emilia): in RFC5280, fractional seconds are forbidden. + * Can we just kill them altogether? */ + if (remaining && *str == '.') { str++; - while ((*str >= '0') && (*str <= '9')) str++; + remaining--; + for (i = 0; i < 3 && remaining; i++, str++, remaining--) + { + if (*str < '0' || *str > '9') + break; + } } } *(p++)='Z'; *(p++)='\0'; + /* We now need either a terminating 'Z' or an offset. */ + if (!remaining) + return 0; if (*str == 'Z') + { + if (remaining != 1) + return 0; offset=0; + } else { + /* (+-)HHMM */ if ((*str != '+') && (*str != '-')) return 0; + /* Historical behaviour: the (+-)hhmm offset is forbidden in RFC5280. */ + if (remaining != 5) + return 0; + if (str[1] < '0' || str[1] > '9' || str[2] < '0' || str[2] > '9' || + str[3] < '0' || str[3] > '9' || str[4] < '0' || str[4] > '9') + return 0; offset=((str[1]-'0')*10+(str[2]-'0'))*60; offset+=(str[3]-'0')*10+(str[4]-'0'); if (*str == '-') |