RFR: 8259709: Disable SHA-1 XML Signatures

[Sean Mullan-2]

Please review this change to disable XML signatures that use SHA-1 based digest or signature algorithms. SHA-1 is weak and is not a recommended algorithm for digital signatures. This will improve out of the box security by restricting XML signatures that use SHA-1 algorithms.

CSR: https://bugs.openjdk.java.net/browse/JDK-8261246
Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364

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

Commit messages:
 - Remove extra whitespace.
 - Merge
 - Initial revision.

Changes: https://git.openjdk.java.net/jdk/pull/2463/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2463&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259709
  Stats: 55 lines in 5 files changed: 50 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2463.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2463/head:pull/2463

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

[Rajan Halade-2]

On Mon, 8 Feb 2021 20:46:41 GMT, Sean Mullan <[hidden email]> wrote:

> Please review this change to disable XML signatures that use SHA-1 based digest or signature algorithms. SHA-1 is weak and is not a recommended algorithm for digital signatures. This will improve out of the box security by restricting XML signatures that use SHA-1 algorithms.
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8261246
> Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364

Marked as reviewed by rhalade (Reviewer).

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

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

[Weijun Wang-2]

On Mon, 8 Feb 2021 20:46:41 GMT, Sean Mullan <[hidden email]> wrote:

> Please review this change to disable XML signatures that use SHA-1 based digest or signature algorithms. SHA-1 is weak and is not a recommended algorithm for digital signatures. This will improve out of the box security by restricting XML signatures that use SHA-1 algorithms.
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8261246
> Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364

Change looks good.

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

Marked as reviewed by weijun (Reviewer).

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

[Weijun Wang-2]

On Tue, 9 Feb 2021 21:04:00 GMT, Weijun Wang <[hidden email]> wrote:

>> Please review this change to disable XML signatures that use SHA-1 based digest or signature algorithms. SHA-1 is weak and is not a recommended algorithm for digital signatures. This will improve out of the box security by restricting XML signatures that use SHA-1 algorithms.
>>
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8261246
>> Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364
>
> Change looks good.

All test changes are about re-enable disabled algorithms. Do we have a test on ensuring disabled algorithms are indeed disabled?

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

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

[Weijun Wang-2]

On Mon, 8 Feb 2021 20:46:41 GMT, Sean Mullan <[hidden email]> wrote:

> Please review this change to disable XML signatures that use SHA-1 based digest or signature algorithms. SHA-1 is weak and is not a recommended algorithm for digital signatures. This will improve out of the box security by restricting XML signatures that use SHA-1 algorithms.
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8261246
> Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364

test/lib/jdk/test/lib/security/SecurityUtils.java line 78:

> 76:      * part of the algorithm URI.
> 77:      */
> 78:     public static void removeAlgsFromDSigPolicy(List<String> algs) {

How about using `String... algs` as arguments?

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

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

[Sean Mullan-2]

On Mon, 22 Feb 2021 03:42:23 GMT, Weijun Wang <[hidden email]> wrote:

>> Please review this change to disable XML signatures that use SHA-1 based digest or signature algorithms. SHA-1 is weak and is not a recommended algorithm for digital signatures. This will improve out of the box security by restricting XML signatures that use SHA-1 algorithms.
>>
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8261246
>> Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364
>
> test/lib/jdk/test/lib/security/SecurityUtils.java line 78:
>
>> 76:      * part of the algorithm URI.
>> 77:      */
>> 78:     public static void removeAlgsFromDSigPolicy(List<String> algs) {
>
> How about using `String... algs` as arguments?

Yes, that is nicer.

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

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

[Sean Mullan-2]

On Fri, 19 Feb 2021 22:36:24 GMT, Weijun Wang <[hidden email]> wrote:

> All test changes are about re-enabling disabled algorithms. Do we have a test on ensuring disabled algorithms are indeed disabled? How about we set "org.jcp.xml.dsig.secureValidation" to false everywhere in the existing tests and add a new dedicated test to check for disabled algorithms/key sizes etc.

That is what test/jdk/javax/xml/crypto/dsig/SecureValidationPolicy.java does, see this code block on lines 65-69:

        for (String alg : restrictedAlgs) {
            if (!Policy.restrictAlg(alg)) {
                throw new Exception(alg + " alg not restricted");
            }
        }

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

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

[Weijun Wang-2]

On Wed, 24 Feb 2021 22:02:45 GMT, Sean Mullan <[hidden email]> wrote:

> > All test changes are about re-enabling disabled algorithms. Do we have a test on ensuring disabled algorithms are indeed disabled? How about we set "org.jcp.xml.dsig.secureValidation" to false everywhere in the existing tests and add a new dedicated test to check for disabled algorithms/key sizes etc.
>
> That is what test/jdk/javax/xml/crypto/dsig/SecureValidationPolicy.java does, see this code block on lines 65-69:
>
> ```
>         for (String alg : restrictedAlgs) {
>             if (!Policy.restrictAlg(alg)) {
>                 throw new Exception(alg + " alg not restricted");
>             }
>         }
> ```

This is only about checking the parsing function of the Policy class. I would be more confident if an actual validation call is made.

I have a test on PSS at https://github.com/openjdk/jdk/blob/a79df58e0ad0b19aa8e0611cc55f5628383c2950/test/jdk/javax/xml/crypto/dsig/SecureValidation.java. Maybe I can enhance it to contain more algorithms.

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

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