Wrong assumptions in ASN.1 BER reader

[phofman]

Please correct me, if I am wrong ;) but I suspect there are some wrong assumptions around ASN.1 BER reader functionality.

I put them all here:

• BerTag values greater than 30 are not supported (checkout point 8.1.2.4.1 of the X.609) and it might cause invalid reads after that tag
• indefinite-form of length is not clearly indicated at all to the upper layer (point 8.1.3.6 of the X.609 above); even if current implementation doesn't cause invalid reads instantly, the upper layer won't know, that it needs to process content until End-Of-Contents DataElement (two zero octets)
• reading lenght doesn't handle end-of-stream correctly and can consume -1 value
• lengthOfLength with all bits set is also a reserved value
• is there any reason for 8kb length limit (beside of handling both cases above)?
• decoded bit-stream doesn't need to be always lead by '0' octet (as applied here to X.509 RSA key info); according to standard first octet describes a number of unused bits at the end of the received stream (point 8.6.2.2); this is just probably 0 for RSA keys

[AArnott]

Thank you for your careful review and report, @phofman. It's been a while since I wrote that code, so I'll have to take some time to refamiliarize myself with it and the specs I was reading at the time. But I can already see at least some of what you say has merit. So I appreciate it!

[AArnott]

BTW can I ask whether any of these issues have manifested in a malfunction that you've observed or can predict in a regular scenario? In other words, is this just spec-conformance or do you expect a valid ASN1 sequence that is likely to occur in the wild may be misinterpreted?

[phofman]

As for now everything works fine in the wild and I haven't got any problems with current ASN.1 implementation.

My comments come from a quick code review of KeyFormatter to find possible cause of #59 (around exporting RSAParameters) and to give me more knowledge of the library and spec compliance ;) I am investigating scenario of adding import/export options from OpenSSH file formats (PEM included).

[AArnott]

I am investigating scenario of adding import/export options from OpenSSH file formats (PEM included).

Cool. That sounds like a worthwhile PR. :)

[AArnott]

to find possible cause of #59

This is most likely due to Pkcs1PrependZeros setting. A recent commit touched up some of these which might have regressed this scenario.

[AArnott]

BerTag values greater than 30 are not supported (checkout point 8.1.2.4.1 of the X.609) and it might cause invalid reads after that tag

Where do you see this limitation in code? Is it the 0x1f mask I use? Are you proposing I use 0x1e instead?

•indefinite-form of length is not clearly indicated at all to the upper layer (point 8.1.3.6 of the X.609 above); even if current implementation doesn't cause invalid reads instantly, the upper layer won't know, that it needs to process content until End-Of-Contents DataElement (two zero octets)

Can you point out the code you're referring to?

•reading lenght doesn't handle end-of-stream correctly and can consume -1 value

Yes, I can fix that one. By throwing for an invalid sequence, I suppose.

• lengthOfLength with all bits set is also a reserved value

I'll review this.

•is there any reason for 8kb length limit (beside of handling both cases above)?

If there's no reason to believe it will be exceeded in a reasonable scenario, limiting it can mitigate risk of bad input consuming huge amounts of memory or CPU.

•decoded bit-stream doesn't need to be always lead by '0' octet (as applied here to X.509 RSA key info); according to standard first octet describes a number of unused bits at the end of the received stream (point 8.6.2.2); this is just probably 0 for RSA keys

IIRC, the zero I'm trimming is the optional leading zero that prevents the large integer from being interpreted as negative due to a leading 1 bit, and can cause array size mismatches for CAPI, which is problematic for some folks (like bug #59 you refer to).

[phofman]

BerTag values greater than 30 are not supported (checkout point 8.1.2.4.1 of the X.609) and it might cause invalid reads after that tag

Where do you see this limitation in code? Is it the 0x1f mask I use? Are you proposing I use 0x1e instead?

The mask 0x1F is OK. However when the tag has all bits set (becomes UseLongForm), it expands to following octets. If you don't skip those extra bytes, they will cause improper interpretation of length field.

indefinite-form of length is not clearly indicated at all to the upper layer (point 8.1.3.6 of the X.609 above); even if current implementation doesn't cause invalid reads instantly, the upper layer won't know, that it needs to process content until End-Of-Contents DataElement (two zero octets)

Can you point out the code you're referring to?

I am mostly concerned about lengthOfLenght calculation and execution of following loop (here). It won't crash, it will simply return 0-length content (as indefinite length uses 0x80 value). The caller has then no way to know, that should keep parsing following DataElements until End-Of-Contents one. Maybe it could return null content or I am exaggerating ;) ? Here is an example from X.609 standard (page 9):

decoded bit-stream doesn't need to be always lead by '0' octet (as applied here to X.509 RSA key info); according to standard first octet describes a number of unused bits at the end of the received stream (point 8.6.2.2); this is just probably 0 for RSA keys

IIRC, the zero I'm trimming is the optional leading zero that prevents the large integer from being interpreted as negative due to a leading 1 bit, and can cause array size mismatches for CAPI, which is problematic for some folks (like bug #59 you refer to).

I am not so sure. First here the leading '0' is removed from a BitString DataElement, which is later read as a Sequence DataElement by PKCS#1 formatter here. Internally inside this Sequence there are 'exponent' and 'modulus', where the latter one is prefixed with '0' octet.

About BitString - ASN.1 :: 8.6.2.2 The initial octet shall encode, as an unsigned binary integer with bit 1 as the least significant bit, the number of unused bits in the final subsequent octet. The number shall be in the range zero to seven.