bug 1060929 - mozilla::pkix: allow explicit encodings of default-valued BOOLEANs for compatibility r=briansmith
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 22 Sep 2014 09:26:10 -0700
changeset 206594 0c50a30b5be7d1ddfd06fc530a1daf0533bc62f0
parent 206593 1f03487d29049341b16ee3c882fe56266be9b2f5
child 206595 8e12b1e8a2f51f06f11a6c2cff24fcf7493a1ed6
push idunknown
push userunknown
push dateunknown
reviewersbriansmith
bugs1060929
milestone35.0a1
bug 1060929 - mozilla::pkix: allow explicit encodings of default-valued BOOLEANs for compatibility r=briansmith
security/pkix/lib/pkixcheck.cpp
security/pkix/lib/pkixder.h
security/pkix/test/gtest/pkixder_universal_types_tests.cpp
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -299,23 +299,17 @@ static const long UNLIMITED_PATH_LEN = -
 
 //  BasicConstraints ::= SEQUENCE {
 //          cA                      BOOLEAN DEFAULT FALSE,
 //          pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
 static Result
 DecodeBasicConstraints(Reader& input, /*out*/ bool& isCA,
                        /*out*/ long& pathLenConstraint)
 {
-  // TODO(bug 989518): cA is by default false. According to DER, default
-  // values must not be explicitly encoded in a SEQUENCE. So, if this
-  // value is present and false, it is an encoding error. However, Go Daddy
-  // has issued many certificates with this improper encoding, so we can't
-  // enforce this yet (hence passing true for allowInvalidExplicitEncoding
-  // to der::OptionalBoolean).
-  if (der::OptionalBoolean(input, true, isCA) != Success) {
+  if (der::OptionalBoolean(input, isCA) != Success) {
     return Result::ERROR_EXTENSION_VALUE_INVALID;
   }
 
   // TODO(bug 985025): If isCA is false, pathLenConstraint MUST NOT
   // be included (as per RFC 5280 section 4.2.1.9), but for compatibility
   // reasons, we don't check this for now.
   if (der::OptionalInteger(input, UNLIMITED_PATH_LEN, pathLenConstraint)
         != Success) {
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -278,33 +278,31 @@ Boolean(Reader& input, /*out*/ bool& val
   switch (intValue) {
     case 0: value = false; return Success;
     case 0xFF: value = true; return Success;
     default:
       return Result::ERROR_BAD_DER;
   }
 }
 
-// This is for any BOOLEAN DEFAULT FALSE.
-// (If it is present and false, this is a bad encoding.)
-// TODO(bug 989518): For compatibility reasons, in some places we allow
-// invalid encodings with the explicit default value.
+// This is for BOOLEAN DEFAULT FALSE.
+// The standard stipulates that "The encoding of a set value or sequence value
+// shall not include an encoding for any component value which is equal to its
+// default value." However, it appears to be common that other libraries
+// incorrectly include the value of a BOOLEAN even when it's equal to the
+// default value, so we allow invalid explicit encodings here.
 inline Result
-OptionalBoolean(Reader& input, bool allowInvalidExplicitEncoding,
-                /*out*/ bool& value)
+OptionalBoolean(Reader& input, /*out*/ bool& value)
 {
   value = false;
   if (input.Peek(BOOLEAN)) {
     Result rv = Boolean(input, value);
     if (rv != Success) {
       return rv;
     }
-    if (!allowInvalidExplicitEncoding && !value) {
-      return Result::ERROR_BAD_DER;
-    }
   }
   return Success;
 }
 
 // This parser will only parse values between 0..127. If this range is
 // increased then callers will need to be changed.
 inline Result
 Enumerated(Reader& input, uint8_t& value)
@@ -538,17 +536,17 @@ OptionalExtensions(Reader& input, uint8_
     //      extnValue   OCTET STRING
     //      }
     Reader extnID;
     rv = ExpectTagAndGetValue(extension, OIDTag, extnID);
     if (rv != Success) {
       return rv;
     }
     bool critical;
-    rv = OptionalBoolean(extension, false, critical);
+    rv = OptionalBoolean(extension, critical);
     if (rv != Success) {
       return rv;
     }
     Input extnValue;
     rv = ExpectTagAndGetValue(extension, OCTET_STRING, extnValue);
     if (rv != Success) {
       return rv;
     }
--- a/security/pkix/test/gtest/pkixder_universal_types_tests.cpp
+++ b/security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ -121,58 +121,57 @@ TEST_F(pkixder_universal_types_tests, Bo
 
   bool value = true;
   ASSERT_EQ(Result::ERROR_BAD_DER, Boolean(reader, value));
 }
 
 // OptionalBoolean implements decoding of OPTIONAL BOOLEAN DEFAULT FALSE.
 // If the field is present, it must be a valid encoding of a BOOLEAN with
 // value TRUE. If the field is not present, it defaults to FALSE. For
-// compatibility reasons, OptionalBoolean can be told to accept an encoding
-// where the field is present with value FALSE (this is technically not a
-// valid DER encoding).
+// compatibility reasons, OptionalBoolean also accepts encodings where the field
+// is present with value FALSE (this is technically not a valid DER encoding).
 TEST_F(pkixder_universal_types_tests, OptionalBooleanValidEncodings)
 {
   {
     const uint8_t DER_OPTIONAL_BOOLEAN_PRESENT_TRUE[] = {
       0x01,                       // BOOLEAN
       0x01,                       // length
       0xff                        // true
     };
     Input input(DER_OPTIONAL_BOOLEAN_PRESENT_TRUE);
     Reader reader(input);
     bool value = false;
-    ASSERT_EQ(Success, OptionalBoolean(reader, false, value)) <<
+    ASSERT_EQ(Success, OptionalBoolean(reader, value)) <<
       "Should accept the only valid encoding of a present OPTIONAL BOOLEAN";
     ASSERT_TRUE(value);
     ASSERT_TRUE(reader.AtEnd());
   }
 
   {
     // The OPTIONAL BOOLEAN is omitted in this data.
     const uint8_t DER_INTEGER_05[] = {
       0x02,                       // INTEGER
       0x01,                       // length
       0x05
     };
     Input input(DER_INTEGER_05);
     Reader reader(input);
     bool value = true;
-    ASSERT_EQ(Success, OptionalBoolean(reader, false, value)) <<
+    ASSERT_EQ(Success, OptionalBoolean(reader, value)) <<
       "Should accept a valid encoding of an omitted OPTIONAL BOOLEAN";
     ASSERT_FALSE(value);
     ASSERT_FALSE(reader.AtEnd());
   }
 
   {
     Input input;
     ASSERT_EQ(Success, input.Init(reinterpret_cast<const uint8_t*>(""), 0));
     Reader reader(input);
     bool value = true;
-    ASSERT_EQ(Success, OptionalBoolean(reader, false, value)) <<
+    ASSERT_EQ(Success, OptionalBoolean(reader, value)) <<
       "Should accept another valid encoding of an omitted OPTIONAL BOOLEAN";
     ASSERT_FALSE(value);
     ASSERT_TRUE(reader.AtEnd());
   }
 }
 
 TEST_F(pkixder_universal_types_tests, OptionalBooleanInvalidEncodings)
 {
@@ -180,54 +179,35 @@ TEST_F(pkixder_universal_types_tests, Op
     0x01,                       // BOOLEAN
     0x01,                       // length
     0x00                        // false
   };
 
   {
     Input input(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE);
     Reader reader(input);
-    bool value;
-    // If the second parameter to OptionalBoolean is false, invalid encodings
-    // that include the field even when it is the DEFAULT FALSE are rejected.
-    bool allowInvalidEncodings = false;
-    ASSERT_EQ(Result::ERROR_BAD_DER,
-              OptionalBoolean(reader, allowInvalidEncodings, value)) <<
-      "Should reject an invalid encoding of present OPTIONAL BOOLEAN";
-  }
-
-  {
-    Input input(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE);
-    Reader reader(input);
     bool value = true;
-    // If the second parameter to OptionalBoolean is true, invalid encodings
-    // that include the field even when it is the DEFAULT FALSE are accepted.
-    bool allowInvalidEncodings = true;
-    ASSERT_EQ(Success, OptionalBoolean(reader, allowInvalidEncodings, value)) <<
-      "Should now accept an invalid encoding of present OPTIONAL BOOLEAN";
+    ASSERT_EQ(Success, OptionalBoolean(reader, value)) <<
+      "Should accept an invalid, default-value encoding of OPTIONAL BOOLEAN";
     ASSERT_FALSE(value);
     ASSERT_TRUE(reader.AtEnd());
   }
 
   const uint8_t DER_OPTIONAL_BOOLEAN_PRESENT_42[] = {
     0x01,                       // BOOLEAN
     0x01,                       // length
     0x42                        // (invalid value for a BOOLEAN)
   };
 
   {
     Input input(DER_OPTIONAL_BOOLEAN_PRESENT_42);
     Reader reader(input);
-    // Even with the second parameter to OptionalBoolean as true, encodings
-    // of BOOLEAN that are invalid altogether are rejected.
-    bool allowInvalidEncodings = true;
     bool value;
-    ASSERT_EQ(Result::ERROR_BAD_DER,
-              OptionalBoolean(reader, allowInvalidEncodings, value)) <<
-      "Should reject another invalid encoding of present OPTIONAL BOOLEAN";
+    ASSERT_EQ(Result::ERROR_BAD_DER, OptionalBoolean(reader, value)) <<
+      "Should reject an invalid-valued encoding of OPTIONAL BOOLEAN";
   }
 }
 
 TEST_F(pkixder_universal_types_tests, Enumerated)
 {
   const uint8_t DER_ENUMERATED[] = {
     0x0a,                       // ENUMERATED
     0x01,                       // length