RFR: 8259535: ECDSA SignatureValue do not always have the specified length

[Weijun Wang-2]

The code change fixes the ECDSA XML signature length issue. It should only happened when there is no P1363 ECDSA support, which is not true when SunEC is used.

Technically, if a PrivateKey is not of ECPrivateKey the bug will still show up, and in this case we can actually look into the OID/parameter of the ASN.1 encoding and do further evaluation, but I think this is not worth doing. Please advise me if you think differently.

-------------

Commit messages:
 - 8259535: ECDSA SignatureValue do not always have the specified length

Changes: https://git.openjdk.java.net/jdk/pull/2550/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2550&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259535
  Stats: 227 lines in 4 files changed: 216 ins; 2 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2550.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2550/head:pull/2550

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

[Sean Mullan-2]

On Fri, 12 Feb 2021 15:24:07 GMT, Weijun Wang <[hidden email]> wrote:

> The code change fixes the ECDSA XML signature length issue. It should only happen when there is no P1363 ECDSA support, which is not true when SunEC is used.
>
> If a PrivateKey is not of ECPrivateKey type then the bug will still show up. Technically, we can drill into the OID/parameter of the ASN.1 encoding and do further evaluation, but I think this is not worth doing. Please advise me if you think differently.

Marked as reviewed by mullan (Reviewer).

src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java line 68:

> 66:      *
> 67:      * @param asn1Bytes
> 68:      * @param rawLen

You should add the same javadoc for these parameters as you did in ECDSAUtils.java.

-------------

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

[Weijun Wang-2]

On Thu, 25 Feb 2021 21:23:51 GMT, Sean Mullan <[hidden email]> wrote:

>> The code change fixes the ECDSA XML signature length issue. It should only happen when there is no P1363 ECDSA support, which is not true when SunEC is used.
>>
>> If a PrivateKey is not of ECPrivateKey type then the bug will still show up. Technically, we can drill into the OID/parameter of the ASN.1 encoding and do further evaluation, but I think this is not worth doing. Please advise me if you think differently.
>
> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java line 68:
>
>> 66:      *
>> 67:      * @param asn1Bytes
>> 68:      * @param rawLen
>
> You should add the same javadoc for these parameters as you did in ECDSAUtils.java.

Well, it looks like a "style" to only list the param names without any explanation in this file and nearby files. Since the body of this method is only one line I assume people curious about the meaning of the parameters can just navigate to `ECDSAUtils.convertASN1toXMLDSIG` to read the description there.

Can we just keep it "clean"?

-------------

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

[Sean Mullan-2]

On Fri, 26 Feb 2021 15:34:42 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java line 68:
>>
>>> 66:      *
>>> 67:      * @param asn1Bytes
>>> 68:      * @param rawLen
>>
>> You should add the same javadoc for these parameters as you did in ECDSAUtils.java.
>
> Well, it looks like a "style" to only list the param names without any explanation in this file and nearby files. Since the body of this method is only one line I assume people curious about the meaning of the parameters can just navigate to `ECDSAUtils.convertASN1toXMLDSIG` to read the description there.
>
> Can we just keep it "clean"?

Ok, sounds reasonable.

-------------

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

[Weijun Wang-2]

On Fri, 12 Feb 2021 15:24:07 GMT, Weijun Wang <[hidden email]> wrote:

> The code change fixes the ECDSA XML signature length issue. It should only happen when there is no P1363 ECDSA support, which is not true when SunEC is used.
>
> If a PrivateKey is not of ECPrivateKey type then the bug will still show up. Technically, we can drill into the OID/parameter of the ASN.1 encoding and do further evaluation, but I think this is not worth doing. Please advise me if you think differently.

This pull request has now been integrated.

Changeset: a4c24961
Author:    Weijun Wang <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/a4c24961
Stats:     227 lines in 4 files changed: 216 ins; 2 del; 9 mod

8259535: ECDSA SignatureValue do not always have the specified length

Reviewed-by: mullan

-------------

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