Skip to content

Commit

Permalink
Add additional validations based on X.690 rules
Browse files Browse the repository at this point in the history
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
  • Loading branch information
noloader committed Dec 24, 2016
1 parent 3475a23 commit b19332a
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 16 deletions.
11 changes: 6 additions & 5 deletions asn.cpp
Expand Up @@ -123,7 +123,7 @@ size_t BERDecodeOctetString(BufferedTransformation &bt, SecByteBlock &str)
size_t bc;
if (!BERLengthDecode(bt, bc))
BERDecodeError();
if (bc > bt.MaxRetrievable())
if (bc > bt.MaxRetrievable()) // Issue 346
BERDecodeError();

str.New(bc);
Expand All @@ -141,7 +141,7 @@ size_t BERDecodeOctetString(BufferedTransformation &bt, BufferedTransformation &
size_t bc;
if (!BERLengthDecode(bt, bc))
BERDecodeError();
if (bc > bt.MaxRetrievable())
if (bc > bt.MaxRetrievable()) // Issue 346
BERDecodeError();

bt.TransferTo(str, bc);
Expand All @@ -165,7 +165,7 @@ size_t BERDecodeTextString(BufferedTransformation &bt, std::string &str, byte as
size_t bc;
if (!BERLengthDecode(bt, bc))
BERDecodeError();
if (bc > bt.MaxRetrievable())
if (bc > bt.MaxRetrievable()) // Issue 346
BERDecodeError();

SecByteBlock temp(bc);
Expand Down Expand Up @@ -196,11 +196,12 @@ size_t BERDecodeBitString(BufferedTransformation &bt, SecByteBlock &str, unsigne
BERDecodeError();
if (bc == 0)
BERDecodeError();
if (bc > bt.MaxRetrievable())
if (bc > bt.MaxRetrievable()) // Issue 346
BERDecodeError();

// X.690, 8.6.2.2: "The number [of unused bits] shall be in the range zero to seven"
byte unused;
if (!bt.Get(unused))
if (!bt.Get(unused) || unused > 7)
BERDecodeError();
unusedBits = unused;
str.resize(bc-1);
Expand Down
32 changes: 21 additions & 11 deletions asn.h
Expand Up @@ -55,13 +55,14 @@ enum ASNTag
//! \note These tags and flags are not complete
enum ASNIdFlag
{
UNIVERSAL = 0x00,
// DATA = 0x01,
// HEADER = 0x02,
CONSTRUCTED = 0x20,
APPLICATION = 0x40,
CONTEXT_SPECIFIC = 0x80,
PRIVATE = 0xc0
UNIVERSAL = 0x00,
// DATA = 0x01,
// HEADER = 0x02,
PRIMITIVE = 0x00,
CONSTRUCTED = 0x20,
APPLICATION = 0x40,
CONTEXT_SPECIFIC = 0x80,
PRIVATE = 0xc0
};

//! \brief Raises a BERDecodeErr
Expand Down Expand Up @@ -478,17 +479,17 @@ size_t DEREncodeUnsigned(BufferedTransformation &out, T w, byte asnTag = INTEGER
}

//! \brief BER Decode unsigned value
//! \tparam T class or type
//! \tparam T fundamental C++ type
//! \param in BufferedTransformation object
//! \param w unsigned value to encode
//! \param w the decoded value
//! \param asnTag the ASN.1 type
//! \param minValue the minimum expected value
//! \param maxValue the maximum expected value
//! \throws BERDecodeErr() if the value cannot be parsed or the decoded value is not within range.
//! \details DEREncodeUnsigned() can be used with INTEGER, BOOLEAN, and ENUM
template <class T>
void BERDecodeUnsigned(BufferedTransformation &in, T &w, byte asnTag = INTEGER,
T minValue = 0, T maxValue = ((std::numeric_limits<T>::max)()))
T minValue = 0, T maxValue = T(0xffffffff))
{
byte b;
if (!in.Get(b) || b != asnTag)
Expand All @@ -498,14 +499,23 @@ void BERDecodeUnsigned(BufferedTransformation &in, T &w, byte asnTag = INTEGER,
bool definite = BERLengthDecode(in, bc);
if (!definite)
BERDecodeError();
if (bc > in.MaxRetrievable())
if (bc > in.MaxRetrievable()) // Issue 346
BERDecodeError();
if (asnTag == BOOLEAN && bc != 1) // X.690, 8.2.1
BERDecodeError();
if ((asnTag == INTEGER || asnTag == ENUMERATED) && bc == 0) // X.690, 8.3.1 and 8.4
BERDecodeError();

SecByteBlock buf(bc);

if (bc != in.Get(buf, bc))
BERDecodeError();

// This consumes leading 0 octets. According to X.690, 8.3.2, it could be non-conforming behavior.
// X.690, 8.3.2 says "the bits of the first octet and bit 8 of the second octet ... (a) shall
// not all be ones and (b) shall not all be zeros ... These rules ensure that an integer value
// is always encoded in the smallest possible number of octet".
// We invented AER (Alternate Encoding Rules), which is more relaxed than BER, CER, and DER.
const byte *ptr = buf;
while (bc > sizeof(w) && *ptr == 0)
{
Expand Down

1 comment on commit b19332a

@noloader
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see Issue 346.

Please sign in to comment.