[8u] RFR: 8172404: Tools should warn if weak algorithms are used before restricting them

[Martin Balao]

On 2/3/21 5:15 PM, Severin Gehwolf wrote:
>> In the case of tSADigestAlg and digestalg, my understanding is that you
>> are assuming that they cannot be null because in 8u they are initialized
>> upon object construction. I.e.: String digestalg = "SHA-256". However,
>> I've seen "if (digestalg != null ..." and "if (tSADigestAlg != null ..."
>> statements
> 
> Can you be more specific? If I grep for 'digestalg' in the jdk source
> tree only usages in jarsigner/Main.java come up and related
> Resources.java files. Similar for tSADigestAlg.
> 

Sure. What I've seen is that the use of 'digestalg' and 'tSADigestAlg'
required a non-null check here [1] and here [2] respectively. These
checks made me think that these variables can eventually be null inside
the signJar method. Otherwise, the checks wouldn't have been necessary.

>>  in 8u which makes me think that this is not necessarily true,
>> as if the instance variable can eventually turn null after the object is
>> created. Otherwise, those checks would be redundant. I've seen a couple
>> of places where the value is updated. Have you ruled out this possibility?
> 
> I believe I have. The only place where 'digestalg' is assigned a value
> is on line 437 in jarsigner/Main.java:
> 
>             } else if (collator.compare(flags, "-digestalg") ==0) {
>                 if (++n == args.length) usageNoArg();
>                 digestalg = args[n];
> 
> where the value comes from the arguments string passed in via jarsigner
> CLI args which cannot be null.

Yes, seems right. If no value is set after -digestalg/-tsadigestalg, the
args length check aborts execution. Otherwise, there has to be something
-assuming a call from CLI-.

The argument for tSADigestAlg is
> similar. Line 400 of jarsigner/Main.java reads:
> 
>             } else if (collator.compare(flags, "-tsadigestalg") ==0) {
>                 if (++n == args.length) usageNoArg();
>                 tSADigestAlg = args[n];
> 
> I might have missed some spots, but I think this needs to be viewed in
> the context of being used as a CLI tool: jarsigner.
> 

Yes, I've seen those usages as well and no others.

Yeah, it well be the case that the check in [1] and [2] were not
necessary -or became unnecessary at some point-.

Thanks for checking this.

One more think regarding 'getDefaultSignatureAlgorithm'. I've seen that
there is a similar method in 8u's keytool/Main.java file. In 11u, it
goes to AlgorithmId::getDefaultSigAlgForKey. AlgorithmId is an internal
class (sun.. package). I'd keep a single implementation of this method
in the AlgorithmId class (if backporting the patch that introduces the
method is non-trivial). Up to you.

Looks good to me.

Martin.-