commit dd444b1d8e2be09532e1601bd63b8212098b105c
author Steven Valdez <svaldez@google.com> Thu May 03 13:15:12 2018 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu May 03 17:40:43 2018 +0000
tree 301876b776b0d64493d76d12d98b3ee173ec1909
parent 0c9ac2e7bf6f5d611ee5ff6f47eb98bcc2855a5a

Fix bugs in X509_NAME_add_entry.

|set| should be evaluated to determine whether to insert/append before
it is reused as a temporary variable.

When incrementing the |set| of X509_NAME_ENTRY, the inserted entry
should not be incremented.

Thanks to Ingo Schwarze for extensive debugging and the initial
fix.

(Imported from upstream bbf27cd58337116c57a1c942153330ff83d5540a)

Change-Id: Ib45d92fc6d52d7490b01d3c475eafc42dd6ef721
Reviewed-on: https://boringssl-review.googlesource.com/28005
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 0c5fc2d..7615324 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1259,3 +1259,27 @@
     }
   }
 }
+
+TEST(X509Test, X509NameSet) {
+  bssl::UniquePtr<X509_NAME> name(X509_NAME_new());
+  EXPECT_TRUE(X509_NAME_add_entry_by_txt(
+      name.get(), "C", MBSTRING_ASC, reinterpret_cast<const uint8_t *>("US"),
+      -1, -1, 0));
+  EXPECT_EQ(X509_NAME_entry_count(name.get()), 1);
+  EXPECT_TRUE(X509_NAME_add_entry_by_txt(
+      name.get(), "C", MBSTRING_ASC, reinterpret_cast<const uint8_t *>("CA"),
+      -1, -1, 0));
+  EXPECT_EQ(X509_NAME_entry_count(name.get()), 2);
+  EXPECT_TRUE(X509_NAME_add_entry_by_txt(
+      name.get(), "C", MBSTRING_ASC, reinterpret_cast<const uint8_t *>("UK"),
+      -1, -1, 0));
+  EXPECT_EQ(X509_NAME_entry_count(name.get()), 3);
+  EXPECT_TRUE(X509_NAME_add_entry_by_txt(
+      name.get(), "C", MBSTRING_ASC, reinterpret_cast<const uint8_t *>("JP"),
+      -1, 1, 0));
+  EXPECT_EQ(X509_NAME_entry_count(name.get()), 4);
+
+  // Check that the correct entries get incremented when inserting new entry.
+  EXPECT_EQ(X509_NAME_ENTRY_set(X509_NAME_get_entry(name.get(), 1)), 1);
+  EXPECT_EQ(X509_NAME_ENTRY_set(X509_NAME_get_entry(name.get(), 2)), 2);
+}

crypto/x509/x509name.c

diff --git a/crypto/x509/x509name.c b/crypto/x509/x509name.c
index 610de5f..fa621f2 100644
--- a/crypto/x509/x509name.c
+++ b/crypto/x509/x509name.c
@@ -238,6 +238,7 @@
     else if (loc < 0)
         loc = n;
 
+    inc = (set == 0);
     name->modified = 1;
 
     if (set == -1) {
@@ -246,7 +247,6 @@
             inc = 1;
         } else {
             set = sk_X509_NAME_ENTRY_value(sk, loc - 1)->set;
-            inc = 0;
         }
     } else {                    /* if (set >= 0) */
 
@@ -257,7 +257,6 @@
                 set = 0;
         } else
             set = sk_X509_NAME_ENTRY_value(sk, loc)->set;
-        inc = (set == 0) ? 1 : 0;
     }
 
     if ((new_name = X509_NAME_ENTRY_dup(ne)) == NULL)
@@ -270,7 +269,7 @@
     if (inc) {
         n = sk_X509_NAME_ENTRY_num(sk);
         for (i = loc + 1; i < n; i++)
-            sk_X509_NAME_ENTRY_value(sk, i - 1)->set += 1;
+            sk_X509_NAME_ENTRY_value(sk, i)->set += 1;
     }
     return (1);
  err: