Bug 1060929 - mozilla::pkix: Allow explicit encodings of default-valued BOOLEANs because lol standards. r=briansmith, a=sledru
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 22 Sep 2014 09:26:10 -0700
changeset 216843 008eb429e655
parent 216842 89d93cece9fd
child 216844 c043fec932a6
push id3939
push userryanvm@gmail.com
push date2014-09-25 15:14 +0000
treeherdermozilla-beta@c043fec932a6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith, sledru
bugs1060929
milestone33.0
Bug 1060929 - mozilla::pkix: Allow explicit encodings of default-valued BOOLEANs because lol standards. r=briansmith, a=sledru
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(Input& 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
@@ -299,33 +299,31 @@ Boolean(Input& input, /*out*/ bool& valu
   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(Input& input, bool allowInvalidExplicitEncoding,
-                /*out*/ bool& value)
+OptionalBoolean(Input& 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(Input& input, uint8_t& value)
@@ -544,17 +542,17 @@ OptionalExtensions(Input& input, uint8_t
     //      extnValue   OCTET STRING
     //      }
     Input 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;
     }
     SECItem 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
@@ -134,105 +134,87 @@ TEST_F(pkixder_universal_types_tests, Bo
 
   bool value = true;
   ASSERT_EQ(Result::ERROR_BAD_DER, Boolean(input, 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 input1;
   ASSERT_EQ(Success, input1.Init(DER_OPTIONAL_BOOLEAN_PRESENT_TRUE,
                                  sizeof DER_OPTIONAL_BOOLEAN_PRESENT_TRUE));
   bool value = false;
-  ASSERT_EQ(Success, OptionalBoolean(input1, false, value)) <<
+  ASSERT_EQ(Success, OptionalBoolean(input1, value)) <<
     "Should accept the only valid encoding of a present OPTIONAL BOOLEAN";
   ASSERT_TRUE(value);
   ASSERT_TRUE(input1.AtEnd());
 
   // The OPTIONAL BOOLEAN is omitted in this data.
   const uint8_t DER_INTEGER_05[] = {
     0x02,                       // INTEGER
     0x01,                       // length
     0x05
   };
 
   Input input2;
   ASSERT_EQ(Success, input2.Init(DER_INTEGER_05, sizeof DER_INTEGER_05));
   value = true;
-  ASSERT_EQ(Success, OptionalBoolean(input2, false, value)) <<
+  ASSERT_EQ(Success, OptionalBoolean(input2, value)) <<
     "Should accept a valid encoding of an omitted OPTIONAL BOOLEAN";
   ASSERT_FALSE(value);
   ASSERT_FALSE(input2.AtEnd());
 
   Input input3;
   ASSERT_EQ(Success, input3.Init(reinterpret_cast<const uint8_t*>(""), 0));
   value = true;
-  ASSERT_EQ(Success, OptionalBoolean(input3, false, value)) <<
+  ASSERT_EQ(Success, OptionalBoolean(input3, value)) <<
     "Should accept another valid encoding of an omitted OPTIONAL BOOLEAN";
   ASSERT_FALSE(value);
   ASSERT_TRUE(input3.AtEnd());
 }
 
 TEST_F(pkixder_universal_types_tests, OptionalBooleanInvalidEncodings)
 {
   const uint8_t DER_OPTIONAL_BOOLEAN_PRESENT_FALSE[] = {
     0x01,                       // BOOLEAN
     0x01,                       // length
     0x00                        // false
   };
 
   Input input1;
   ASSERT_EQ(Success, input1.Init(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE,
                                  sizeof DER_OPTIONAL_BOOLEAN_PRESENT_FALSE));
-  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(input1, allowInvalidEncodings, value)) <<
-    "Should reject an invalid encoding of present OPTIONAL BOOLEAN";
-
-  Input input2;
-  ASSERT_EQ(Success, input2.Init(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE,
-                                 sizeof DER_OPTIONAL_BOOLEAN_PRESENT_FALSE));
-  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.
-  allowInvalidEncodings = true;
-  ASSERT_EQ(Success, OptionalBoolean(input2, allowInvalidEncodings, value)) <<
-    "Should now accept an invalid encoding of present OPTIONAL BOOLEAN";
+  bool value = true;
+  ASSERT_EQ(Success, OptionalBoolean(input1, value)) <<
+    "Should accept an invalid, default-value encoding of OPTIONAL BOOLEAN";
   ASSERT_FALSE(value);
-  ASSERT_TRUE(input2.AtEnd());
+  ASSERT_TRUE(input1.AtEnd());
 
   const uint8_t DER_OPTIONAL_BOOLEAN_PRESENT_42[] = {
     0x01,                       // BOOLEAN
     0x01,                       // length
     0x42                        // (invalid value for a BOOLEAN)
   };
 
-  Input input3;
-  ASSERT_EQ(Success, input3.Init(DER_OPTIONAL_BOOLEAN_PRESENT_42,
+  Input input2;
+  ASSERT_EQ(Success, input2.Init(DER_OPTIONAL_BOOLEAN_PRESENT_42,
                                  sizeof DER_OPTIONAL_BOOLEAN_PRESENT_42));
-  // Even with the second parameter to OptionalBoolean as true, encodings
-  // of BOOLEAN that are invalid altogether are rejected.
-  ASSERT_EQ(Result::ERROR_BAD_DER,
-            OptionalBoolean(input3, allowInvalidEncodings, value)) <<
-    "Should reject another invalid encoding of present OPTIONAL BOOLEAN";
+  ASSERT_EQ(Result::ERROR_BAD_DER, OptionalBoolean(input2, 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
     0x42                        // value