On 03/12/2014 02:14 PM, Wang Weijun wrote:
First, let me clarify again that your webrev is fine. All items below are
unrelated and we can address it with another bug if necessary.
Ok, thank you.
On Mar 12, 2014, at 16:55, Artem Smotrakov <[email protected]> wrote:
Hi Max,
1. DerInputStream.readVector() method is used to decode sets and sequences that
can be encoded with indefinite length. If I understand correctly, currently we
don't support indefinite length for octet string.
According to Xuelei, BER (that supports indefinite length method) is still a
popular format, PKCS#7 is BER based, and JDK accepts PKCS#7 records. I think
that's why it needs to support indefinite length.
I don't know about the details in PKCS #7. Does it say a set/sequence can have
indefinite length, but not octet string? My previous confusion was that we only
support readVector() with indef len but notgetOctetString().
I think that octet string can have indefinite length. I found the
following in RFC 2315 (PKCS #7: Cryptographic Message Syntax):
The values produced according to this document are intended to be
BER-encoded, which means that the values would typically be
represented as octet strings
https://tools.ietf.org/html/rfc2315
If I understand correctly, DerInputStream parses DER sequentially. For example,
if there are two DER sequences, the second one will be parsed when the first
one is done. If we want the data have already been converted to definite length
when a DerInputStream is created, I think we will probably need to re-work all
DER parsing classes to parse all the data at first. It probably can be done in
DerInputStream, DerValue or DerInputBuffer class. Currently DerInputBuffer does
not take care about DER tags at all.
I think it will be significant change, and it can affect performance, and use
more memory. Do you want to re-work current API this way?
No. What I observed is that when a DerInputStream is constructed from a byte[],
the init() method already convert indef len to def len, so there is no need to
care about it in those getXXX() methods. However, a DerInputStream object can
also be created with a DerInputBuffer and there is no conversion there. This
makes the internal state inconsistent.
2. In DerValue.init() method, if fullyBuffered is not true, it tries to read
all available data. Then, it checks tags and length. I think it is more
flexible to let it try to do this regardless fullyBuffered flag. Did I miss
anything?
If fullyBuffered is not true, in.available() will be meaningless and the result of
indef->def conversion is not predictable. I honestly believe that reading an
indef len encoded data in streaming mode is quite difficult.
3. DerIndefLenConverter.convert() method process passed data, and return
newData:
...
// parse and set up the vectors of all the indefinite-lengths
while (dataPos < dataSize) {
parseTag();
...
}
...
newData = new byte[dataSize + numOfTotalLenBytes + unused];
...
// write out the new byte array replacing all the indefinite-lengths
// and EOCs
while (dataPos < dataSize) {
writeTag();
writeLengthAndValue();
}
System.arraycopy(indefData, dataSize,
newData, dataSize + numOfTotalLenBytes, unused);
return newData;
...
The tag should be invariant, I think "if (tag != in.read())" added to make sure
that it is.
My understanding is that since EOC is used to end an existing indef len
structure, it cannot appear as the 1st tag in the BER. Therefore the 1st
writeTag() always write a non-EOC tag into the 1st byte of output, which is
also the 1st byte of the input, so there is no need to check.
Can you show me a counter-example? ;-)
I guess this check was added to make sure that tag value is the same
before DerIndefLenConverter.convert() invocation and after that. If the
check fails, appropriate IOException is thrown immediately. Probably if
there were not this check, and tags differed, it would cause some
failure later. But we want to check this situation right away.
Artem
Thanks
Max
Artem
On 02/26/2014 01:54 PM, Wang Weijun wrote:
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
