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

[Sean Mullan]

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