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

[Wang Weijun]

Hi Artem

The code change looks fine. It seems all your s/getLength/getDefiniteLength/ substitutions are those that only works with a definite length.

However, I do find the indefinite length support not satisfying. Just not sure if it's worth fixing. For example:

1. No idea why DerImputStream::readVector supports indefinite length. Shouldn't the data already have already been converted to definite length when a DerImputStream is created? Or maybe it's created from a DerInputBuffer that has not been converted? Then why don't getOctetString do the same?

2. In DerValue::init, if fullyBuffered is not true, then indefinite length should not be supported

3. In the same method above, I have no idea why "if (tag != in.read())" is checked after the conversion. Is it possible to be false?

Thanks
Max

On Feb 26, 2014, at 15:41, Artem Smotrakov <artem.smotrakov at oracle.com> wrote:

> 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
>> 
>