commit 380bc30f0c048368b884ec9da90eeda2a28cb1e7
author Adam Langley <alangley@gmail.com> Thu Dec 28 19:39:01 2017 -0800
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Jan 02 16:11:31 2018 +0000
tree 992632518f5d942d9e8adbb340dbd84587083eca
parent f8d05579b4c529a9132ece8e2ab7ae70cee9321e

Fix |ASN1_INTEGER_set| when setting zero.

|ASN1_INTEGER_set| and |BN_to_ASN1_INTEGER| disagree about how to encode
zero. OpenSSL master has aligned around the behaviour of the latter
(i.e. a single zero byte) so fix |ASN1_INTEGER_set| to do that. (This is
also the form that DER requires.)

At the same time, fix undefined behaviour when negative a |long| whose
value is |LONG_MIN|.

Change-Id: I1198de35e61a286ac6472e99152f3d22fda59044
Reviewed-on: https://boringssl-review.googlesource.com/24485
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>

crypto/asn1/a_int.c

diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c
index b53c00b..dd74550 100644
--- a/crypto/asn1/a_int.c
+++ b/crypto/asn1/a_int.c
@@ -347,40 +347,16 @@
 
 int ASN1_INTEGER_set(ASN1_INTEGER *a, long v)
 {
-    int j, k;
-    unsigned int i;
-    unsigned char buf[sizeof(long) + 1];
-    long d;
-
-    a->type = V_ASN1_INTEGER;
-    if (a->length < (int)(sizeof(long) + 1)) {
-        if (a->data != NULL)
-            OPENSSL_free(a->data);
-        if ((a->data =
-             (unsigned char *)OPENSSL_malloc(sizeof(long) + 1)) != NULL)
-            OPENSSL_memset((char *)a->data, 0, sizeof(long) + 1);
-    }
-    if (a->data == NULL) {
-        OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
-        return (0);
-    }
-    d = v;
-    if (d < 0) {
-        d = -d;
-        a->type = V_ASN1_NEG_INTEGER;
+    if (v >= 0) {
+        return ASN1_INTEGER_set_uint64(a, (uint64_t) v);
     }
 
-    for (i = 0; i < sizeof(long); i++) {
-        if (d == 0)
-            break;
-        buf[i] = (int)d & 0xff;
-        d >>= 8;
+    if (!ASN1_INTEGER_set_uint64(a, 0 - (uint64_t) v)) {
+        return 0;
     }
-    j = 0;
-    for (k = i - 1; k >= 0; k--)
-        a->data[j++] = buf[k];
-    a->length = j;
-    return (1);
+
+    a->type = V_ASN1_NEG_INTEGER;
+    return 1;
 }
 
 int ASN1_INTEGER_set_uint64(ASN1_INTEGER *out, uint64_t v)

crypto/asn1/asn1_test.cc

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 55c0f17..7c114dd 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -15,6 +15,7 @@
 #include <stdio.h>
 
 #include <gtest/gtest.h>
+#include <limits.h>
 
 #include <openssl/asn1.h>
 #include <openssl/err.h>
@@ -67,23 +68,23 @@
   bssl::UniquePtr<ASN1_INTEGER> by_uint64(M_ASN1_INTEGER_new());
   bssl::UniquePtr<BIGNUM> bn(BN_new());
 
-  const std::vector<unsigned> kValues = {
-      0, 1, 2, 0xff, 0x100, 0xffff, 0x10000,
+  const std::vector<int64_t> kValues = {
+      LONG_MIN, -2, -1, 0, 1, 2, 0xff, 0x100, 0xffff, 0x10000, LONG_MAX,
   };
   for (const auto &i : kValues) {
     SCOPED_TRACE(i);
 
     ASSERT_EQ(1, ASN1_INTEGER_set(by_long.get(), i));
-    ASSERT_EQ(1, ASN1_INTEGER_set_uint64(by_uint64.get(), i));
-    ASSERT_TRUE(BN_set_word(bn.get(), i));
+    const uint64_t abs = i < 0 ? (0 - (uint64_t) i) : i;
+    ASSERT_TRUE(BN_set_u64(bn.get(), abs));
+    BN_set_negative(bn.get(), i < 0);
     ASSERT_TRUE(BN_to_ASN1_INTEGER(bn.get(), by_bn.get()));
 
-    if (i != 0) {
-      // |ASN1_INTEGER_set| and |BN_to_ASN1_INTEGER| disagree about how to
-      // encode zero. The former leaves an empty value while the latter encodes
-      // as a single zero byte.
-      EXPECT_EQ(0, ASN1_INTEGER_cmp(by_bn.get(), by_long.get()));
+    EXPECT_EQ(0, ASN1_INTEGER_cmp(by_bn.get(), by_long.get()));
+
+    if (i >= 0) {
+      ASSERT_EQ(1, ASN1_INTEGER_set_uint64(by_uint64.get(), i));
+      EXPECT_EQ(0, ASN1_INTEGER_cmp(by_bn.get(), by_uint64.get()));
     }
-    EXPECT_EQ(0, ASN1_INTEGER_cmp(by_bn.get(), by_uint64.get()));
   }
 }