commit 468cde90ca58421d63f4dfeaebcf8bb3fccb4127
author David Benjamin <davidben@google.com> Fri Apr 16 15:31:16 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Fri Apr 16 20:15:15 2021 +0000
tree 1ab85c5443cfbaf5fab9c8abfab238b6e6e2d7ae
parent 2f3958a4156d95c7e9778979acda4e43c7d15979

Always encode booleans as DER.

The ASN1_BOOLEAN representation is a mess. ASN1_BOOLEAN is an int
and if non-negative (negative values mean omitted or default), gets cast
to uint8_t and encoded as the value. This means callers are simply
expected to know true is 0xff, not 1. Fix this by only encoding 0 or
0xff.

This also fixes a bug where values like 0x100 are interpreted as true
(e.g. in the tasn_enc.c logic to handle default values), but encoded as
false because the cast only looks at the least significant byte.

This CL does not change the parsing behavior, which is to allow any BER
encoding and preserve the value in the in-memory representation (though
we should tighten that). However the BER encode will no longer be
preserved when re-encoding.

Update-Note: Callers setting ASN1_BOOLEANs to a positive value other
than 0xff will now encode 0xff. This probably fixes a bug, but if anyone
was attaching significance to incorrectly-encoded booleans, that will
break.

Change-Id: I5bb53e068d5900daca07299a27c0551e78ffa91d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46924
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/a_bool.c

diff --git a/crypto/asn1/a_bool.c b/crypto/asn1/a_bool.c
index 34bbd15..64ae9e5 100644
--- a/crypto/asn1/a_bool.c
+++ b/crypto/asn1/a_bool.c
@@ -78,7 +78,7 @@
     }
 
     ASN1_put_object(&p, 0, 1, V_ASN1_BOOLEAN, V_ASN1_UNIVERSAL);
-    *p = (unsigned char)a;
+    *p = a ? 0xff : 0x00;
 
     /*
      * If a new buffer was allocated, just return it back.

crypto/asn1/asn1_test.cc

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 058e07a..90a6d7c 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -26,6 +26,7 @@
 #include <openssl/mem.h>
 #include <openssl/obj.h>
 #include <openssl/span.h>
+#include <openssl/x509v3.h>
 
 #include "../test/test_util.h"
 
@@ -125,11 +126,35 @@
 TEST(ASN1Test, SerializeBoolean) {
   static const uint8_t kTrue[] = {0x01, 0x01, 0xff};
   TestSerialize(0xff, i2d_ASN1_BOOLEAN, kTrue);
+  // Other constants are also correctly encoded as TRUE.
+  TestSerialize(1, i2d_ASN1_BOOLEAN, kTrue);
+  TestSerialize(0x100, i2d_ASN1_BOOLEAN, kTrue);
 
   static const uint8_t kFalse[] = {0x01, 0x01, 0x00};
   TestSerialize(0x00, i2d_ASN1_BOOLEAN, kFalse);
 }
 
+// The templates go through a different codepath, so test them separately.
+TEST(ASN1Test, SerializeEmbeddedBoolean) {
+  bssl::UniquePtr<BASIC_CONSTRAINTS> val(BASIC_CONSTRAINTS_new());
+  ASSERT_TRUE(val);
+
+  // BasicConstraints defaults to FALSE, so the encoding should be empty.
+  static const uint8_t kLeaf[] = {0x30, 0x00};
+  val->ca = 0;
+  TestSerialize(val.get(), i2d_BASIC_CONSTRAINTS, kLeaf);
+
+  // TRUE should always be encoded as 0xff, independent of what value the caller
+  // placed in the |ASN1_BOOLEAN|.
+  static const uint8_t kCA[] = {0x30, 0x03, 0x01, 0x01, 0xff};
+  val->ca = 0xff;
+  TestSerialize(val.get(), i2d_BASIC_CONSTRAINTS, kCA);
+  val->ca = 1;
+  TestSerialize(val.get(), i2d_BASIC_CONSTRAINTS, kCA);
+  val->ca = 0x100;
+  TestSerialize(val.get(), i2d_BASIC_CONSTRAINTS, kCA);
+}
+
 TEST(ASN1Test, ASN1Type) {
   const struct {
     int type;

crypto/asn1/tasn_enc.c

diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c
index 1323439..5ec57a4 100644
--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -569,7 +569,7 @@
             if (!*tbool && !it->size)
                 return -1;
         }
-        c = (unsigned char)*tbool;
+        c = *tbool ? 0xff : 0x00;
         cont = &c;
         len = 1;
         break;