Re: [8u] [RFR] Request for Review of JDK-8196952: Bad primeCertainty value setting in DSAParameterGenerator

[Seán Coffey]

Thanks for looking at this one Andrew. I've had it on my to-do list.

The patch looks fine.

Regards,
Sean.

On 22/02/18 04:09, Andrew Hughes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8196952
Webrev: http://cr.openjdk.java.net/~andrew/openjdk8/8196952/webrev.01/
Review thread: N/A

When merging the recent 8u161 update into IcedTea, I came across
a merge issue related to the ordering of changes to DSAParameterGenerator.

It's an issue only relevant to OpenJDK 8 and any further backports that
use the version from that repository.

In OpenJDK 9 and up, the work in 8072452 was done in April 2016,
and 8181048 in July 2017.

However, in OpenJDK 8, 8072452 was not present when 8181048
was backported in July 2017, and was only added in the last CPU.
As a result, the two changes were applied there in the opposite order
and 8072452 ended up reverting parts of 8181048.

The main issue is that 8181048 has this change:

- int primeCertainty = 80; // for 1024-bit prime P
- if (valueL == 2048) {
+ int primeCertainty = -1;
+ if (valueL <= 1024) {
+ primeCertainty = 80;
+ } else if (valueL == 2048) {

But then the backport of 8072452 reverts part of it:

@@ -205,11 +206,13 @@
          int b = (valueL - 1) % outLen;
          byte[] seedBytes = new byte[seedLen/8];
          BigInteger twoSl = TWO.pow(seedLen);
- int primeCertainty = -1;
+ int primeCertainty = 80; // for 1024-bit prime P
          if (valueL <= 1024) {
              primeCertainty = 80;
          } else if (valueL == 2048) {
              primeCertainty = 112;
+ } else if (valueL == 3072) {
+ primeCertainty = 128;
          }

          if (primeCertainty < 0) {

So primeCertainty is never going to be < 0.

The other differences are minor, but help keep the code in
sync with OpenJDK 9 and slightly more efficient; we don't
bother setting valueN in the if block as it will be reset by
the call to getDefDSASubprimeSize anyway, and we
use BigInteger.ONE instead of creating our own version.
Sadly, BigInteger.TWO is not public in OpenJDK 8 so we
still need a local variable for that.

As stated above, the patch is inapplicable for 9 and up,
so needs a review and approval for OpenJDK 8.

Thanks,