Skip to content

Commit f48b83b

Browse files
ekaspermattcaswell
authored andcommitted
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 Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org>
1 parent 708cf59 commit f48b83b

File tree

1 file changed

+47
-10
lines changed

1 file changed

+47
-10
lines changed

Diff for: crypto/x509/x509_vfy.c

+47-10
Original file line numberDiff line numberDiff line change
@@ -1808,47 +1808,84 @@ int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)
18081808
ASN1_TIME atm;
18091809
long offset;
18101810
char buff1[24], buff2[24], *p;
1811-
int i, j;
1811+
int i, j, remaining;
18121812

18131813
p = buff1;
1814-
i = ctm->length;
1814+
remaining = ctm->length;
18151815
str = (char *)ctm->data;
1816+
/*
1817+
* Note that the following (historical) code allows much more slack in the
1818+
* time format than RFC5280. In RFC5280, the representation is fixed:
1819+
* UTCTime: YYMMDDHHMMSSZ
1820+
* GeneralizedTime: YYYYMMDDHHMMSSZ
1821+
*/
18161822
if (ctm->type == V_ASN1_UTCTIME) {
1817-
if ((i < 11) || (i > 17))
1823+
/* YYMMDDHHMM[SS]Z or YYMMDDHHMM[SS](+-)hhmm */
1824+
int min_length = sizeof("YYMMDDHHMMZ") - 1;
1825+
int max_length = sizeof("YYMMDDHHMMSS+hhmm") - 1;
1826+
if (remaining < min_length || remaining > max_length)
18181827
return 0;
18191828
memcpy(p, str, 10);
18201829
p += 10;
18211830
str += 10;
1831+
remaining -= 10;
18221832
} else {
1823-
if (i < 13)
1833+
/* YYYYMMDDHHMM[SS[.fff]]Z or YYYYMMDDHHMM[SS[.f[f[f]]]](+-)hhmm */
1834+
int min_length = sizeof("YYYYMMDDHHMMZ") - 1;
1835+
int max_length = sizeof("YYYYMMDDHHMMSS.fff+hhmm") - 1;
1836+
if (remaining < min_length || remaining > max_length)
18241837
return 0;
18251838
memcpy(p, str, 12);
18261839
p += 12;
18271840
str += 12;
1841+
remaining -= 12;
18281842
}
18291843

18301844
if ((*str == 'Z') || (*str == '-') || (*str == '+')) {
18311845
*(p++) = '0';
18321846
*(p++) = '0';
18331847
} else {
1848+
/* SS (seconds) */
1849+
if (remaining < 2)
1850+
return 0;
18341851
*(p++) = *(str++);
18351852
*(p++) = *(str++);
1836-
/* Skip any fractional seconds... */
1837-
if (*str == '.') {
1853+
remaining -= 2;
1854+
/*
1855+
* Skip any (up to three) fractional seconds...
1856+
* TODO(emilia): in RFC5280, fractional seconds are forbidden.
1857+
* Can we just kill them altogether?
1858+
*/
1859+
if (remaining && *str == '.') {
18381860
str++;
1839-
while ((*str >= '0') && (*str <= '9'))
1840-
str++;
1861+
remaining--;
1862+
for (i = 0; i < 3 && remaining; i++, str++, remaining--) {
1863+
if (*str < '0' || *str > '9')
1864+
break;
1865+
}
18411866
}
18421867

18431868
}
18441869
*(p++) = 'Z';
18451870
*(p++) = '\0';
18461871

1847-
if (*str == 'Z')
1872+
/* We now need either a terminating 'Z' or an offset. */
1873+
if (!remaining)
1874+
return 0;
1875+
if (*str == 'Z') {
1876+
if (remaining != 1)
1877+
return 0;
18481878
offset = 0;
1849-
else {
1879+
} else {
1880+
/* (+-)HHMM */
18501881
if ((*str != '+') && (*str != '-'))
18511882
return 0;
1883+
/* Historical behaviour: the (+-)hhmm offset is forbidden in RFC5280. */
1884+
if (remaining != 5)
1885+
return 0;
1886+
if (str[1] < '0' || str[1] > '9' || str[2] < '0' || str[2] > '9' ||
1887+
str[3] < '0' || str[3] > '9' || str[4] < '0' || str[4] > '9')
1888+
return 0;
18521889
offset = ((str[1] - '0') * 10 + (str[2] - '0')) * 60;
18531890
offset += (str[3] - '0') * 10 + (str[4] - '0');
18541891
if (*str == '-')

0 commit comments

Comments
 (0)