Closed Bug 483113 Opened 15 years ago Closed 15 years ago

add environment variable to disable/enable hash algorithms in cert/CRL signatures

Categories

(NSS :: Libraries, defect, P1)

3.12.2
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: julien.pierre, Assigned: nelson)

References

Details

Attachments

(6 files, 3 obsolete files)

1.80 KB, patch
julien.pierre
: review+
Details | Diff | Splinter Review
681 bytes, patch
julien.pierre
: review+
Details | Diff | Splinter Review
9.10 KB, text/plain
Details
5.92 KB, patch
nelson
: review+
Details | Diff | Splinter Review
2.53 KB, patch
julien.pierre
: review+
Details | Diff | Splinter Review
1.14 KB, patch
slavomir.katuscak+mozilla
: review+
Details | Diff | Splinter Review
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.
Depends on: 471539
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.
OS: Windows Server 2003 → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 3.12.3
Version: unspecified → 3.12.2
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.
Severity: normal → blocker
Summary: add environment variable to disable/enable hash algorithms → add environment variable to disable/enable hash algorithms in cert/CRL signatures
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.
Assignee: nobody → nelson
Status: NEW → ASSIGNED
Attachment #368497 - Flags: review?(julien.pierre.boogz)
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.
Attachment #368497 - Flags: review?(julien.pierre.boogz) → review+
In comment 5,

I meant that NSS_HASH_ALG_SUPPORT was duplicate with NSS_ALLOW_WEAK_SIGNATURE_ALG, and the later is not really needed.
Thanks for the review, Julien.  I'm going to commit the patch as is, 
but I am also going to look at the strstr idea.  It intrigues me. 
I may write a subsequent patch.
Comment on attachment 368497 [details] [diff] [review]
Patch v1 for NSS Trunk (checked in)

Checking in secoid.c; new revision: 1.49; previous revision: 1.48
Attachment #368497 - Attachment description: Patch v1 for NSS Trunk → Patch v1 for NSS Trunk (checked in)
I think this is a good idea.  Please review.
Attachment #368620 - Flags: review?(julien.pierre.boogz)
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.
Attachment #368622 - Attachment description: Currect set of OID desc strings, sorted alphabetically → Current set of OID desc strings, sorted alphabetically
Attachment #368620 - Flags: review?(julien.pierre.boogz) → review+
Nelson,

I hadn't noticed that there were commas in the mechanism names. Perhaps we should use another separator in the environment variable. How about semicolon ?
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?
Here are the strings with commas:

"ANSI X9.62 elliptic curve prime192v1 (aka secp192r1, NIST P-192)"
"ANSI X9.62 elliptic curve prime256v1 (aka secp256r1, NIST P-256)"
"PKIX CRMF Registration Control, Old Certificate ID"
"PKIX CRMF Registration Control, PKI Archive Options"
"PKIX CRMF Registration Control, PKI Publication Info"
"PKIX CRMF Registration Control, Protocol Encryption Key"
"PKIX CRMF Registration Control, Registration Authenticator"
"PKIX CRMF Registration Control, Registration Token"
"PKIX CRMF Registration Info, Certificate Request"
"PKIX CRMF Registration Info, UTF8 Pairs"

Semi-colon would be OK, I think, but I think that with strstr, it's not a 
problem because there exist unique substrings that don't need them.
Comment on attachment 368620 [details] [diff] [review]
supplemental patch suggested by Julien, v1 (Checked in)

Checking in secoid.c; new revision: 1.50; previous revision: 1.49

I'll do some string tweaks next.
Attachment #368620 - Attachment description: supplemental patch suggested by Julien, v1 → supplemental patch suggested by Julien, v1 (Checked in)
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:)
This corrects the OID string inconsistencies I described above, and uses
semi-colon for the separator.
Attachment #368626 - Flags: review?(julien.pierre.boogz)
But wait!  There's More!  :)

PKCS5 
PKCS#5
PKCS 5
PKCS #5

Same for PKCS12.

I'll write another subsequent patch.
This fixes PKCS12 also.
Attachment #368626 - Attachment is obsolete: true
Attachment #368631 - Flags: review?(julien.pierre.boogz)
Attachment #368626 - Flags: review?(julien.pierre.boogz)
bonsai's interdiff is lame beyond words.  
Maybe it will be able to diff THIS copy of V2 against v1
Attachment #368632 - Attachment is patch: true
Comment on attachment 368632 [details] [diff] [review]
supplemental patch - tweak OID strings, use semi-colon - V2 AGAIN

Nope.  :(
Attachment #368632 - Attachment is obsolete: true
Attachment #368631 - Flags: review?(julien.pierre.boogz) → review-
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.
I think this one is right.
Attachment #368631 - Attachment is obsolete: true
Comment on attachment 368638 [details] [diff] [review]
supplemental patch - tweak OID strings, use semi-colon - V3 (Checked in)

Marking r+ per Julien's comment 22.

Checking in secoid.c; new revision: 1.51; previous revision: 1.50
Attachment #368638 - Attachment description: supplemental patch - tweak OID strings, use semi-colon - V3 → supplemental patch - tweak OID strings, use semi-colon - V3 (Checked in)
Attachment #368638 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Attachment #368644 - Flags: review?(julien.pierre.boogz)
Attachment #368644 - Flags: review?(julien.pierre.boogz) → review+
Comment on attachment 368644 [details] [diff] [review]
supplemental patch - fix test scripts

Checking in tools/tools.sh; new revision: 1.18; previous revision: 1.17

This should turn the tinderbox green again, but I'm watching it
Not out of the woods, yet.  
Another patch is forthcoming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #368734 - Flags: review?(slavomir.katuscak)
Attachment #368734 - Flags: review?(slavomir.katuscak) → review+
Comment on attachment 368734 [details] [diff] [review]
supplemental patch - fix test scripts some more (Checked in)

Checking in tools.sh; new revision: 1.20; previous revision: 1.19
Attachment #368734 - Attachment description: supplemental patch - fix test scripts some more → supplemental patch - fix test scripts some more (Checked in)
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.