Fix undefined shift in asn1 parsing

[catenacyber]

Found by oss-fuzz
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18102

This is similar to a previous fix some time ago :
https://github.com/ARMmbed/mbedtls/pull/1625/files#diff-8d3acce410d251c6ead39efd401db23eR3316

[gilles-peskine-arm]

Thanks for the proposed fix, but I think it isn't right.

I've already started to look at the issue and expect to have a fix soon.

[gilles-peskine-arm]

That results in the wrong value when the value fits in an unsigned int but not in an int, doesn't it?

[athoelke]

The undefined behaviour is one thing, but the entire function looks ... tricky.

If I was to reverse engineer the ASN1 INTEGER spec from this code I would have to deduce that:

1. an empty byte string is not a valid ASN1 integer.
2. the value zero can be represented as any number (>=1) of 00 bytes.
3. the bytes 00 00 00 00 00 00 00 00 00 00 12 34 56 78 are a valid ASN1 integer, with the value 0x12345678
4. the byte 87 is not a valid integer in ASN1, presumably 0x87 has to rendered as 00 87?
5. the bytes 87 65 43 21 are not a valid integer in ASN1
6. the bytes 00 87 65 43 21 are a valid ASN1 integer in ASN1, with 'value' 0x87654321. Which is negative.
7. the bytes 12 34 56 78 00 are not a valid integer in ASN1.

The strangest artefact of the implementation is the (**p & 0x80) != 0 test which rejects values that have msb set (e.g. 4 above). But it doesn't reject values >=2³¹ which in our systems are cast to negative signed integer values (see 6 above).

[athoelke]

.. and now I've read teh ASN.1 DER encoding rules... and this function implementation (after the UB fix):

1. is too relaxed about leading zeros (each value has exactly one valid representation)
2. errors on correctly represented negative values
3. returns a negative result for correctly formatted postive integers in the range 2³¹ <= v < 2³²

Item 1 does no substantial harm. Item 2 might make some scenarios fail that should succeed - but the scope of ASN.1 usage in this library might not require negative integer values. Item 3 is a real defect, with unclear consequences.

[gilles-peskine-arm]

now I've read teh ASN.1 DER encoding rules

Commiserations.

is too relaxed about leading zeros

We don't necessarily reject non-BER encodings. It's a matter of interoperability. I'd like to tighten things up, because being liberal in what you accept is not such a good idea, but we can only do that after a careful review to ensure we don't break somebody's workflow.

errors on correctly represented negative values

This is cryptography. We don't deal in negative values. The fact that the encoding supports them is an unfortunate historical relic.

returns a negative result for correctly formatted postive integers

Or rather, since this is a signed int, triggered undefined behavior, which in many environments would manifest itself by returning a negative result. The correct behavior is actually to return an error since the number is out of the representable range in this case.

[gilles-peskine-arm]

If I was to reverse engineer the ASN1 INTEGER spec

Note that we don't exactly implement the ASN.1 specification or even a subset of it. We implement something that is close to a subset of it, but that works in the real world which is a vicious circle of people not quite implementing the specification correctly and the rest having to follow through for interoperability and backward compatibility.

[gilles-peskine-arm]

I'm closing in favor of a tested fix in #297