From 60eba30f60de55e3c782469fa555eede82606099 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 6 Jul 2017 11:39:03 +1000 Subject: [PATCH] Memory bounds checking in asn1 code. Check that sprint, strcpy don't overflow. Avoid some strlen operations when the previous sprintf return value can be used. Also fix the undefined behaviour `*(long *)x = y` when x isn't a long or character pointer. ISO/IEC 9899:1999 6.5/7 for the details. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3869) --- crypto/asn1/a_gentm.c | 12 ++++++------ crypto/asn1/a_mbstr.c | 6 +++--- crypto/asn1/a_time.c | 19 +++++++++++-------- crypto/asn1/a_utctm.c | 28 +++++++++++++--------------- crypto/asn1/asn1_par.c | 20 ++++++++++---------- crypto/asn1/x_long.c | 18 ++++++++++++------ 6 files changed, 55 insertions(+), 48 deletions(-) diff --git a/crypto/asn1/a_gentm.c b/crypto/asn1/a_gentm.c index 2c5fb1c3d2..5cfc3ff15e 100644 --- a/crypto/asn1/a_gentm.c +++ b/crypto/asn1/a_gentm.c @@ -1,5 +1,5 @@ /* - * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -207,7 +207,7 @@ ASN1_GENERALIZEDTIME *ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, char *p; struct tm *ts; struct tm data; - size_t len = 20; + const size_t len = 20; ASN1_GENERALIZEDTIME *tmps = NULL; if (s == NULL) @@ -237,10 +237,10 @@ ASN1_GENERALIZEDTIME *ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, tmps->data = (unsigned char *)p; } - sprintf(p, "%04d%02d%02d%02d%02d%02dZ", ts->tm_year + 1900, - ts->tm_mon + 1, ts->tm_mday, ts->tm_hour, ts->tm_min, - ts->tm_sec); - tmps->length = strlen(p); + tmps->length = BIO_snprintf(p, len, "%04d%02d%02d%02d%02d%02dZ", + ts->tm_year + 1900, ts->tm_mon + 1, + ts->tm_mday, ts->tm_hour, ts->tm_min, + ts->tm_sec); tmps->type = V_ASN1_GENERALIZEDTIME; #ifdef CHARSET_EBCDIC_not ebcdic2ascii(tmps->data, tmps->data, tmps->length); diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c index 46764b292c..e644fe0542 100644 --- a/crypto/asn1/a_mbstr.c +++ b/crypto/asn1/a_mbstr.c @@ -1,5 +1,5 @@ /* - * Copyright 1999-2016 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1999-2017 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -100,14 +100,14 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, if ((minsize > 0) && (nchar < minsize)) { ASN1err(ASN1_F_ASN1_MBSTRING_NCOPY, ASN1_R_STRING_TOO_SHORT); - sprintf(strbuf, "%ld", minsize); + BIO_snprintf(strbuf, sizeof(strbuf), "%ld", minsize); ERR_add_error_data(2, "minsize=", strbuf); return -1; } if ((maxsize > 0) && (nchar > maxsize)) { ASN1err(ASN1_F_ASN1_MBSTRING_NCOPY, ASN1_R_STRING_TOO_LONG); - sprintf(strbuf, "%ld", maxsize); + BIO_snprintf(strbuf, sizeof(strbuf), "%ld", maxsize); ERR_add_error_data(2, "maxsize=", strbuf); return -1; } diff --git a/crypto/asn1/a_time.c b/crypto/asn1/a_time.c index f0ec42f71c..fc78e309a6 100644 --- a/crypto/asn1/a_time.c +++ b/crypto/asn1/a_time.c @@ -1,5 +1,5 @@ /* - * Copyright 1999-2016 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1999-2017 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -81,17 +81,20 @@ ASN1_GENERALIZEDTIME *ASN1_TIME_to_generalizedtime(const ASN1_TIME *t, goto done; } - /* grow the string */ + /* + * Grow the string by two bytes. + * The actual allocation is t->length + 3 to include a terminator byte. + */ if (!ASN1_STRING_set(ret, NULL, t->length + 2)) goto err; str = (char *)ret->data; /* Work out the century and prepend */ - if (t->data[0] >= '5') - strcpy(str, "19"); - else - strcpy(str, "20"); - - strcat(str, (const char *)t->data); + memcpy(str, t->data[0] >= '5' ? "19" : "20", 2); + /* + * t->length + 1 is the size of the data and the allocated buffer has + * this much space after the first two characters. + */ + OPENSSL_strlcpy(str + 2, (const char *)t->data, t->length + 1); done: if (out != NULL && *out == NULL) diff --git a/crypto/asn1/a_utctm.c b/crypto/asn1/a_utctm.c index 25393ee152..5a4b1742f7 100644 --- a/crypto/asn1/a_utctm.c +++ b/crypto/asn1/a_utctm.c @@ -1,5 +1,5 @@ /* - * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -21,7 +21,7 @@ int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d) int n, i, l, o, min_l = 11, strict = 0; if (d->type != V_ASN1_UTCTIME) - return (0); + return 0; l = d->length; a = (char *)d->data; o = 0; @@ -150,9 +150,9 @@ int ASN1_UTCTIME_set_string(ASN1_UTCTIME *s, const char *str) return 0; s->type = V_ASN1_UTCTIME; } - return (1); - } else - return (0); + return 1; + } + return 0; } ASN1_UTCTIME *ASN1_UTCTIME_set(ASN1_UTCTIME *s, time_t t) @@ -166,7 +166,7 @@ ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, char *p; struct tm *ts; struct tm data; - size_t len = 20; + const size_t len = 20; int free_s = 0; if (s == NULL) { @@ -199,15 +199,14 @@ ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, s->data = (unsigned char *)p; } - sprintf(p, "%02d%02d%02d%02d%02d%02dZ", ts->tm_year % 100, - ts->tm_mon + 1, ts->tm_mday, ts->tm_hour, ts->tm_min, - ts->tm_sec); - s->length = strlen(p); + s->length = BIO_snprintf(p, len, "%02d%02d%02d%02d%02d%02dZ", + ts->tm_year % 100, ts->tm_mon + 1, ts->tm_mday, + ts->tm_hour, ts->tm_min, ts->tm_sec); s->type = V_ASN1_UTCTIME; #ifdef CHARSET_EBCDIC_not ebcdic2ascii(s->data, s->data, s->length); #endif - return (s); + return s; err: if (free_s) ASN1_UTCTIME_free(s); @@ -272,10 +271,9 @@ int ASN1_UTCTIME_print(BIO *bp, const ASN1_UTCTIME *tm) if (BIO_printf(bp, "%s %2d %02d:%02d:%02d %d%s", _asn1_mon[M - 1], d, h, m, s, y + 1900, (gmt) ? " GMT" : "") <= 0) - return (0); - else - return (1); + return 0; + return 1; err: BIO_write(bp, "Bad time value", 14); - return (0); + return 0; } diff --git a/crypto/asn1/asn1_par.c b/crypto/asn1/asn1_par.c index 19b21e7d49..4b60c615de 100644 --- a/crypto/asn1/asn1_par.c +++ b/crypto/asn1/asn1_par.c @@ -1,5 +1,5 @@ /* - * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -38,32 +38,32 @@ static int asn1_print_info(BIO *bp, int tag, int xclass, int constructed, p = str; if ((xclass & V_ASN1_PRIVATE) == V_ASN1_PRIVATE) - sprintf(str, "priv [ %d ] ", tag); + BIO_snprintf(str, sizeof(str), "priv [ %d ] ", tag); else if ((xclass & V_ASN1_CONTEXT_SPECIFIC) == V_ASN1_CONTEXT_SPECIFIC) - sprintf(str, "cont [ %d ]", tag); + BIO_snprintf(str, sizeof(str), "cont [ %d ]", tag); else if ((xclass & V_ASN1_APPLICATION) == V_ASN1_APPLICATION) - sprintf(str, "appl [ %d ]", tag); + BIO_snprintf(str, sizeof(str), "appl [ %d ]", tag); else if (tag > 30) - sprintf(str, "", tag); + BIO_snprintf(str, sizeof(str), "", tag); else p = ASN1_tag2str(tag); if (BIO_printf(bp, fmt, p) <= 0) goto err; - return (1); + return 1; err: - return (0); + return 0; } int ASN1_parse(BIO *bp, const unsigned char *pp, long len, int indent) { - return (asn1_parse2(bp, &pp, len, 0, 0, indent, 0)); + return asn1_parse2(bp, &pp, len, 0, 0, indent, 0); } int ASN1_parse_dump(BIO *bp, const unsigned char *pp, long len, int indent, int dump) { - return (asn1_parse2(bp, &pp, len, 0, 0, indent, dump)); + return asn1_parse2(bp, &pp, len, 0, 0, indent, dump); } static int asn1_parse2(BIO *bp, const unsigned char **pp, long length, @@ -342,7 +342,7 @@ static int asn1_parse2(BIO *bp, const unsigned char **pp, long length, ASN1_OBJECT_free(o); ASN1_OCTET_STRING_free(os); *pp = p; - return (ret); + return ret; } const char *ASN1_tag2str(int tag) diff --git a/crypto/asn1/x_long.c b/crypto/asn1/x_long.c index 78f4b764c3..bf9371ef55 100644 --- a/crypto/asn1/x_long.c +++ b/crypto/asn1/x_long.c @@ -1,5 +1,5 @@ /* - * Copyright 2000-2016 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 2000-2017 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -14,6 +14,9 @@ #if !(OPENSSL_API_COMPAT < 0x10200000L) NON_EMPTY_TRANSLATION_UNIT #else + +#define COPY_SIZE(a, b) (sizeof(a) < sizeof(b) ? sizeof(a) : sizeof(b)) + /* * Custom primitive type for long handling. This converts between an * ASN1_INTEGER and a long directly. @@ -49,13 +52,13 @@ ASN1_ITEM_end(ZLONG) static int long_new(ASN1_VALUE **pval, const ASN1_ITEM *it) { - *(long *)pval = it->size; + memcpy(pval, &it->size, COPY_SIZE(*pval, it->size)); return 1; } static void long_free(ASN1_VALUE **pval, const ASN1_ITEM *it) { - *(long *)pval = it->size; + memcpy(pval, &it->size, COPY_SIZE(*pval, it->size)); } /* @@ -90,7 +93,7 @@ static int long_i2c(ASN1_VALUE **pval, unsigned char *cont, int *putype, unsigned long utmp, sign; int clen, pad, i; - ltmp = *(long *)pval; + memcpy(<mp, pval, COPY_SIZE(*pval, ltmp)); if (ltmp == it->size) return -1; /* @@ -183,13 +186,16 @@ static int long_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, ASN1err(ASN1_F_LONG_C2I, ASN1_R_INTEGER_TOO_LARGE_FOR_LONG); return 0; } - *(long*)pval = ltmp; + memcpy(pval, <mp, COPY_SIZE(*pval, ltmp)); return 1; } static int long_print(BIO *out, ASN1_VALUE **pval, const ASN1_ITEM *it, int indent, const ASN1_PCTX *pctx) { - return BIO_printf(out, "%ld\n", *(long *)pval); + long l; + + memcpy(&l, pval, COPY_SIZE(*pval, l)); + return BIO_printf(out, "%ld\n", l); } #endif -- 2.25.1