Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v3]

2020-09-25 Thread Weijun Wang 
On Fri, 25 Sep 2020 04:21:04 GMT, Hai-May Chao  wrote:

>> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
>> parameters field instead of encoding a
>> Null tag.
>
> Hai-May Chao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comment for RFC

Marked as reviewed by weijun (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]

2020-09-24 Thread Hai-May Chao 
On Fri, 25 Sep 2020 00:45:09 GMT, Weijun Wang  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated test case to use DerUtils
>
> src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 214:
> 
>> 212: || algid.equals(SHA512withECDSA_oid)) {
>> 213: // RFC 4055 3.3: when an RSASSA-PSS key does not require
>> 214: // parameter validation, field is absent.
> 
> Would you like to add more comments on other RFCs here?

Added the comments for RFC reference.

-

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v3]

2020-09-24 Thread Hai-May Chao 
> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
> parameters field instead of encoding a
> Null tag.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Added comment for RFC

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/312/files
  - new: https://git.openjdk.java.net/jdk/pull/312/files/edc71d03..4f4e3688

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=312=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=312=01-02

  Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]

2020-09-24 Thread Weijun Wang 
On Thu, 24 Sep 2020 23:49:08 GMT, Hai-May Chao  wrote:

>> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
>> parameters field instead of encoding a
>> Null tag.
>
> Hai-May Chao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated test case to use DerUtils

src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 214:

> 212: || algid.equals(SHA512withECDSA_oid)) {
> 213: // RFC 4055 3.3: when an RSASSA-PSS key does not require
> 214: // parameter validation, field is absent.

Would you like to add more comments on other RFCs here?

-

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v2]

2020-09-24 Thread Hai-May Chao 
> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
> parameters field instead of encoding a
> Null tag.

Hai-May Chao has updated the pull request incrementally with one additional 
commit since the last revision:

  Updated test case to use DerUtils

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/312/files
  - new: https://git.openjdk.java.net/jdk/pull/312/files/f52268a7..edc71d03

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=312=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=312=00-01

  Stats: 189 lines in 1 file changed: 35 ins; 132 del; 22 mod
  Patch: https://git.openjdk.java.net/jdk/pull/312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier

2020-09-23 Thread Weijun Wang 
On Wed, 23 Sep 2020 06:07:09 GMT, Hai-May Chao  wrote:

>> I don't quite understand what the test is for. The bug is about encoding but 
>> the test seems to be decoding the
>> certificates. Does the test fail before this fix and succeed after it?
>
> This is because the encoding of Algorithm Identifier incorrectly adds two 
> NULL tags to the parameters field in the
> canned certificate. (There are two Algorithm Identifiers in the cert, with 
> each NULL tag containing two bytes:  tag +
> length.) I use the length of an encoded certificate 
> (x509Cert.getEncoded().length) to verify that the certificate
> contains an extra 4 bytes to hold the two NULL tags. Therefore, the 
> certificate without the fix should be 4 bytes (5
> bytes if one byte alignment is applied) longer in length than the certificate 
> with the fix.

But the certificates included in the test are static and if one day we 
mistakenly add the NULL back it will not be
detected by the test. My idea is to generate a cert on the fly inside the test 
so that can check the `derEncode` method
in *this* JDK. As for checking if the NULL is there, you can try looking at the 
`DerUtils::shouldNotExist` method with
"021" and "11" as arguments.

-

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier

2020-09-23 Thread Hai-May Chao 
On Wed, 23 Sep 2020 02:49:29 GMT, Weijun Wang  wrote:

>> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
>> parameters field instead of encoding a
>> Null tag.
>
> I don't quite understand what the test is for. The bug is about encoding but 
> the test seems to be decoding the
> certificates. Does the test fail before this fix and succeed after it?

This is because the encoding of Algorithm Identifier incorrectly adds two NULL 
tags to the parameters field in the
canned certificate. (There are two Algorithm Identifiers in the cert, with each 
NULL tag containing two bytes:  tag +
length.) I use the length of an encoded certificate 
(x509Cert.getEncoded().length) to verify that the certificate
contains an extra 4 bytes to hold the two NULL tags. Therefore, the certificate 
without the fix should be 4 bytes (5
bytes if one byte alignment is applied) longer in length than the certificate 
with the fix.

-

PR: https://git.openjdk.java.net/jdk/pull/312

Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier

2020-09-22 Thread Weijun Wang 
On Tue, 22 Sep 2020 22:21:20 GMT, Hai-May Chao  wrote:

> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
> parameters field instead of encoding a
> Null tag.

I don't quite understand what the test is for. The bug is about encoding but 
the test seems to be decoding the
certificates. Does the test fail before this fix and succeed after it?

-

PR: https://git.openjdk.java.net/jdk/pull/312

RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier

2020-09-22 Thread Hai-May Chao 
This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the 
parameters field instead of encoding a
Null tag.

-

Commit messages:
 - 8252377: Incorrect encoding for EC AlgorithmIdentifier

Changes: https://git.openjdk.java.net/jdk/pull/312/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=312=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252377
  Stats: 217 lines in 2 files changed: 216 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/312/head:pull/312

PR: https://git.openjdk.java.net/jdk/pull/312