Fix up some integer types in crypto/asn1

tag and utype are always accessed as int, so make the structs match.
Boolean ASN1_ITEMs put an ASN1_BOOLEAN in it->size, so add a cast. Also
fix the time set_string functions to call the underlying CBS parser
directly, so they don't need to put a strlen into an int.

Bug: 516
Change-Id: Ie10e7eaf58ec0b0dec59813a0ddcb0197fce1fd1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55449
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

crypto/asn1/a_gentm.c

@@ -81,22 +81,16 @@ int ASN1_GENERALIZEDTIME_check(const ASN1_GENERALIZEDTIME *d) {
 }
 
 int ASN1_GENERALIZEDTIME_set_string(ASN1_GENERALIZEDTIME *s, const char *str) {
-  ASN1_GENERALIZEDTIME t;
-
-  t.type = V_ASN1_GENERALIZEDTIME;
-  t.length = strlen(str);
-  t.data = (unsigned char *)str;
-  if (ASN1_GENERALIZEDTIME_check(&t)) {
-    if (s != NULL) {
-      if (!ASN1_STRING_set((ASN1_STRING *)s, (unsigned char *)str, t.length)) {
-        return 0;
-      }
-      s->type = V_ASN1_GENERALIZEDTIME;
-    }
-    return 1;
-  } else {
+  size_t len = strlen(str);
+  CBS cbs;
+  CBS_init(&cbs, (const uint8_t *)str, len);
+  if (!CBS_parse_generalized_time(&cbs, /*out_tm=*/NULL,
+                                  /*allow_timezone_offset=*/0) ||
+      !ASN1_STRING_set(s, str, len)) {
     return 0;
   }
+  s->type = V_ASN1_GENERALIZEDTIME;
+  return 1;
 }
 
 ASN1_GENERALIZEDTIME *ASN1_GENERALIZEDTIME_set(ASN1_GENERALIZEDTIME *s,

crypto/asn1/a_object.c

@@ -107,8 +107,12 @@ int i2t_ASN1_OBJECT(char *buf, int buf_len, const ASN1_OBJECT *a) {
 }
 
 static int write_str(BIO *bp, const char *str) {
-  int len = strlen(str);
-  return BIO_write(bp, str, len) == len ? len : -1;
+  size_t len = strlen(str);
+  if (len > INT_MAX) {
+    OPENSSL_PUT_ERROR(ASN1, ERR_R_OVERFLOW);
+    return -1;
+  }
+  return BIO_write(bp, str, (int)len) == (int)len ? (int)len : -1;
 }
 
 int i2a_ASN1_OBJECT(BIO *bp, const ASN1_OBJECT *a) {

crypto/asn1/a_strex.c

@@ -121,7 +121,7 @@ static int do_esc_char(uint32_t c, unsigned long flags, char *do_quotes,
     return maybe_write(out, &u8, 1) ? 1 : -1;
   }
 
-  int len = strlen(buf);
+  int len = (int)strlen(buf);  // |buf| is guaranteed to be short.
   return maybe_write(out, buf, len) ? len : -1;
 }
 

crypto/asn1/a_time.c

@@ -164,28 +164,9 @@ err:
   return NULL;
 }
 
-
 int ASN1_TIME_set_string(ASN1_TIME *s, const char *str) {
-  ASN1_TIME t;
-
-  t.length = strlen(str);
-  t.data = (unsigned char *)str;
-  t.flags = 0;
-
-  t.type = V_ASN1_UTCTIME;
-
-  if (!ASN1_TIME_check(&t)) {
-    t.type = V_ASN1_GENERALIZEDTIME;
-    if (!ASN1_TIME_check(&t)) {
-      return 0;
-    }
-  }
-
-  if (s && !ASN1_STRING_copy((ASN1_STRING *)s, (ASN1_STRING *)&t)) {
-    return 0;
-  }
-
-  return 1;
+  return ASN1_UTCTIME_set_string(s, str) ||
+         ASN1_GENERALIZEDTIME_set_string(s, str);
 }
 
 static int asn1_time_to_tm(struct tm *tm, const ASN1_TIME *t,

crypto/asn1/a_utctm.c

@@ -82,22 +82,16 @@ int ASN1_UTCTIME_check(const ASN1_UTCTIME *d) {
 }
 
 int ASN1_UTCTIME_set_string(ASN1_UTCTIME *s, const char *str) {
-  ASN1_UTCTIME t;
-
-  t.type = V_ASN1_UTCTIME;
-  t.length = strlen(str);
-  t.data = (unsigned char *)str;
-  if (ASN1_UTCTIME_check(&t)) {
-    if (s != NULL) {
-      if (!ASN1_STRING_set((ASN1_STRING *)s, (unsigned char *)str, t.length)) {
-        return 0;
-      }
-      s->type = V_ASN1_UTCTIME;
-    }
-    return 1;
-  } else {
+  size_t len = strlen(str);
+  CBS cbs;
+  CBS_init(&cbs, (const uint8_t *)str, len);
+  if (!CBS_parse_utc_time(&cbs, /*out_tm=*/NULL,
+                          /*allow_timezone_offset=*/1) ||
+      !ASN1_STRING_set(s, str, len)) {
     return 0;
   }
+  s->type = V_ASN1_UTCTIME;
+  return 1;
 }
 
 ASN1_UTCTIME *ASN1_UTCTIME_set(ASN1_UTCTIME *s, time_t t) {

crypto/asn1/asn1_test.cc

@@ -1048,6 +1048,34 @@ TEST(ASN1Test, SetTime) {
   }
 }
 
+TEST(ASN1Test, TimeSetString) {
+  bssl::UniquePtr<ASN1_STRING> s(ASN1_STRING_new());
+  ASSERT_TRUE(s);
+
+  ASSERT_TRUE(ASN1_UTCTIME_set_string(s.get(), "700101000000Z"));
+  EXPECT_EQ(V_ASN1_UTCTIME, ASN1_STRING_type(s.get()));
+  EXPECT_EQ("700101000000Z", ASN1StringToStdString(s.get()));
+
+  ASSERT_TRUE(ASN1_GENERALIZEDTIME_set_string(s.get(), "19700101000000Z"));
+  EXPECT_EQ(V_ASN1_GENERALIZEDTIME, ASN1_STRING_type(s.get()));
+  EXPECT_EQ("19700101000000Z", ASN1StringToStdString(s.get()));
+
+  // |ASN1_TIME_set_string| accepts either format. It relies on there being no
+  // overlap between the two.
+  ASSERT_TRUE(ASN1_TIME_set_string(s.get(), "700101000000Z"));
+  EXPECT_EQ(V_ASN1_UTCTIME, ASN1_STRING_type(s.get()));
+  EXPECT_EQ("700101000000Z", ASN1StringToStdString(s.get()));
+
+  ASSERT_TRUE(ASN1_TIME_set_string(s.get(), "19700101000000Z"));
+  EXPECT_EQ(V_ASN1_GENERALIZEDTIME, ASN1_STRING_type(s.get()));
+  EXPECT_EQ("19700101000000Z", ASN1StringToStdString(s.get()));
+
+  // Invalid inputs are rejected.
+  EXPECT_FALSE(ASN1_UTCTIME_set_string(s.get(), "nope"));
+  EXPECT_FALSE(ASN1_GENERALIZEDTIME_set_string(s.get(), "nope"));
+  EXPECT_FALSE(ASN1_TIME_set_string(s.get(), "nope"));
+}
+
 TEST(ASN1Test, AdjTime) {
   struct tm tm1, tm2;
   int days, secs;

crypto/asn1/tasn_fre.c

@@ -218,7 +218,7 @@ void ASN1_primitive_free(ASN1_VALUE **pval, const ASN1_ITEM *it) {
 
     case V_ASN1_BOOLEAN:
       if (it) {
-        *(ASN1_BOOLEAN *)pval = it->size;
+        *(ASN1_BOOLEAN *)pval = (ASN1_BOOLEAN)it->size;
       } else {
         *(ASN1_BOOLEAN *)pval = -1;
       }

crypto/asn1/tasn_new.c

@@ -273,9 +273,6 @@ static void asn1_template_clear(ASN1_VALUE **pval, const ASN1_TEMPLATE *tt) {
 // all the old functions.
 
 static int ASN1_primitive_new(ASN1_VALUE **pval, const ASN1_ITEM *it) {
-  ASN1_TYPE *typ;
-  int utype;
-
   if (!it) {
     return 0;
   }
@@ -284,6 +281,7 @@ static int ASN1_primitive_new(ASN1_VALUE **pval, const ASN1_ITEM *it) {
   // |ASN1_PRIMITIVE_FUNCS| table of calbacks.
   assert(it->funcs == NULL);
 
+  int utype;
   if (it->itype == ASN1_ITYPE_MSTRING) {
     utype = -1;
   } else {
@@ -295,15 +293,15 @@ static int ASN1_primitive_new(ASN1_VALUE **pval, const ASN1_ITEM *it) {
       return 1;
 
     case V_ASN1_BOOLEAN:
-      *(ASN1_BOOLEAN *)pval = it->size;
+      *(ASN1_BOOLEAN *)pval = (ASN1_BOOLEAN)it->size;
       return 1;
 
     case V_ASN1_NULL:
       *pval = (ASN1_VALUE *)1;
       return 1;
 
-    case V_ASN1_ANY:
-      typ = OPENSSL_malloc(sizeof(ASN1_TYPE));
+    case V_ASN1_ANY: {
+      ASN1_TYPE *typ = OPENSSL_malloc(sizeof(ASN1_TYPE));
       if (!typ) {
         return 0;
       }
@@ -311,6 +309,7 @@ static int ASN1_primitive_new(ASN1_VALUE **pval, const ASN1_ITEM *it) {
       typ->type = -1;
       *pval = (ASN1_VALUE *)typ;
       break;
+    }
 
     default:
       *pval = (ASN1_VALUE *)ASN1_STRING_type_new(utype);
@@ -333,7 +332,7 @@ static void asn1_primitive_clear(ASN1_VALUE **pval, const ASN1_ITEM *it) {
     utype = it->utype;
   }
   if (utype == V_ASN1_BOOLEAN) {
-    *(ASN1_BOOLEAN *)pval = it->size;
+    *(ASN1_BOOLEAN *)pval = (ASN1_BOOLEAN)it->size;
   } else {
     *pval = NULL;
   }

include/openssl/asn1t.h

@@ -349,7 +349,7 @@ typedef struct ASN1_TLC_st ASN1_TLC;
 
 struct ASN1_TEMPLATE_st {
 uint32_t flags;		/* Various flags */
-long tag;			/* tag, not used if no tagging */
+int tag;			/* tag, not used if no tagging */
 unsigned long offset;		/* Offset of this field in structure */
 const char *field_name;		/* Field name */
 ASN1_ITEM_EXP *item;		/* Relevant ASN1_ITEM or ASN1_ADB */
@@ -455,7 +455,7 @@ struct ASN1_ADB_TABLE_st {
 
 struct ASN1_ITEM_st {
 char itype;			/* The item type, primitive, SEQUENCE, CHOICE or extern */
-long utype;			/* underlying type */
+int utype;			/* underlying type */
 const ASN1_TEMPLATE *templates;	/* If SEQUENCE or CHOICE this contains the contents */
 long tcount;			/* Number of templates if SEQUENCE or CHOICE */
 const void *funcs;		/* functions that handle this type */