Code Review request: 8028591: NegativeArraySizeException in sun.security.util.DerInputStream.getUnalignedBitString()

[Artem Smotrakov]

Hi Sean,

Thank you for your feedback.

It was confusing to me that the impl supports indefinite-length encoding 
for DER. According to [1], indefinite-length method shall be used for DER:

...
10.1
Length forms
The definite form of length encoding shall be used, encoded in the 
minimum number of octets. [Contrast with 8.1.3.2 b).]
...

But then I found a couple of bugs for support of indefinite-length (for 
example [2]). Probably it is needed for real applications.

I updated the diff:
- added getDefiniteLength() methods that throw IOException in case of 
indefinite-length encoding
- getLength() method, which can return a negative value, is used to 
decode sequences, sets in DerInputStream
- getLength() method is also used in constructor and init() method of 
DerValue class that check for indefinite-length encoding

Tested with available regression, JCK and SQE tests.

Please take a look:

http://cr.openjdk.java.net/~asmotrak/8028591/webrev.01/

[1] Information technology – ASN.1 encoding rules: Specification of 
Basic Encoding Rules (BER), Canonical Encoding Rules (CER) and 
Distinguished Encoding Rules (DER), 
http://www.itu.int/ITU-T/recommendations/rec.aspx?rec=x.690
[2] https://bugs.openjdk.java.net/browse/JDK-4119673: Need to support 
indefinite length DER encodings

Artem

On 02/05/2014 06:37 PM, Sean Mullan wrote:
> Hi Artem,
>
> The specific fix looks fine, but there are many other calls to 
> getLength() in DerInputStream that subsequently initialize an array 
> with the return value, and could also cause the same issue. It seems 
> to me that a better fix would be to pass a flag to the getLength 
> method (or create a new method) and if the flag is true, throw an 
> IOException if an indefinite length encoding is used (instead of 
> returning -1). Then, for the encodings where it is illegal to use the 
> indefinite-length method, change the code to call the method with the 
> flag set to true.
>
> --Sean
>
> On 01/30/2014 03:47 AM, Artem Smotrakov wrote:
>> Please review this fix for 9:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8028591
>> http://cr.openjdk.java.net/~asmotrak/8028591/webrev.00/
>> <http://cr.openjdk.java.net/%7Easmotrak/8028591/webrev.00/>
>>
>> getLength() method is used to get a length of bit string. The method can
>> return a negative value that means indefinite-length encoding that is
>> not allowed in DER. Currently a negative value is not checked. As a
>> result, NegativeArraySizeException can occur.
>>
>> I added the following checks in
>> sun.security.util.DerInputStream.getUnalignedBitString() method:
>> 1. IOException is thrown if getLength() method returns a negative value.
>> 2. Empty BitArray is returned if getLength() method returns zero.
>>
>> I think that an empty bit string should be encoded as "03 01 00" in DER.
>> I am not sure, but probably "03 00" is valid one as well. I tried both
>> ones with OpenSSL asn1parse, and both ones were parsed successfully:
>>
>> hexdump -C emtpy_bit_string_1
>> 00000000 03 01 00 |...|
>> 00000003
>> openssl asn1parse -inform der -in emtpy_bit_string_1
>> 0:d=0 hl=2 l= 1 prim: BIT STRING
>>
>> hexdump -C emtpy_bit_string_2
>> 00000000 03 00 |..|
>> 00000002
>> openssl asn1parse -inform der -in emtpy_bit_string_2
>> 0:d=0 hl=2 l= 0 prim: BIT STRING
>>
>> 3. IOException is thrown if number of calculated valid bits is negative.
>>
>> Added a test case for
>> test/java/security/cert/X509Certificate/X509BadCertificate.java
>> (bad-cert-2.pem is corrupted self-signed certificate). Tested with
>> available regression, SQE and JCK tests.
>>
>> Artem
>