ASN1_UTCTIME_print: fix, comment, test.

The motiviation is that M2Crypto passes an ASN1_GENERALIZEDTIME to
this function.  This is not distinct from ASN1_UTCTIME (both are
asn1_string_st), but ASN1_GENERALIZEDTIME uses a 4-digit year in its
string representation, whereas ASN1_UTCTIME uses a 2-digit year.

ASN1_UTCTIME_print previously did not return an error on such inputs.
So, stricten (?) the function, ensuring that it checks for trailing
data, and rejects values that are invalid for their place.  Along the
way, clean it up and add tests.

Change-Id: Ia8298bed573f2acfdab96638ea69c78b5bba4e4b
Reviewed-on: https://boringssl-review.googlesource.com/13082
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/x509/t_x509.c

@@ -54,6 +54,7 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
+#include <ctype.h>
 #include <openssl/asn1.h>
 #include <openssl/bio.h>
 #include <openssl/digest.h>
@@ -415,45 +416,84 @@ int ASN1_GENERALIZEDTIME_print(BIO *bp, const ASN1_GENERALIZEDTIME *tm)
     return (0);
 }
 
-int ASN1_UTCTIME_print(BIO *bp, const ASN1_UTCTIME *tm)
-{
-    const char *v;
-    int gmt = 0;
-    int i;
-    int y = 0, M = 0, d = 0, h = 0, m = 0, s = 0;
+// consume_two_digits is a helper function for ASN1_UTCTIME_print. If |*v|,
+// assumed to be |*len| bytes long, has two leading digits, updates |*out| with
+// their value, updates |v| and |len|, and returns one. Otherwise, returns
+// zero.
+static int consume_two_digits(int* out, const char **v, int *len) {
+  if (*len < 2|| !isdigit((*v)[0]) || !isdigit((*v)[1])) {
+    return 0;
+  }
+  *out = ((*v)[0] - '0') * 10 + ((*v)[1] - '0');
+  *len -= 2;
+  *v += 2;
+  return 1;
+}
 
-    i = tm->length;
-    v = (const char *)tm->data;
+// consume_zulu_timezone is a helper function for ASN1_UTCTIME_print. If |*v|,
+// assumed to be |*len| bytes long, starts with "Z" then it updates |*v| and
+// |*len| and returns one. Otherwise returns zero.
+static int consume_zulu_timezone(const char **v, int *len) {
+  if (*len == 0 || (*v)[0] != 'Z') {
+    return 0;
+  }
+
+  *len -= 1;
+  *v += 1;
+  return 1;
+}
 
-    if (i < 10)
-        goto err;
-    if (v[i - 1] == 'Z')
-        gmt = 1;
-    for (i = 0; i < 10; i++)
-        if ((v[i] > '9') || (v[i] < '0'))
-            goto err;
-    y = (v[0] - '0') * 10 + (v[1] - '0');
-    if (y < 50)
-        y += 100;
-    M = (v[2] - '0') * 10 + (v[3] - '0');
-    if ((M > 12) || (M < 1))
-        goto err;
-    d = (v[4] - '0') * 10 + (v[5] - '0');
-    h = (v[6] - '0') * 10 + (v[7] - '0');
-    m = (v[8] - '0') * 10 + (v[9] - '0');
-    if (tm->length >= 12 &&
-        (v[10] >= '0') && (v[10] <= '9') && (v[11] >= '0') && (v[11] <= '9'))
-        s = (v[10] - '0') * 10 + (v[11] - '0');
-
-    if (BIO_printf(bp, "%s %2d %02d:%02d:%02d %d%s",
-                   mon[M - 1], d, h, m, s, y + 1900,
-                   (gmt) ? " GMT" : "") <= 0)
-        return (0);
-    else
-        return (1);
- err:
-    BIO_write(bp, "Bad time value", 14);
-    return (0);
+int ASN1_UTCTIME_print(BIO *bp, const ASN1_UTCTIME *tm) {
+  const char *v = (const char *)tm->data;
+  int len = tm->length;
+  int Y = 0, M = 0, D = 0, h = 0, m = 0, s = 0;
+
+  // YYMMDDhhmm are required to be present.
+  if (!consume_two_digits(&Y, &v, &len) ||
+      !consume_two_digits(&M, &v, &len) ||
+      !consume_two_digits(&D, &v, &len) ||
+      !consume_two_digits(&h, &v, &len) ||
+      !consume_two_digits(&m, &v, &len)) {
+    goto err;
+  }
+  // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, requires seconds
+  // to be present, but historically this code has forgiven its absence.
+  consume_two_digits(&s, &v, &len);
+
+  // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, specifies this
+  // interpretation of the year.
+  if (Y < 50) {
+    Y += 2000;
+  } else {
+    Y += 1900;
+  }
+  if (M > 12 || M == 0) {
+    goto err;
+  }
+  if (D > 31 || D == 0) {
+    goto err;
+  }
+  if (h > 23 || m > 59 || s > 60) {
+    goto err;
+  }
+
+  // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, requires the "Z"
+  // to be present, but historically this code has forgiven its absence.
+  const int is_gmt = consume_zulu_timezone(&v, &len);
+
+  // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, does not permit
+  // the specification of timezones using the +hhmm / -hhmm syntax, which is
+  // the only other thing that might legitimately be found at the end.
+  if (len) {
+    goto err;
+  }
+
+  return BIO_printf(bp, "%s %2d %02d:%02d:%02d %d%s", mon[M - 1], D, h, m, s, Y,
+                    is_gmt ? " GMT" : "") > 0;
+
+err:
+  BIO_write(bp, "Bad time value", 14);
+  return 0;
 }
 
 int X509_NAME_print(BIO *bp, X509_NAME *name, int obase)

crypto/x509/x509_test.cc

@@ -17,6 +17,7 @@
 #include <assert.h>
 #include <string.h>
 
+#include <openssl/bio.h>
 #include <openssl/bytestring.h>
 #include <openssl/crypto.h>
 #include <openssl/digest.h>
@@ -1011,6 +1012,73 @@ static bool TestFailedParseFromBuffer() {
   return true;
 }
 
+static bool TestPrintUTCTIME() {
+  static const struct {
+    const char *val, *want;
+  } asn1_utctime_tests[] = {
+    {"", "Bad time value"},
+
+    // Correct RFC 5280 form. Test years < 2000 and > 2000.
+    {"090303125425Z", "Mar  3 12:54:25 2009 GMT"},
+    {"900303125425Z", "Mar  3 12:54:25 1990 GMT"},
+    {"000303125425Z", "Mar  3 12:54:25 2000 GMT"},
+
+    // Correct form, bad values.
+    {"000000000000Z", "Bad time value"},
+    {"999999999999Z", "Bad time value"},
+
+    // Missing components. Not legal RFC 5280, but permitted.
+    {"090303125425", "Mar  3 12:54:25 2009"},
+    {"9003031254", "Mar  3 12:54:00 1990"},
+    {"9003031254Z", "Mar  3 12:54:00 1990 GMT"},
+
+    // GENERALIZEDTIME confused for UTCTIME.
+    {"20090303125425Z", "Bad time value"},
+
+    // Legal ASN.1, but not legal RFC 5280.
+    {"9003031254+0800", "Bad time value"},
+    {"9003031254-0800", "Bad time value"},
+
+    // Trailing garbage.
+    {"9003031254Z ", "Bad time value"},
+  };
+
+  for (auto t : asn1_utctime_tests) {
+    bssl::UniquePtr<ASN1_UTCTIME> tm(ASN1_UTCTIME_new());
+    bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+
+    // Use this instead of ASN1_UTCTIME_set() because some callers get
+    // type-confused and pass ASN1_GENERALIZEDTIME to ASN1_UTCTIME_print().
+    // ASN1_UTCTIME_set_string() is stricter, and would reject the inputs in
+    // question.
+    if (!ASN1_STRING_set(tm.get(), t.val, strlen(t.val))) {
+      fprintf(stderr, "ASN1_STRING_set\n");
+      return false;
+    }
+    const int ok = ASN1_UTCTIME_print(bio.get(), tm.get());
+
+    const uint8_t *contents;
+    size_t len;
+    if (!BIO_mem_contents(bio.get(), &contents, &len)) {
+      fprintf(stderr, "BIO_mem_contents\n");
+      return false;
+    }
+
+    if (ok != (strcmp(t.want, "Bad time value") != 0)) {
+      fprintf(stderr, "ASN1_UTCTIME_print(%s): bad return value\n", t.val);
+      return false;
+    }
+    if (len != strlen(t.want) || memcmp(contents, t.want, len)) {
+      fprintf(stderr, "ASN1_UTCTIME_print(%s): got %.*s, want %s\n", t.val,
+              static_cast<int>(len),
+              reinterpret_cast<const char *>(contents), t.want);
+      return false;
+    }
+  }
+
+  return true;
+}
+
 int main() {
   CRYPTO_library_init();
 
@@ -1023,7 +1091,8 @@ int main() {
       !TestFromBufferTrailingData() ||
       !TestFromBufferModified() ||
       !TestFromBufferReused() ||
-      !TestFailedParseFromBuffer()) {
+      !TestFailedParseFromBuffer() ||
+      !TestPrintUTCTIME()) {
     return 1;
   }