Closed Bug 1009429 Opened 6 years ago Closed 4 years ago

enhancement: Make the algorithm selection in NSS more flexible

Categories

(NSS :: Libraries, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nmav, Assigned: rrelyea)

References

Details

Attachments

(8 files, 16 obsolete files)

90.00 KB, application/x-tar
Details
6.31 KB, patch
rrelyea
: review+
elio.maldonado.batiz
: review+
Details | Diff | Splinter Review
9.31 KB, patch
elio.maldonado.batiz
: review+
Details | Diff | Splinter Review
6.38 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
9.48 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
8.62 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
101.33 KB, patch
elio.maldonado.batiz
: review+
Details | Diff | Splinter Review
1.20 KB, patch
ekr
: review+
Details | Diff | Splinter Review
The attached set of patches allow restricting the algorithms, protocols and methods used on TLS sessions. For example with this patches one can disable SHA1 signatures for certificate verification, or a particular curve being used for TLS key exchange or certificate verification. In addition minimum limits for DH, RSA or DSA parameters can be set (e.g, only allow RSA keys over 1024 bits), and the final two patches allow the global configuration of these options using pkcs11.txt.

They are submitted in one go, as they are dependent.

[PATCH 1/7] Restricts the SignatureAlgorithms in SSL based on the NSS settings.
[PATCH 2/7] The NSS_G/SetAlgorithmPolicy functions can be used to restrict the curves used in SSL.
[PATCH 3/7] Added functions to set and get numeric options for NSS.
[PATCH 4/7] The NSS_G/SetAlgorithmPolicy functions can be used to restrict the curves used in cert verification
[PATCH 5/7] Check for acceptable certificate parameters when verifying signatures.
[PATCH 6/7] Added config parameter.
[PATCH 7/7] Apply the NSS policies read by the config parameter.
Attachment #8421575 - Flags: superreview?(rrelyea)
Attachment #8421575 - Flags: review?(wtc)
Comment on attachment 8421575 [details]
This file contains the series of patches that provide the above functionality

r+ with some minor tweaks:

1) Add PORT_Assert()s after the dynamic table building in ssl3_ClientSendSigAlgsXtn() [ssl3ext.c] and ssl_SendSupportedCurvesXtn() [ssl3ecc.c] to make sure we don't overrun the stack array (we currently can't in the existing code, but the asserts will trigger is we add more SigAlgs or Curves in the future.

2) Update NSS_USE_ALG_RESERVED as we add more defined flags.

I'll attach a patch with both of these comments fullfilled and the patch updated to apply to the latest hg tree.
Attachment #8421575 - Flags: superreview?(rrelyea) → superreview+
Attached patch nss-patches.tar (obsolete) — Splinter Review
Kai, I'll check in the path as soon as I finish a text build
Assignee: nobody → rrelyea
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8487627 - Flags: review+
Keywords: checkin-needed
Attached patch The real set of update patches. (obsolete) — Splinter Review
oops, I attached the wrong set, here is the correct set.
Attachment #8487627 - Attachment is obsolete: true
Attachment #8488037 - Flags: review+
Comment on attachment 8488037 [details] [diff] [review]
The real set of update patches.

Review of attachment 8488037 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/nss/manifest.mn
@@ +15,5 @@
>  MODULE = nss
>  
>  CSRCS = \
>  	nssinit.c \
> +	nssoptions.c \

nssoptions.c is missing

::: lib/nss/nss.def
@@ +1061,5 @@
>  PK11_PrivDecrypt;
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSS_3.16.2 { 	# NSS 3.16.2 release

Shouldn't this be 
+NSS_3.17.2 { # NSS 3.17.2 release
or whatever is the intended release target?

Similar comment for nss-util.dif

::: lib/util/nssutil.def
@@ +275,5 @@
> +;+    global:
> +NSSUTIL_ArgParseModuleSpecEx;
> +;+    local:
> +;+       *;
> +;+};

Shouldn't this be
+NSS_UTIL_3.17.2 { # NSS Utilities 3.17.2 release
or whatever is the intended release target?
Hello,
 I received an e-mail to attach a missing file. I've committed it in the github repository:
https://raw.githubusercontent.com/nmav/nss/3d520c1670b74ce785b76210372113f1c6625175/lib/nss/nssoptions.c
https://github.com/nmav/nss/commits/master
Added the missing file fron Nikos' github repo to Bob's combined and updated patch. As mentioned in earlier comments I changed the release in nss.def and nssutil.def to 3.17.2, otherwise it wouldn't compile as 3.16 it was duplicate. It builds now. Not ready yet for full review as I still have to run the tests via all.sh.
Attachment #8421575 - Attachment is obsolete: true
Attachment #8488037 - Attachment is obsolete: true
Attachment #8421575 - Flags: review?(wtc)
Keywords: checkin-needed
Updated to apply with latest sources, changed the target to 3.18 and did minor white space cleanup. The patch is not still ready as several ssl and ocsp tests are failing.
Attachment #8491578 - Attachment is obsolete: true
Just keeping the patch up to date with latest. Unfortunately, I haven't had the time to investigate the cause of test failures. Some of which are:

ssl.sh: #1727: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #1732: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #1736: OCSP stapling, signed response, good status produced a returncode of 2, expected is 0 - FAILED
ssl.sh: #1737: OCSP stapling, signed response, revoked status produced a returncode of 2, expected is 3 - FAILED
ssl.sh: #2153: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #2158: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #2162: OCSP stapling, signed response, good status produced a returncode of 2, expected is 0 - FAILED
ssl.sh: #2163: OCSP stapling, signed response, revoked status produced a returncode of 2, expected is 3 - FAILED
ssl.sh: #2566: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #2571: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #2940: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #2945: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #6507: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #6512: TLS Server hello response with SNI produced a returncode of 1, expected is 0 - FAILED
ssl.sh: #6516: OCSP stapling, signed response, good status produced a returncode of 2, expected is 0 - FAILED
ssl.sh: #6517: OCSP stapling, signed response, revoked status produced a returncode of 2, expected is 3 - FAILED
...
Attachment #8564198 - Attachment is obsolete: true
Attachment #8580854 - Attachment is obsolete: true
Attachment #8654839 - Attachment is obsolete: true
Thank you Nikos for the updated patch set. I tested this set and to do so I also applied  https://hg.mozilla.org/projects/nss/rev/dc7bb2f8cc50 and all test passed. That other patch is of course out of the scope of this bug. Perhaps it deserves its own bug which would be on this one. Bob would know how best to proceed as he carries forward with the review.
Attachment #8587768 - Flags: review?(rrelyea)
s/would be on this one/would be a blocker of this one/
Attachment #8655388 - Flags: review?(rrelyea)
Attachment #8587768 - Attachment is obsolete: true
Attachment #8587768 - Flags: review?(rrelyea)
nikos 0001-Restrict-the-SignatureAlgorithms-in-SSL-based-on-the.patch applied to hg trunk.
Comment on attachment 8656697 [details] [diff] [review]
0001-Restrict-the-SignatureAlgorithms-in-SSL-based-on-the.patch

Review of attachment 8656697 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +12800,1 @@
>  }

This is better handled with a table rather than a bunch of explicit calls.

Since it's isolated into it's own function, though I'll let it go for now.
Attachment #8656697 - Flags: review+
Attachment #8663173 - Flags: review?(emaldona)
Comment on attachment 8663189 [details] [diff] [review]
0002-The-NSS_G-SetAlgorithmPolicy-functions-can-be-used-t.patch

Review of attachment 8663189 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea
Attachment #8663189 - Flags: review+
Comment on attachment 8663213 [details] [diff] [review]
0003-Added-functions-to-set-and-get-numeric-options-for-N.patch

Review of attachment 8663213 [details] [diff] [review]:
-----------------------------------------------------------------

r- this patch is missing nssoptions.c

::: lib/nss/nss.def
@@ +1087,5 @@
> +NSS_OptionGet;
> +NSS_OptionSet;
> +;+    local:
> +;+       *;
> +;+};

I updated this one to 3.21 since that's the next release off the trunk of NSS.
Attachment #8663213 - Flags: review-
Attachment #8663213 - Attachment is obsolete: true
Comment on attachment 8663215 [details] [diff] [review]
0003-Added-functions-to-set-and-get-numeric-options-for-N.patch

Review of attachment 8663215 [details] [diff] [review]:
-----------------------------------------------------------------

This policy can't be enforced in softoken because policy resided in libnss3 which is above softoken.

::: lib/nss/nss.def
@@ +1082,5 @@
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSS_3.21 { 	# NSS 3.21 release
> +;+    global:

I updated this number to the current NSS release.

::: lib/nss/nss.h
@@ +297,5 @@
> +/* Available options for NSS_OptionSet() and NSS_OptionGet().
> + */
> +#define NSS_RSA_MIN_KEY_SIZE (1<<0)
> +#define NSS_DH_MIN_KEY_SIZE  (1<<1)
> +#define NSS_DSA_MIN_KEY_SIZE (1<<2)

These are defined as a bitmask, but we can only fetch one at a time from NSS_OptionSet/NSS_OptionGet. It might make more sense for these to be an enum?

::: lib/nss/nssoptions.c
@@ +23,5 @@
> +static struct nssOps nss_ops = {
> +    512,
> +    512,
> +    512
> +};

We have some defines for these values, we should use them here.

@@ +45,5 @@
> +	rv = SECFailure;
> +    }
> +
> +    return rv;
> +}

We should also add MAX sizes as well. The API is general enough to add this in the future.

NOTE: this patch only activates NSS_DH_MIN_KEY_SIZE.

::: lib/ssl/ssl3con.c
@@ +6955,5 @@
>      	if (rv != SECSuccess) {
>  	    goto loser;		/* malformed. */
>  	}
>          dh_p_bits = SECKEY_BigIntegerBitLength(&dh_p);
> +        if (dh_p_bits < minDH) {

Here's one of the defines we should be using (SSL_DH_MIN_P_BITS).
Attachment #8663215 - Flags: review+
Comment on attachment 8663215 [details] [diff] [review]
0003-Added-functions-to-set-and-get-numeric-options-for-N.patch

Review of attachment 8663215 [details] [diff] [review]:
-----------------------------------------------------------------

SSL_DH_MIN_P_BITS is 1023 bits and defined in sslimpl.h. We'll have to move the define and use it in nssoptions.c. Right now nssoptions set minbits to 512 is is too small. We want to use the define because some implementations of NSS back the define down to 768 and some keep it at 1023. We want to have the default not change with this code unless the user has configured a different number explicitly.

so it's r-
Attachment #8663215 - Flags: review+ → review-
Committed https://hg.mozilla.org/projects/nss/rev/a96ed73556a4 to back out patch 1 until we can investigate the cause of and fix the failures at https://circleci.com/gh/nss-dev/nss/99of
This code appears to have compile errors:

gcc -arch x86_64 -o Darwin14.5.0_64_DBG.OBJ/ssl3con.o -c -g -fPIC  -Wall -fno-common -pipe -DDARWIN -DHAVE_STRERROR -DHAVE_BSD_FLOCK  -Werror -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_ekr -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_ENABLE_TLS_1_3 -DNSS_ENABLE_ZLIB -I../../../dist/Darwin14.5.0_64_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  ssl3con.c
ssl3con.c:9223:47: error: implicit conversion from enumeration type 'const SSLHashType' to
      different enumeration type 'SECOidTag' [-Werror,-Wenum-conversion]
       if (NSS_GetAlgorithmPolicy(serverPref->hashAlg, &policy) == SECSuccess &&
           ~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~^~~~~~~
1 error generated.

Since SECOidTag and SSLHashType appear to be totally different namespaces, this seems like
it's going to create problems. I also note that this code doesn't seem to have any new
tests, which seem like it should be necessary for a change this significant.
I'm going to take a stronger position than ekr here and say that this shouldn't be landed without tests.
Martin, the things I'd like feedback on here is the moving the the SSL Min key sizes so I can access them in the NSS options, but if you want to review more I'm open to feedback.

bob
Attachment #8663215 - Attachment is obsolete: true
Attachment #8667646 - Flags: review?(emaldona)
Attachment #8667646 - Flags: feedback?(martin.thomson)
Comment on attachment 8667646 [details] [diff] [review]
0003-Added-functions-to-set-and-get-numeric-options-for-N-v2.patch

Review of attachment 8667646 [details] [diff] [review]:
-----------------------------------------------------------------

I can live with that.

::: lib/nss/nssoptions.c
@@ +29,5 @@
> +
> +SECStatus
> +NSS_OptionSet(PRInt32 which, PRInt32 value)
> +{
> +SECStatus rv = SECSuccess;

Fix indent.

::: lib/nss/nssoptions.h
@@ +6,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/*
> + *  Include the default limits here
> + */
> +/* SSL default limits are here so we don't have to import a private SSL header 

Trailing space.
Attachment #8667646 - Flags: feedback?(martin.thomson) → feedback+
Comment on attachment 8667646 [details] [diff] [review]
0003-Added-functions-to-set-and-get-numeric-options-for-N-v2.patch

Review of attachment 8667646 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/nss/nssoptions.c
@@ +51,5 @@
> +
> +SECStatus
> +NSS_OptionGet(PRInt32 which, PRInt32 *value)
> +{
> +SECStatus rv = SECSuccess;

Fix indent.

@@ +53,5 @@
> +NSS_OptionGet(PRInt32 which, PRInt32 *value)
> +{
> +SECStatus rv = SECSuccess;
> +
> +    switch (which) {

Nit: If I remember correctly, aligning the case's with the switch statement is the preferred way.
Attachment #8667646 - Flags: review?(emaldona) → review+
Comment on attachment 8663173 [details] [diff] [review]
0001-Restrict-the-SignatureAlgorithms-in-SSL-based-on-the.patch as checked int

Review of attachment 8663173 [details] [diff] [review]:
-----------------------------------------------------------------

conditional r+ but solving the gtest breakage remains. I agree with Martin's request to add test cases.

::: lib/ssl/ssl3con.c
@@ +12730,5 @@
> +    /* Only send the algorithms that are allowed by policy */
> +    for (i=0; i < defaultSize; i++) {
> +	SECOidTag hashOid = ssl3_TLSHashAlgorithmToOID(
> +				defaultSignatureAlgorithms[i].hashAlg);
> +	/* This sets the default signature algorithms. 

Nit: trailing white space.

@@ +12731,5 @@
> +    for (i=0; i < defaultSize; i++) {
> +	SECOidTag hashOid = ssl3_TLSHashAlgorithmToOID(
> +				defaultSignatureAlgorithms[i].hashAlg);
> +	/* This sets the default signature algorithms. 
> +	 * If NSS_GetAlgorithmPolicy fails, add the algorithms anyway, 

Nit: trailing white space.

::: lib/util/secoid.c
@@ +1891,1 @@
>  	if ((*arg == '+' || *arg == '-') && *++arg) { 

Nit: trailing white space.

::: lib/util/secoidt.h
@@ +471,3 @@
>  };
>  
>  /* New Opaque extended OID table API.  

Nit: trailing white space.
Attachment #8663173 - Flags: review?(emaldona) → review+
This is both 0004 and 0005 since 4 is very short and almost completely rewritten by 0005
Comment on attachment 8668697 [details] [diff] [review]
0005-Check-for-acceptable-certificate-parameters-when-ver.patch (also 0004

Review of attachment 8668697 [details] [diff] [review]:
-----------------------------------------------------------------

Everything is fine in general, but I think we need to use the DER decoder to extract the object ID. That way we don't introduce more areas we have to check for possible DER errors.

::: lib/certhigh/certvfy.c
@@ +158,2 @@
>  	rv = NSS_GetAlgorithmPolicy(hashAlg, &policyFlags);
>  	if (rv == SECSuccess && 

Nit whie space..

::: lib/cryptohi/seckey.c
@@ +1910,5 @@
> +{
> +    SECItem oid = { siBuffer, NULL, 0};
> +    SECOidData *oidData = NULL;
> +
> +    /* 

Nit, fix white space

@@ +1917,5 @@
> +     * before the actual OID and use the OID to look up a named curve.
> +     */
> +    if (params->data[0] != SEC_ASN1_OBJECT_ID) return 0;
> +    oid.len = params->len - 2;
> +    oid.data = params->data + 2;

Hmm this should probably use DER decode with an object id template. We are ignoring the length field here.
Attachment #8668697 - Flags: review-
Comment on attachment 8668697 [details] [diff] [review]
0005-Check-for-acceptable-certificate-parameters-when-ver.patch (also 0004

Review of attachment 8668697 [details] [diff] [review]:
-----------------------------------------------------------------

The code is essentially what freebl does.
Attachment #8668697 - Flags: review- → review+
Comment on attachment 8668754 [details] [diff] [review]
0006-Added-config-parameter.patch

Review of attachment 8668754 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take care of the NSSUTIL_3.21 symbol.

::: lib/util/nssutil.def
@@ +276,5 @@
>  _SGN_VerifyPKCS1DigestInfo;
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSSUTIL_3.16 {         # NSS Utilities 3.16 release

needs to be NSSUTIL_3.21
Attachment #8668754 - Flags: review+
Attachment #8669166 - Flags: review?(rrelyea)
Bob, this last patch broke the tree:
https://circleci.com/gh/nss-dev/nss/119
Flags: needinfo?(rrelyea)
0007 hasn't been applied yet. it was 0001 and 0002 (again). I've backed out 0001 and 0002. 0007 is still waiting. 0003, 0004, 0005, and 0006 are in (already in early). I build with gtests before I push 0001 and 0002, but it may have been an old try and interacted with some other changes. I'm seeing test failures on my 0007 (with everything, so it's probably the same issue.
Flags: needinfo?(rrelyea)
Comment on attachment 8655388 [details]
Patch set for the current (2015-9-1) head.

Extracted the seven patches and those are the ones now under review.
Attachment #8655388 - Flags: review?(rrelyea)
Bob, some of these patches landed, while some are backed out.  Elio noticed that we are exporting NSSUTIL_ArgParseModuleSpecEx and some other functions in 3.21.  Are these functions usable in their current state?

Or, should we hide these by removing them from nss.def until the remaining patches land?
Flags: needinfo?(rrelyea)
The are usable. They will parse the config strings out of the module strings. What isn't there yet is parsing the actual config strings and having them set the policy.

bob
Flags: needinfo?(rrelyea)
0002 as checked in.
Attachment #8663189 - Attachment is obsolete: true
Attachment #8686356 - Flags: review+
0007 applied to the current tree, ready for review.
Attachment #8669166 - Attachment is obsolete: true
Attachment #8669166 - Flags: review?(rrelyea)
Attachment #8686359 - Flags: review?(rrelyea)
Comment on attachment 8686359 [details] [diff] [review]
007-Apply-the-NSS-policies-read-by-the-config-parameter.patch

Review of attachment 8686359 [details] [diff] [review]:
-----------------------------------------------------------------

There's a few things to fix here, I'll attach a new patch with the changes.

::: lib/pk11wrap/pk11pars.c
@@ +145,5 @@
> + * may enable more:
> + * config=curve1:curve2:hash1:hash2:rsa-1024...
> + *
> + * Only the specified hashes and curves will be enabled:
> + * config=sha1:sha256:secp256r1:secp384r1

We also need to be able to change the defaults. We may want to disable sha1 for signing, but not key exchange, for instance,

config=sha1/ssl_kx:sha256

@@ +150,5 @@
> + *
> + * Only the specified hashes and curves will be enabled, and
> + *  RSA keys of 2048 or more will be accepted, and DH key exchange
> + *  with 1024-bit primes or more:
> + * config=sha1:sha256:secp256r1:secp384r1:min-rsa=2048:min-dh=1024

We also need to be able to specify that certain features are turned on or off by default (but the application can change it. This is most critical for SSL cipher suites.

something like:
config={default_on=(list) default_off=(list) policy_on=(list) policy_off=(list) }

It's OK if only policy_on and policy_off are implemented initially.

@@ +358,5 @@
> +    {"TLS1.1", SEC_OID_TLS_V1_1, NSS_USE_ALG_IN_SSL_KX},
> +    {"TLS1.2", SEC_OID_TLS_V1_2, NSS_USE_ALG_IN_SSL_KX},
> +    {"DTLS1.0", SEC_OID_DTLS_V1_0, NSS_USE_ALG_IN_SSL_KX},
> +    {"DTLS1.2", SEC_OID_DTLS_V1_2, NSS_USE_ALG_IN_SSL_KX},
> +};

This is yet another location where we are mapping strings to tags. The main thing this table adds is the default configuration option.

@@ +458,5 @@
>      char *nssc = (char *)nss;
> +    char *configc = NULL;
> +
> +    if (config) {
> +        configc = PORT_Strdup(config); /* no const */

why isn't applyCryptoPolicy using const?

::: lib/util/secoid.c
@@ +1723,5 @@
> +    ODE( SEC_OID_DTLS_V1_2,
> +	"DTLS 1.2 protocol", CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ),
> +    ODE( SEC_OID_APPLY_SSL_POLICY,
> +	"Apply SSL policy (pseudo-OID)", CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ),
> +

The order of the oids must match the order specified in secoidt.h. Failure to include this order will cause an assert at startup.
Attachment #8686359 - Flags: review?(rrelyea) → review-
Updated with my review comments
Attachment #8686359 - Attachment is obsolete: true
Attachment #8691149 - Flags: review?(emaldona)
Attachment #8691149 - Flags: feedback?(martin.thomson)
Comment on attachment 8691149 [details] [diff] [review]
0007-Apply-the-NSS-policies-read-by-the-config-parameter.patch

Review of attachment 8691149 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see a little discussion on the policy format.  From what I can see, the grammar is a little strange, perhaps unnecessarily so.

It's not clear that it's a strictly good thing to have this all dependent on an init function.  Mozilla are looking to reduce our dependency on init functions, this potentially makes it worse.

Whitespace throughout is a mess.

::: lib/pk11wrap/pk11pars.c
@@ +153,5 @@
> + *  with 1024-bit primes or more:
> + * config="policy_off=all policy_on=sha1:sha256:secp256r1:secp384r1:min-rsa=2048:min-dh=1024"
> + *
> + * A policy that enables the AES ciphersuites and the SECP256/384 curves:
> + * config="aes128-cbc:aes128-gcm:TLS1.0:TLS1.2:TLS1.1:HMAC-SHA1:SHA1:SHA256:SHA384:RSA:ECDHE-RSA:SECP256R1:SECP384R1

This format needs to be more precisely specified.  The names that are accepted, whether they are case sensitive tokens (apparently not), and what the grammar for the config is.  It's not clear from this what the processing rules might be.

@@ +177,5 @@
> +    PRUint32 flag;
> +} policyFlagDef;
> +
> +/*
> + *  This table should be merged with the SECOID table. 

Trailing space.

@@ +382,5 @@
> +static const policyFlagDef policyFlagList[] = {
> +    {CIPHER_NAME("SSL"), NSS_USE_ALG_IN_SSL},
> +    {CIPHER_NAME("SSL_KEY_EXCHANGE"), NSS_USE_ALG_IN_SSL_KX},
> +    /* add other key exhanges in the future */
> +    {CIPHER_NAME("KEY_ EXCHANGE"), NSS_USE_ALG_IN_SSL_KX}, 

please clean up all the trailing whitespace

@@ +528,5 @@
> +        
> +        for (i = 0; i < PR_ARRAY_SIZE(freeOptList); i++) {
> +	    if (cipher[freeOptList[i].name_size] == '=' &&
> +		PORT_Strncasecmp(freeOptList[i].name, cipher, 
> +					freeOptList[i].name_size) == 0 ) {

ws, alignment

@@ +557,5 @@
> +
> +    if (policyConfig == NULL) {
> +	return SECSuccess; /* no policy given */
> +    }
> +    policy_off = NSSUTIL_ArgGetParamValue("policy_off",policyConfig);

space after comma

::: lib/ssl/ssl3con.c
@@ +275,5 @@
> +    /*                                       y  c               |  o  g  n d */
> +    /*                                       |  r               |  c  |  c | */
> +    /*                                       |  e               |  k  |  e | */
> +    /*                                       |  t               |  |  |  | | */
> +    {cipher_null,         calg_null,         0, 0, type_stream, 0, 0, 0, 0, SEC_OID_NULL_CIPHER},

Does it even make sense to have a null cipher OID?  We certainly don't want to be able to turn this off (or maybe we don't want to turn it on).

@@ +12944,5 @@
> +    {SSL_LIBRARY_VERSION_TLS_1_1, ssl_variant_datagram},
> +    {SSL_LIBRARY_VERSION_TLS_1_2, ssl_variant_datagram},
> +};
> +
> +SECStatus SSL_applyNSSPolicy(void)

The name here doesn't follow convention.

::: lib/util/secoidt.h
@@ +471,5 @@
> +    SEC_OID_TLS_ECDH_RSA                    = 337,
> +    SEC_OID_TLS_ECDH_ANON                   = 338,
> +    SEC_OID_TLS_RSA_EXPORT                  = 339,
> +
> +    SEC_OID_SSL_V2_0                  = 340,

Seriously?

::: lib/util/utilpars.c
@@ +127,5 @@
>  /*
>   * point to the next parameter in string
>   */
> +const char *
> +NSSUTIL_ArgSkipParameter(const char *string) 

ws

@@ +292,5 @@
>  /*
>   * read an argument at a Long integer
>   */
>  long
> +NSSUTIL_ArgReadLong(const char *label, const char *params, 

ws, plus more below
Attachment #8691149 - Flags: feedback?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #47)
> Comment on attachment 8691149 [details] [diff] [review]
> 0007-Apply-the-NSS-policies-read-by-the-config-parameter.patch
> 
> Review of attachment 8691149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see a little discussion on the policy format.  From what I can
> see, the grammar is a little strange, perhaps unnecessarily so.

For the discussion let's keeping in mind the purpose of the set of patches. I hope http://fedoraproject.org/wiki/Changes/CryptoPolicy will help.
Comment on attachment 8691149 [details] [diff] [review]
0007-Apply-the-NSS-policies-read-by-the-config-parameter.patch

Review of attachment 8691149 [details] [diff] [review]:
-----------------------------------------------------------------

My problem with the patch is in ssl_Init we are ignoring the return value of the call to ssl_ApplyCryptoPolicies, otherwise I'm okay with the overall patch. I understand that a set of tests are coming. I will be willing to review or at least give feedback on those.

::: lib/pk11wrap/pk11pars.c
@@ +19,1 @@
>     

ws

@@ +19,2 @@
>     
>  #include "utilpars.h" 

ws

@@ +759,5 @@
>   * spec.
>   */
>  char *
>  secmod_ParseModuleSpecForTokens(PRBool convert, PRBool isFIPS, 
> +				const char *moduleSpec, char ***children, 

ws

::: lib/pk11wrap/secmodi.h
@@ +80,1 @@
>  				      char ***children, 

ws

::: lib/softoken/sftkpars.c
@@ +37,5 @@
>      return;
>  }
>  
>  static CK_RV
>  sftk_parseTokenParameters(char *param, sftk_token_parameters *parsed) 

ws

@@ +115,1 @@
>      return; 

ws

@@ +115,5 @@
>      return; 
>  }
>  
>  CK_RV
>  sftk_parseParameters(char *param, sftk_parameters *parsed, PRBool isFIPS) 

ws

::: lib/ssl/sslimpl.h
@@ +777,4 @@
>  };
>  
>  typedef enum {
>      wait_client_hello, 

ws

@@ +777,5 @@
>  };
>  
>  typedef enum {
>      wait_client_hello, 
>      wait_client_cert, 

ws

@@ +779,5 @@
>  typedef enum {
>      wait_client_hello, 
>      wait_client_cert, 
>      wait_client_key,
>      wait_cert_verify, 

ws

@@ +780,5 @@
>      wait_client_hello, 
>      wait_client_cert, 
>      wait_client_key,
>      wait_cert_verify, 
>      wait_change_cipher, 

ws

@@ +1949,5 @@
>  SECStatus SSL_DisableDefaultExportCipherSuites(void);
>  SECStatus SSL_DisableExportCipherSuites(PRFileDesc * fd);
>  PRBool    SSL_IsExportCipherSuite(PRUint16 cipherSuite);
>  
> +SECStatus SSL_applyNSSPolicy(void);

As noted elsewhere, this is a private function and I suggest renaming it
ssl_ApplyNSSPolicy per our coding conventions.

::: lib/ssl/sslinit.c
@@ +30,2 @@
>  	ssl_inited = 1;
> +	SSL_applyNSSPolicy();

It's ignoring the SECStatus that SSL_applyNSSPolicy returns. 
This is a private function. I suggest renaming it be ssl_ApplyNSSPolicy.

::: lib/util/secoidt.h
@@ +438,1 @@
>       * szOID_KP_CTL_USAGE_SIGNING 

ws

@@ +515,2 @@
>  
>  /* New Opaque extended OID table API.  

ws

::: lib/util/utilpars.c
@@ +1112,4 @@
>      const char *lconfigdir;
>      PRBool noModDB = PR_FALSE;
>      param = NSSUTIL_ArgStrip(param);
>  	

whitespace

::: lib/util/utilpars.h
@@ +21,2 @@
>  PRBool NSSUTIL_ArgIsBlank(char c);
> +PRBool NSSUTIL_ArgHasFlag(const char *label, const char *flag, 

ws

@@ +21,4 @@
>  PRBool NSSUTIL_ArgIsBlank(char c);
> +PRBool NSSUTIL_ArgHasFlag(const char *label, const char *flag, 
> +			  const char *parameters);
> +long NSSUTIL_ArgReadLong(const char *label,const char *params, long defValue, 

ws

@@ +45,1 @@
>  char *NSSUTIL_MkModuleSpec(char *dllName, char *commonName, 

ws

@@ +45,3 @@
>  char *NSSUTIL_MkModuleSpec(char *dllName, char *commonName, 
>  					char *parameters, char *NSS);
>  char *NSSUTIL_MkModuleSpecEx(char *dllName, char *commonName, 

ws

@@ +55,5 @@
>  
>  /*
>   * private functions for softoken.
>   */
> +char * _NSSUTIL_GetSecmodName(const char *param, NSSDBType *dbType, 

ws
Attachment #8691149 - Flags: review?(emaldona) → review-
Comment on attachment 8691149 [details] [diff] [review]
0007-Apply-the-NSS-policies-read-by-the-config-parameter.patch

Review of attachment 8691149 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +12944,5 @@
> +    {SSL_LIBRARY_VERSION_TLS_1_1, ssl_variant_datagram},
> +    {SSL_LIBRARY_VERSION_TLS_1_2, ssl_variant_datagram},
> +};
> +
> +SECStatus SSL_applyNSSPolicy(void)

Looking at this function more closely it seems that it will always return SECSuccess. As I understand the code if a failure is detected it will just continue and the result is that it will silently not apply the requested policy thus keeping the default in place. It's only caller is ssl_Init which ignores the result to which I complained. This is the type of practice that covscan complains about and I end up having to do waivers. Perhaps checking the result coupled with an assert will be a good thing to add to ssl_init.

@@ +13021,5 @@
> +        }
> +    }
> +
> +    return SECSuccess;
> +}

Looking at
Updated patch with Elio's comments, test cases and fixes to make the test cases work.
Attachment #8691149 - Attachment is obsolete: true
Attachment #8697679 - Flags: review?(emaldona)
Comment on attachment 8697679 [details] [diff] [review]
0007-Apply-the-NSS-policies-read-by-the-config-parameter.patch

Review of attachment 8697679 [details] [diff] [review]:
-----------------------------------------------------------------

This is a much improved patch but I must reject it because of the hard-coded paths in ssl.sh. I would very much like to run the policy tests myself as it will give me greater confidence on my own review, not to mention that we need to add this to the automated test suite.

::: lib/nss/nssoptions.c
@@ +28,5 @@
>  static struct nssOps nss_ops = {
>      SSL_RSA_MIN_MODULUS_BITS,
>      SSL_DH_MIN_P_BITS,
> +    SSL_DSA_MIN_P_BITS,
> +    1,   /* Set TLS min to less the the smallest legal SSL value */

nit: could you align the start of the comment with one below?

::: lib/pk11wrap/pk11pars.c
@@ +19,1 @@
>     

nit: ws

@@ +498,5 @@
> +	}
> +    }
> +    return val;
> +}
> +    

spaces

@@ +499,5 @@
> +    }
> +    return val;
> +}
> +    
> +    

omit the line

@@ +587,5 @@
> +        }
> +	if (!unknown) {
> +	    continue;
> +	}
> +        

nit: spaces

@@ +591,5 @@
> +        
> +        for (i = 0; i < PR_ARRAY_SIZE(freeOptList); i++) {
> +	    const optionFreeDef *freeOpt = &freeOptList[i];
> +	    unsigned name_size = freeOpt->name_size;
> +			

nit: spaces

@@ +608,5 @@
> +		 * we don't understand */
> +		unknown = PR_FALSE;
> +                break;
> +	    }
> +        

nit: spaces

@@ +639,5 @@
> +    allow = NSSUTIL_ArgGetParamValue("allow",policyConfig);
> +    rv = secmod_applyCryptoPolicy(allow, PR_TRUE);
> +    if (allow) PORT_Free(allow);
> +    return rv;
> +}    

nit: trailing spaces

::: lib/pk11wrap/secmodi.h
@@ +80,1 @@
>  				      char ***children, 

nit: space

::: lib/ssl/ssl3con.c
@@ +12931,5 @@
>  }
>  
> +#define MAP_NULL(x)  (((x)!=0)?(x):SEC_OID_NULL_CIPHER)
> +
> +SECStatus ssl_applyNSSPolicy(void)

Suggest renaming ssl_ApplyNSSPolicy to follow the style used in other functions.

::: lib/ssl/sslinit.c
@@ +30,5 @@
> +#ifdef DEBUG
> +        ssl3_CheckCipherSuiteOrderConsistency();
> +#endif
> +
> +    rv = ssl_applyNSSPolicy();

Suggest renaming ssl_ApplyNSSPolicy to follow the style used in other functions.

::: lib/util/secoidt.h
@@ +438,1 @@
>       * szOID_KP_CTL_USAGE_SIGNING 

trailing space

@@ +477,5 @@
> +    SEC_OID_TLS_DH_RSA_EXPORT          = 342,
> +    SEC_OID_TLS_DH_DSS_EXPORT          = 343,
> +    SEC_OID_TLS_DH_ANON_EXPORT         = 344,
> +    SEC_OID_APPLY_SSL_POLICY           = 345,
> +

Nit: it would be nice to align the values for these new six ones with ones above.

@@ +508,2 @@
>  
>  /* New Opaque extended OID table API.  

trailing spaces

::: lib/util/utilmod.c
@@ +605,4 @@
>  }
>  
>  /*
>   * Add a module to the Data base 

nit: trailing space

::: lib/util/utilpars.c
@@ +170,5 @@
>  	parameters = NSSUTIL_ArgStrip(parameters);
>     }
>     return returnValue;
>  }
>    

ws

@@ +173,5 @@
>  }
>    
>  /*
>   * find the next flag in the parameter list
>   */  

ws

@@ +264,5 @@
>   * parameters are tag value pairs. This function returns the tag or label (the
>   * value before the equal size.
>   */
>  char *
> +NSSUTIL_ArgGetLabel(const char *inString, int *next) 

trailing space

@@ +292,5 @@
>  /*
>   * read an argument at a Long integer
>   */
>  long
> +NSSUTIL_ArgReadLong(const char *label, const char *params, 

nit: trailing space

@@ +298,4 @@
>  {
>      char *value;
>      long retValue;
>      if (isdefault) *isdefault = PR_FALSE; 

nit: trailing space

@@ +560,3 @@
>  };
>  
>  static int nssutil_argSlotFlagTableSize = 

nit: trailing space

@@ +618,1 @@
>      slotInfo->hasRootCerts = NSSUTIL_ArgHasFlag("rootFlags", "hasRootCerts", 

trailing space

@@ +618,3 @@
>      slotInfo->hasRootCerts = NSSUTIL_ArgHasFlag("rootFlags", "hasRootCerts", 
>                                                 params);
>      slotInfo->hasRootTrust = NSSUTIL_ArgHasFlag("rootFlags", "hasRootTrust", 

trailing space

@@ +622,5 @@
>  }
>  
>  /* parse all the slot specific parameters. */
>  struct NSSUTILPreSlotInfoStr *
> +NSSUTIL_ArgParseSlotInfo(PLArenaPool *arena, const char *slotParams, 

trailing space

@@ +633,5 @@
>      *retCount = 0;
>      if ((slotParams == NULL) || (*slotParams == 0))  return NULL;
>  
>      /* first count the number of slots */
>      for (slotIndex = NSSUTIL_ArgStrip(slotParams); *slotIndex; 

trailing space

@@ +971,3 @@
>  /* Assemble a full NSS string. */
>  char *
>  NSSUTIL_MkNSSString(char **slotStrings, int slotCount, PRBool internal, 

trailing space

@@ +971,4 @@
>  /* Assemble a full NSS string. */
>  char *
>  NSSUTIL_MkNSSString(char **slotStrings, int slotCount, PRBool internal, 
>  	  PRBool isFIPS, PRBool isModuleDB,  PRBool isModuleDBOnly, 

trailing space

@@ +971,5 @@
>  /* Assemble a full NSS string. */
>  char *
>  NSSUTIL_MkNSSString(char **slotStrings, int slotCount, PRBool internal, 
>  	  PRBool isFIPS, PRBool isModuleDB,  PRBool isModuleDBOnly, 
>  	  PRBool isCritical, unsigned long trustOrder, 

trailing space

@@ +1112,4 @@
>      const char *lconfigdir;
>      PRBool noModDB = PR_FALSE;
>      param = NSSUTIL_ArgStrip(param);
>  	

nit: ws - looks like the line could be omitted

::: lib/util/utilpars.h
@@ +21,2 @@
>  PRBool NSSUTIL_ArgIsBlank(char c);
> +PRBool NSSUTIL_ArgHasFlag(const char *label, const char *flag, 

trailing space

@@ +21,4 @@
>  PRBool NSSUTIL_ArgIsBlank(char c);
> +PRBool NSSUTIL_ArgHasFlag(const char *label, const char *flag, 
> +			  const char *parameters);
> +long NSSUTIL_ArgReadLong(const char *label,const char *params, long defValue, 

trailing space

@@ +45,1 @@
>  char *NSSUTIL_MkModuleSpec(char *dllName, char *commonName, 

nit: trailing space

@@ +45,3 @@
>  char *NSSUTIL_MkModuleSpec(char *dllName, char *commonName, 
>  					char *parameters, char *NSS);
>  char *NSSUTIL_MkModuleSpecEx(char *dllName, char *commonName, 

nit: trailing space

@@ +55,5 @@
>  
>  /*
>   * private functions for softoken.
>   */
> +char * _NSSUTIL_GetSecmodName(const char *param, NSSDBType *dbType, 

nit: trailing space

::: tests/all.sh
@@ +120,5 @@
>          fi
>  
>          SCRIPTNAME=${TEST}.sh
>          echo "Running tests for ${TEST}"
>          echo "TIMESTAMP ${TEST} BEGIN: `date`" 

nit: ws

@@ +281,5 @@
>  TESTS=${NSS_TESTS:-$tests}
>  
>  ALL_TESTS=${TESTS}
>  
> +nss_ssl_tests="crl bypass_normal normal_bypass fips_normal normal_fips iopr policy"

The patch didn't apply cleanly against the latest sources. When applying I got a tests/all.sh.rej with this hunk on it. Easy to fix so it didn't stop me from reviewing.

::: tests/ssl/ssl.sh
@@ +711,5 @@
> +  start_selfserv # Launch the server
> +
> +  VMIN="ssl3"
> +  VMAX="tls1.2"
> +               

nit: ws

@@ +717,5 @@
> +  while read value ectype testmax param policy testname
> +  do
> +      SSL2_FLAGS=
> +      VMIN="ssl3"
> +      

nit: ws

@@ +751,5 @@
> +              mixed=1
> +            else
> +              is_selfserv_alive
> +            fi
> +          else 

nit: ws

@@ +767,5 @@
> +
> +          cat  > ${P_R_CLIENTDIR}/pkcs11.txt << ++EOF++
> +library=
> +name=NSS Internal PKCS #11 Module
> +parameters=configdir='/builds/nss-hg-2/tests_results/security/rrelyea-laptop.31/sharedb/client' certPrefix='' keyPrefix='' secmod='secmod.db' flags= updatedir='' updateCertPrefix='' updateKeyPrefix='' updateid='' updateTokenDescription='' 

The path /builds/nss-hg-2/tests_results/security/rrelyea-laptop.31/sharedb/client is your specific setup hard-coded into the script which won't allow others, humans and the buildbot, to run this test as part of the test suite.
Also nitpick on trailing space.

@@ +773,5 @@
> +++EOF++
> +          echo "config=${policy}" >> ${P_R_CLIENTDIR}/pkcs11.txt
> +          cat  >> ${P_R_CLIENTDIR}/pkcs11.txt << ++EOF++
> +
> +library=/builds/nss-hg-2/dist/Linux3.10_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssckbi.so

Another hardcoded patch to your setup.

@@ +790,5 @@
> +          ${PROFTOOL} ${BINDIR}/tstclnt -p ${PORT} -h ${HOSTADDR} -c ${param} -V ${VMIN}:${VMAX} ${CLIENT_OPTIONS} -f \
> +                  -d ${P_R_CLIENTDIR} -v -w nss < ${REQUEST_FILE} \
> +                  >${TMP}/$HOST.tmp.$$  2>&1
> +          ret=$?
> +          cat ${TMP}/$HOST.tmp.$$ 

nit: trailing space

@@ +912,2 @@
>          fi
>          start_selfserv        

nit: trailing spaces. was actually present before the patch like may others.

::: tests/ssl/sslpolicy.txt
@@ +146,5 @@
> +#  allow=min-rsa=512:min-dh=1024
> +#
> +# Exp Enable Enable Cipher Config Policy      Test Name
> +# Ret  EC     TLS
> +# turn on single cipher 

nit: trailing space

@@ +155,5 @@
> +# turn off signature only
> +  1 noECC  SSL3   d    disallow=sha256 Disallow SHA256 Signatures Explicitly.
> +  1 noECC  SSL3   d    disallow=all_allow=hmac-sha1:rsa/ssl-key-exchange:des-ede3-cbc:tls-version-min=ssl3.0:tls-version-max=ssl3.0 Disallow SHA256 Signatures Implicitly Narrow.
> +  1 noECC  SSL3   d    disallow=all_allow=md2/all:md4/all:md5/all:sha1/all:sha384/all:sha512/all:hmac-sha1/all:hmac-sha224/all:hmac-sha256/all:hmac-sha384/all:hmac-sha512/all:hmac-md5/all:camellia128-cbc/all:camellia192-cbc/all:camellia256-cbc/all:seed-cbc/all:des-ede3-cbc/all:des-40-cbc/all:des-cbc/all:null-cipher/all:rc2/all:rc4/all:idea/all:rsa/all:rsa-export/all:dhe-rsa/all:dhe-dss/all:ecdhe-ecdsa/all:ecdhe-rsa/all:ecdh-ecdsa/all:ecdh-rsa/all:tls-version-min=ssl2.0:tls-version-max=tls1.2 Disallow SHA256 Signatures Implicitly.
> +# turn off single cipher 

nit: trailing space

@@ +163,5 @@
> +# turn off H-Mac
> +  1 noECC  SSL3   d    disallow=hmac-sha1 Disallow HMAC Explicitly
> +  1 noECC  SSL3   d    disallow=all_allow=md5:sha256:rsa:des-ede3-cbc:tls-version-min=ssl3.0:tls-version-max=ssl3.0 Disallow HMAC Implicitly Narrow.
> +  1 noECC  SSL3   d    disallow=all_allow=md2/all:md4/all:md5/all:sha1/all:sha256/all:sha384/all:sha512/all:hmac-sha224/all:hmac-sha256/all:hmac-sha384/all:hmac-sha512/all:hmac-md5/all:camellia128-cbc/all:camellia192-cbc/all:camellia256-cbc/all:seed-cbc/all:des-ede3-cbc/all:des-40-cbc/all:des-cbc/all:null-cipher/all:rc2/all:rc4/all:idea/all:rsa/all:rsa-export/all:dhe-rsa/all:dhe-dss/all:ecdhe-ecdsa/all:ecdhe-rsa/all:ecdh-ecdsa/all:ecdh-rsa/all:tls-version-min=ssl2.0:tls-version-max=tls1.2 Disallow HMAC Signatures Implicitly.
> +# turn off key exchange 

nit: trailing space
Attachment #8697679 - Flags: review?(emaldona) → review-
> The path /builds/nss-hg-2/tests_results/security/rrelyea-laptop.31/sharedb/client is your specific setup 
> hard-coded into the script which won't allow others, humans and the buildbot, to run this test as part of the 
> test suite.

I'll fix it, but this path is a noop in normal operations. I've already ran this test against different paths, so it shouldn't affect your ability to run the tests.

> +library=/builds/nss-hg-2/dist/Linux3.10_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssckbi.so

This one is an issue. Good catch.

bob
This fixes most of the issues Elio identified. I did not fix all the white space issues that were preexisting, though if I modified the existing line, I removed the extraneous white space.

This patch was made from the trunk as of 3:20 p.m. PDT.
Attachment #8697679 - Attachment is obsolete: true
Attachment #8698239 - Flags: review?(emaldona)
Comment on attachment 8698239 [details] [diff] [review]
0007-Apply-the-NSS-policies-read-by-the-config-parameter.patch

Review of attachment 8698239 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +12953,5 @@
>  
> +#define MAP_NULL(x)  (((x)!=0)?(x):SEC_OID_NULL_CIPHER)
> +
> +SECStatus ssl3_ApplyNSSPolicy(void)
> + {

nit: leading space

::: tests/ssl/ssl.sh
@@ +722,5 @@
>  
>    html "</TABLE><BR>"
>  }
>  
> +############################## ssl_cov #################################

change it to be ssl_policy

@@ +804,5 @@
> +
> +          cat  > ${P_R_CLIENTDIR}/pkcs11.txt << ++EOF++
> +library=
> +name=NSS Internal PKCS #11 Module
> +parameters=configdir='./client' certPrefix='' keyPrefix='' secmod='secmod.db' flags= updatedir='' updateCertPrefix='' updateKeyPrefix='' updateid='' updateTokenDescription='' 

nit: trailing space

@@ +827,5 @@
> +          ${PROFTOOL} ${BINDIR}/tstclnt -p ${PORT} -h ${HOSTADDR} -c ${param} -V ${VMIN}:${VMAX} ${CLIENT_OPTIONS} -f \
> +                  -d ${P_R_CLIENTDIR} -v -w nss < ${REQUEST_FILE} \
> +                  >${TMP}/$HOST.tmp.$$  2>&1
> +          ret=$?
> +          cat ${TMP}/$HOST.tmp.$$ 

nit: trailing space

::: tests/ssl/sslpolicy.txt
@@ +146,5 @@
> +#  allow=min-rsa=512:min-dh=1024
> +#
> +# Exp Enable Enable Cipher Config Policy      Test Name
> +# Ret  EC     TLS
> +# turn on single cipher 

nit: trailing space

@@ +155,5 @@
> +# turn off signature only
> +  1 noECC  SSL3   d    disallow=sha256 Disallow SHA256 Signatures Explicitly.
> +  1 noECC  SSL3   d    disallow=all_allow=hmac-sha1:rsa/ssl-key-exchange:des-ede3-cbc:tls-version-min=ssl3.0:tls-version-max=ssl3.0 Disallow SHA256 Signatures Implicitly Narrow.
> +  1 noECC  SSL3   d    disallow=all_allow=md2/all:md4/all:md5/all:sha1/all:sha384/all:sha512/all:hmac-sha1/all:hmac-sha224/all:hmac-sha256/all:hmac-sha384/all:hmac-sha512/all:hmac-md5/all:camellia128-cbc/all:camellia192-cbc/all:camellia256-cbc/all:seed-cbc/all:des-ede3-cbc/all:des-40-cbc/all:des-cbc/all:null-cipher/all:rc2/all:rc4/all:idea/all:rsa/all:rsa-export/all:dhe-rsa/all:dhe-dss/all:ecdhe-ecdsa/all:ecdhe-rsa/all:ecdh-ecdsa/all:ecdh-rsa/all:tls-version-min=ssl2.0:tls-version-max=tls1.2 Disallow SHA256 Signatures Implicitly.
> +# turn off single cipher 

nit: trailing space

@@ +163,5 @@
> +# turn off H-Mac
> +  1 noECC  SSL3   d    disallow=hmac-sha1 Disallow HMAC Explicitly
> +  1 noECC  SSL3   d    disallow=all_allow=md5:sha256:rsa:des-ede3-cbc:tls-version-min=ssl3.0:tls-version-max=ssl3.0 Disallow HMAC Implicitly Narrow.
> +  1 noECC  SSL3   d    disallow=all_allow=md2/all:md4/all:md5/all:sha1/all:sha256/all:sha384/all:sha512/all:hmac-sha224/all:hmac-sha256/all:hmac-sha384/all:hmac-sha512/all:hmac-md5/all:camellia128-cbc/all:camellia192-cbc/all:camellia256-cbc/all:seed-cbc/all:des-ede3-cbc/all:des-40-cbc/all:des-cbc/all:null-cipher/all:rc2/all:rc4/all:idea/all:rsa/all:rsa-export/all:dhe-rsa/all:dhe-dss/all:ecdhe-ecdsa/all:ecdhe-rsa/all:ecdh-ecdsa/all:ecdh-rsa/all:tls-version-min=ssl2.0:tls-version-max=tls1.2 Disallow HMAC Signatures Implicitly.
> +# turn off key exchange 

nit: trailing space
Attachment #8698239 - Flags: review?(emaldona) → review+
Bob, please hold checking in the changes. I have noticed in may system the total number of tests reported passing is less than normal. Around high 8 thousand versus more than 12 without the patcg K and I haven't figured out what's causing this. I would like to look at this you. How may tests do you get passing?

-Elio
Attachment #8698239 - Flags: review+
I checked the most recent build using fedora (without patch 7) and I get:

https://bot.nss-crypto.org:8011/builders/fedora-x64-DBG/builds/620/steps/shell/logs/stdio
grep -c PASSED stdio -- 12874
grep -c FAILED stdio -- 0

I expect similar numbers with patch 7 applied. There could be something wrong on my setup. Sorry for being paranoid but this is such an important piece of work.
Attachment #8698239 - Flags: review+
I saw 14485 tests in a ppc64 build that already completed.
I found the reason why my local runs of the test gave me a low number of tests executed and it's due to a syntax error in ssl.sh. 
$ bash -x tests/ssl/ssl.sh 
tests/ssl/ssl.sh: line 845: syntax error near unexpected token `<'
tests/ssl/ssl.sh: line 845: `  html "</TABLE><BR>"'
Inside the while loop
  while read value ectype testmax param policy testname
  do
   ....
echo "library=${DIST}/${OBJDIR}/lib/libnssckbi.so"" >> ${P_R_CLIENTDIR}/pkcs11.txt >> ....
                                                  ^ - repeated double quote
   ....
  done
I'll attach a patch.
Attachment #8700445 - Flags: review?(ekr)
Comment on attachment 8700445 [details] [diff] [review]
Fixes the ssl.sh syntax error I pointed out in Comment 59

Review of attachment 8700445 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8700445 - Flags: review?(ekr) → review+
Comment on attachment 8700445 [details] [diff] [review]
Fixes the ssl.sh syntax error I pointed out in Comment 59

Thank Eric for the review.
Attachment #8700445 - Flags: review?(rrelyea)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1181814
No longer depends on: 1181814
Target Milestone: --- → 3.22
A follow-up checkin was made to fix a bug introduced with this patch,
see https://hg.mozilla.org/projects/nss/rev/72122a7dc17e
and 1228410 comment 4 and following.
Duplicate of this bug: 1261800
Bob, why did you restrict execution of the ssl_policy test to the sharedb scenario?

        case "${SSL_TEST}" in
        "policy")
            if [ "${TEST_MODE}" = "SHARED_DB" ] ; then
	        ssl_policy
            fi
Flags: needinfo?(rrelyea)
Because the policy tests doesn't use policy.cfg, but adds policy lines to pkcs11.txt. The dbm database can't store policy (because secmod.db doesn't know how to store or generate policy information). I belioeve that not all platforms load policy.cfg, but all platforms can take policy directives from pkcs11.txt (or a separate loadable module).
Flags: needinfo?(rrelyea)
Blocks: 1474875
You need to log in before you can comment on or make changes to this bug.