This is a followup to bug 471539 .
The environment variable could be defined as follows :
name : NSS_HASH_ALG_SUPPORT
value : comma-separated list of [+/-]ALGNAME
This environment variable would not need to define every algorithm, it would
only make changes from the default.
Eg.
NSS_HASH_ALG_SUPPORT=+MD2,-MD5 would enable MD2, which we decided to disable by
default, and disable MD5, which we decided to leave on by default.

This environment variable will potentially increase the
support burden of all NSS-based apps. It adds a variable
that may alter the behavior of NSS, out of the control
of the application developers. If such environment
variable start to proliferate, imagine a day when we
need to ask for a dump of the user's environment in
bug reports. So I think this bug should be marked
"won't fix".

On the contrary, it has the potential to reduce the support requirement on
NSS apps. Many NSS apps have chosen not to provide any UI for user
configuration of capabilities. The proposal obviates such UI.

I didn't make it clear in this bug, but I was only proposing to have this new env. variable effect the processing of cert/CRL signatures, just like in bug 471539 . That's why I had made the comment in that bug originally.
If we have this more fine-grain environment variable syntax, then the need for the variable NSS_ALLOW_WEAK_SIGNATURE_ALG which was just implemented goes away, so we might as well .
We can implement more generic disabling of algorithms separately. I think that's the subject of bug 482882. If we have more generic disabling of MD2, the incremental work to completely disable other algorithms is probably not that much.

Created attachment 368497[details][diff][review]
Patch v1 for NSS Trunk (checked in)
I believe This patch implements Julien's proposal. It uses the algorithm
name strings that are already in secoid.c's table of OIDs.
Julien, please review and test to your satisfaction.

Comment on attachment 368497[details][diff][review]
Patch v1 for NSS Trunk (checked in)
Nelson,
This patch looks fine, and you have my approval to check it in as-is.
I still have the following comments about possible improvements, in case you want to make them.
1. You are doing an exact comparison of the algorithm with strcmp against oids[i].desc .
I would prefer if you would allow a partial match, eg. with strstr. That way you could disable, say, all algorithms that include MD5, with just "-MD5", as opposed to having to set the environment variable to "-MD5,-PKCS #1 MD5 With RSA Encryption,-PKCS #5 Password Based Encryption with MD5 and DES CBC" to disable all algorithms that include MD5. I didn't clearly explain that was my intent, though. Either method works, and your exact method match is more fine-grained, though I don't know if anyone will really want to disable MD5 without also disabling the other algorithms that include MD5 .
If you prefer this suggestion, feel free to change to strstr.
2. If you implement 1 above, then I think it obviates the need for the NSS_HASH_ALG_SUPPORT environment variable , since those variables would be doing partially duplicate jobs, and potentially set with conflicting values. You could delete the block of code in SECOID_Init that processes it.
3. There could be a bit more of error handling in handleHashAlgSupport about the syntax.
Eg, if there is no - or +, or no algorithm name match. I would suggest changing the function to return SECStatus . In debug builds, a message could be printed on the stderr to notify the user of the problem.

Created attachment 368622[details]
Current set of OID desc strings, sorted alphabetically
Here are all the name strings presently in the OID table.
Some of them contain commas, and so would be impossible to enter without
this strstr approach (or adding escaping). I just need to convince myself
that we don't have problems with common substrings.

Hmm. Maybe we ought to tweak some of the strings in the OID table to
be more consistent about some things. For example, SHA1 is represented
in the table by these strings:
Sha1
SHA1
SHA-1
DES with CBC is represented by all these:
DES CBC
DES-CBC
DES-cbc
Triple DES is both 3DES and "Triple DES".
An observation that I want to make, that I think is NOT a problem, is
that some small algorithm name strings appear in many other strings,
making it impossible to disable the short name without also disabling all
the longer names. e.g. MD5 appears as
"MD5"
"PKCS #1 MD5 With RSA Encryption"
"PKCS #5 Password Based Encryption with MD5 and DES CBC"
But I don't think this is a problem, because I think that anyone who wants
to disable the short form (the generic algorithm) will also want to disable
the compound algorithms that use it.
Agreed?

Nelson,
Re: Sha1 vs SHA1, if there was a stristr, we should use it :) Though it can be done fairly easily by changing the case of both arguments to strstr before searching, but that requires a copy as they are const.
For SHA1 vs SHA-1, or Triple DES vs 3DES, it is more difficult though. We could think about changing the OID table, but I think for now we are OK.
Re: your last observation, I was suggesting in comment 5 precisely that we wanted to use strstr in order to be able to turn off all 3 mechanisms that used MD5 at once by just specifying "-MD5". So, it's not a problem, it's actually the desired behavior for this case, IMO.
If we think there are other possible use cases where we only want to disable the algorithm with the substring, and not others that include it, then perhaps we would need an extended syntax to do specifically strcmp instead of strstr. Maybe -- or ++ for strict comparison. But I am not sure that's necessary.

Re: comment 13,
If we want to make this idiot-proof enough, we should try to avoid the comma problem. Users may try to paste the whole algorithm name and it will mess up the rest of their environment variable. We should either fix the table or the separator in the env. variable code.
These mechs with commas don't look like hashing algorithms though, so maybe it's a non-issue:)

Created attachment 368626[details][diff][review]
supplemental patch - tweak OID strings, use semi-colon
This corrects the OID string inconsistencies I described above, and uses
semi-colon for the separator.

Created attachment 368632[details][diff][review]
supplemental patch - tweak OID strings, use semi-colon - V2 AGAIN
bonsai's interdiff is lame beyond words.
Maybe it will be able to diff THIS copy of V2 against v1

Comment on attachment 368631[details][diff][review]
supplemental patch - tweak OID strings, use semi-colon - V2
r+ for the OID table change
r- for the code change in handleHashAlgSupport . You forgot to change the test of
while (*nextArg == ',')
to
while (*nextArg == ';')
r+ with that fix.
I would have provided an mxr link but your checkin did not propagate yet.

Created attachment 368644[details][diff][review]
supplemental patch - fix test scripts
Evidently on windows, all.sh does not run tools.sh :(
When I tried to run tools.sh on Windows, it complained of syntax errors
in the script. This patch fixes those syntax errors, and also fixes
the script's copies of the OID name strings. But it does not address
the problem that tools.sh isn't run by all.sh on Windows.

Created attachment 368734[details][diff][review]
supplemental patch - fix test scripts some more (Checked in)
When I changed the name of SHA1 to SHA-1 to be self-consistent throughout
the OIDs table, I missed a spot in the test scripts where that string was
embedded.
This patch fixes some more places where that problem appears. I'm not sure
that this will be the last of these patches.
Slavo, please review.

I think more work needs to be done to make NSS consistent with respect to
the presence or absence of dashes in SHA[-]NNN (e.g. SHA-256 or SHA256)
and also in xxx-CBC (e.g. AES-CBC or AES CBC). But that should be done
in a separate bug.

Note

You need to
log in
before you can comment on or make changes to this bug.