Last Comment Bug 483113 - add environment variable to disable/enable hash algorithms in cert/CRL signatures
: add environment variable to disable/enable hash algorithms in cert/CRL signat...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12.2
: All All
: P1 blocker (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on: 471539
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-12 19:17 PDT by Julien Pierre
Modified: 2009-03-31 08:24 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 for NSS Trunk (checked in) (1.80 KB, patch)
2009-03-20 02:41 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Review
supplemental patch suggested by Julien, v1 (Checked in) (681 bytes, patch)
2009-03-20 17:28 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Review
Current set of OID desc strings, sorted alphabetically (9.10 KB, text/plain)
2009-03-20 17:31 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
supplemental patch - tweak OID strings, use semi-colon (5.55 KB, patch)
2009-03-20 18:05 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review
supplemental patch - tweak OID strings, use semi-colon - V2 (5.57 KB, patch)
2009-03-20 18:24 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review-
Details | Diff | Review
supplemental patch - tweak OID strings, use semi-colon - V2 AGAIN (5.57 KB, patch)
2009-03-20 18:29 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review
supplemental patch - tweak OID strings, use semi-colon - V3 (Checked in) (5.92 KB, patch)
2009-03-20 18:59 PDT, Nelson Bolyard (seldom reads bugmail)
nelson: review+
Details | Diff | Review
supplemental patch - fix test scripts (2.53 KB, patch)
2009-03-20 20:12 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Review
supplemental patch - fix test scripts some more (Checked in) (1.14 KB, patch)
2009-03-21 16:03 PDT, Nelson Bolyard (seldom reads bugmail)
slavomir.katuscak+mozilla: review+
Details | Diff | Review

Description Julien Pierre 2009-03-12 19:17:03 PDT
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.
Comment 1 Wan-Teh Chang 2009-03-12 21:44:24 PDT
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".
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-03-12 22:00:14 PDT
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.
Comment 3 Julien Pierre 2009-03-13 19:02:27 PDT
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.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-03-20 02:41:42 PDT
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 5 Julien Pierre 2009-03-20 16:58:26 PDT
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.
Comment 6 Julien Pierre 2009-03-20 16:59:25 PDT
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.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:05:03 PDT
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 8 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:07:28 PDT
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
Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:28:32 PDT
Created attachment 368620 [details] [diff] [review]
supplemental patch suggested by Julien, v1 (Checked in)

I think this is a good idea.  Please review.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:31:18 PDT
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.
Comment 11 Julien Pierre 2009-03-20 17:37:59 PDT
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 ?
Comment 12 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:41:52 PDT
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?
Comment 13 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:43:59 PDT
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 14 Nelson Bolyard (seldom reads bugmail) 2009-03-20 17:46:45 PDT
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.
Comment 15 Julien Pierre 2009-03-20 17:52:41 PDT
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.
Comment 16 Julien Pierre 2009-03-20 17:54:32 PDT
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:)
Comment 17 Nelson Bolyard (seldom reads bugmail) 2009-03-20 18:05:01 PDT
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.
Comment 18 Nelson Bolyard (seldom reads bugmail) 2009-03-20 18:07:38 PDT
But wait!  There's More!  :)

PKCS5 
PKCS#5
PKCS 5
PKCS #5

Same for PKCS12.

I'll write another subsequent patch.
Comment 19 Nelson Bolyard (seldom reads bugmail) 2009-03-20 18:24:42 PDT
Created attachment 368631 [details] [diff] [review]
supplemental patch - tweak OID strings, use semi-colon - V2

This fixes PKCS12 also.
Comment 20 Nelson Bolyard (seldom reads bugmail) 2009-03-20 18:29:59 PDT
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 21 Nelson Bolyard (seldom reads bugmail) 2009-03-20 18:31:05 PDT
Comment on attachment 368632 [details] [diff] [review]
supplemental patch - tweak OID strings, use semi-colon - V2 AGAIN

Nope.  :(
Comment 22 Julien Pierre 2009-03-20 18:34:24 PDT
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.
Comment 23 Nelson Bolyard (seldom reads bugmail) 2009-03-20 18:59:51 PDT
Created attachment 368638 [details] [diff] [review]
supplemental patch - tweak OID strings, use semi-colon - V3 (Checked in)

I think this one is right.
Comment 24 Nelson Bolyard (seldom reads bugmail) 2009-03-20 19:04:25 PDT
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
Comment 25 Nelson Bolyard (seldom reads bugmail) 2009-03-20 20:12:30 PDT
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.
Comment 26 Nelson Bolyard (seldom reads bugmail) 2009-03-20 21:01:44 PDT
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
Comment 27 Nelson Bolyard (seldom reads bugmail) 2009-03-21 15:58:56 PDT
Not out of the woods, yet.  
Another patch is forthcoming.
Comment 28 Nelson Bolyard (seldom reads bugmail) 2009-03-21 16:03:15 PDT
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.
Comment 29 Nelson Bolyard (seldom reads bugmail) 2009-03-21 16:19:52 PDT
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
Comment 30 Nelson Bolyard (seldom reads bugmail) 2009-03-24 08:07:02 PDT
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.