On Sat, 26 Sep 2020 00:04:28 GMT, Weijun Wang <[email protected]> wrote:
>> src/java.base/share/classes/sun/security/util/DerValue.java line 284:
>>
>>> 282:
>>> 283: /**
>>> 284: * Parse an ASN.1/BER encoded datum. The entire encoding must hold
>>> exactly
>>
>> Are you intentionally put BER here because allowBER=true?
>
> Yes. In fact I'm not quite confident on our usage of allowBER. In a lot of
> cases we are actually quite strict. Since
> this code change is meant to be a refactoring and does not intend to fix many
> things, I don't intent to make many
> behavior change.
We have to be strict in "sensitive" area such as signatures. The parsing code
seems to be still mainly DER. It was
never fully BER, but just some. It's good to keep behavior change minimum as
this is like a re-write and may already
have some unintentional changes.
>> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>>
>>> 320: } else if ((lenByte & 0x080) == 0x00) { // short form, 1 byte
>>> datum
>>> 321: length = lenByte;
>>> 322: } else { // long form or indefinite
>>
>> This should just the long form. Isn't the indefinite case covered by the
>> first if-check?
>
> Correct.
Fix the comment then?
>> src/java.base/share/classes/sun/security/util/DerValue.java line 226:
>>
>>> 224: this.end = end;
>>> 225: this.allowBER = allowBER;
>>> 226: this.data = new DerInputStream(this);
>>
>> this.data used to just contain the "value" part of the DerValue, now it's
>> the whole DerValue (including tag and length
>> in addition to value). Is its usage still compatible?
>
> The constructor `DerInputStream(DerValue)` has not read tag at all. I can
> rewrite it the last line to `new
> DerInputStream(buffer, start, end - start, allowBER)`.
Yes, I think it is better to just use the buffer/start/end-start/allowBER for
this.data.
-------------
PR: https://git.openjdk.java.net/jdk/pull/232
