[PATCH] Add additional validations based on X.690 rules

The library was a tad bit fast and loose with respect to parsing some of the ASN.1 presented to it. It was kind of like we used Alternate Encoding Rules (AER), which was more relaxed than BER, CER or DER. This commit closes most of the gaps.

The changes are distantly related to Issue 346. Issue 346 caught a CVE bcause of the transient DoS. These fixes did not surface with negative effcts. Rather, the library was a bit too accomodating to the point it was not conforming

Gbp-Pq: Name Additional_ASN.1_validations.patch

diff --git a/asn.cpp b/asn.cpp
index 0017f28fa792a864226892da40110fe798894acd..26fc649bf2c0e28081d3ab1cea997d2cdc99701a 100644 (file)
--- a/asn.cpp
+++ b/asn.cpp
@@ -123,7 +123,7 @@ size_t BERDecodeOctetString(BufferedTransformation &bt, SecByteBlock &str)
        size_t bc;\r
        if (!BERLengthDecode(bt, bc))\r
                BERDecodeError();\r
-       if (bc > bt.MaxRetrievable())\r
+       if (bc > bt.MaxRetrievable()) // Issue 346\r
                BERDecodeError();\r
 \r
        str.New(bc);\r
@@ -141,7 +141,7 @@ size_t BERDecodeOctetString(BufferedTransformation &bt, BufferedTransformation &
        size_t bc;\r
        if (!BERLengthDecode(bt, bc))\r
                BERDecodeError();\r
-       if (bc > bt.MaxRetrievable())\r
+       if (bc > bt.MaxRetrievable()) // Issue 346\r
                BERDecodeError();\r
 \r
        bt.TransferTo(str, bc);\r
@@ -165,7 +165,7 @@ size_t BERDecodeTextString(BufferedTransformation &bt, std::string &str, byte as
        size_t bc;\r
        if (!BERLengthDecode(bt, bc))\r
                BERDecodeError();\r
-       if (bc > bt.MaxRetrievable())\r
+       if (bc > bt.MaxRetrievable()) // Issue 346\r
                BERDecodeError();\r
 \r
        SecByteBlock temp(bc);\r
@@ -196,11 +196,12 @@ size_t BERDecodeBitString(BufferedTransformation &bt, SecByteBlock &str, unsigne
                BERDecodeError();\r
        if (bc == 0)\r
                BERDecodeError();\r
-       if (bc > bt.MaxRetrievable())\r
+       if (bc > bt.MaxRetrievable()) // Issue 346\r
                BERDecodeError();\r
 \r
+       // X.690, 8.6.2.2: "The number [of unused bits] shall be in the range zero to seven"\r
        byte unused;\r
-       if (!bt.Get(unused))\r
+       if (!bt.Get(unused) || unused > 7)\r
                BERDecodeError();\r
        unusedBits = unused;\r
        str.resize(bc-1);\r

diff --git a/asn.h b/asn.h
index adb677d3da9c30575862b36084f1e54d1da0cf65..d787ddda61d34e59b85caf8850a239c9f739c586 100644 (file)
--- a/asn.h
+++ b/asn.h
@@ -48,13 +48,14 @@ enum ASNTag
 //! \note These tags and flags are not complete\r
 enum ASNIdFlag\r
 {\r
-       UNIVERSAL                       = 0x00,\r
-//     DATA                            = 0x01,\r
-//     HEADER                          = 0x02,\r
-       CONSTRUCTED             = 0x20,\r
-       APPLICATION             = 0x40,\r
-       CONTEXT_SPECIFIC        = 0x80,\r
-       PRIVATE                         = 0xc0\r
+       UNIVERSAL           = 0x00,\r
+//     DATA                = 0x01,\r
+//     HEADER              = 0x02,\r
+       PRIMITIVE           = 0x00,\r
+       CONSTRUCTED         = 0x20,\r
+       APPLICATION         = 0x40,\r
+       CONTEXT_SPECIFIC    = 0x80,\r
+       PRIVATE             = 0xc0\r
 };\r
 \r
 //! \brief Raises a BERDecodeErr\r
@@ -466,9 +467,9 @@ size_t DEREncodeUnsigned(BufferedTransformation &out, T w, byte asnTag = INTEGER
 }\r
 \r
 //! \brief BER Decode unsigned value\r
-//! \tparam T class or type\r
+//! \tparam T fundamental C++ type\r
 //! \param in BufferedTransformation object\r
-//! \param w unsigned value to encode\r
+//! \param w the decoded value\r
 //! \param asnTag the ASN.1 type\r
 //! \param minValue the minimum expected value\r
 //! \param maxValue the maximum expected value\r
@@ -476,7 +477,7 @@ size_t DEREncodeUnsigned(BufferedTransformation &out, T w, byte asnTag = INTEGER
 //! \details DEREncodeUnsigned() can be used with INTEGER, BOOLEAN, and ENUM\r
 template <class T>\r
 void BERDecodeUnsigned(BufferedTransformation &in, T &w, byte asnTag = INTEGER,\r
-                                          T minValue = 0, T maxValue = ((std::numeric_limits<T>::max)()))\r
+                                          T minValue = 0, T maxValue = T(0xffffffff))\r
 {\r
        byte b;\r
        if (!in.Get(b) || b != asnTag)\r
@@ -486,7 +487,11 @@ void BERDecodeUnsigned(BufferedTransformation &in, T &w, byte asnTag = INTEGER,
        bool definite = BERLengthDecode(in, bc);\r
        if (!definite)\r
                BERDecodeError();\r
-       if (bc > in.MaxRetrievable())\r
+       if (bc > in.MaxRetrievable())  // Issue 346\r
+               BERDecodeError();\r
+       if (asnTag == BOOLEAN && bc != 1) // X.690, 8.2.1\r
+               BERDecodeError();\r
+       if ((asnTag == INTEGER || asnTag == ENUMERATED) && bc == 0) // X.690, 8.3.1 and 8.4\r
                BERDecodeError();\r
 \r
        SecByteBlock buf(bc);\r
@@ -494,6 +499,11 @@ void BERDecodeUnsigned(BufferedTransformation &in, T &w, byte asnTag = INTEGER,
        if (bc != in.Get(buf, bc))\r
                BERDecodeError();\r
 \r
+       // This consumes leading 0 octets. According to X.690, 8.3.2, it could be non-conforming behavior.\r
+       //  X.690, 8.3.2 says "the bits of the first octet and bit 8 of the second octet ... (a) shall\r
+       //  not all be ones and (b) shall not all be zeros ... These rules ensure that an integer value\r
+       //  is always encoded in the smallest possible number of octet".\r
+       // We invented AER (Alternate Encoding Rules), which is more relaxed than BER, CER, and DER.\r
        const byte *ptr = buf;\r
        while (bc > sizeof(w) && *ptr == 0)\r
        {\r