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 <[email protected]> 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 >> >
