[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,