785169 - Change tools to use a version range for SSL/TLS versions + adjust the test suite

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Kai Engert [:KaiE:]]

We should enhance the NSS test tools to support TLS 1.1

Let's start with selfserv, tstclnt and strsclnt.

I propose to remove the old configuration options -2, -3, -T
and replace them with (a) new option(s) that can be used to define the range of allowed protocol versions.

By removing the old flags, we can enforce that all existing tests get updated to correctly specify the intended set of enabled versions.

Comment 1

[Kai Engert [:KaiE:]]

Bob, Elio,

selfserv uses the following statement related to "bypass":

	protos = (disableTLS ? 0 : SSL_CBP_TLS1_0) +
		 (disableSSL3 ? 0 : SSL_CBP_SSL3);

Can you please come up with a new definition for the protos variable,
based on SSLVersionRange { min, max }, where min and max can be any of the following?

#define SSL_LIBRARY_VERSION_2			0x0002
#define SSL_LIBRARY_VERSION_3_0			0x0300
#define SSL_LIBRARY_VERSION_TLS_1_0		0x0301
#define SSL_LIBRARY_VERSION_TLS_1_1		0x0302

Comment 2

[Kai Engert [:KaiE:]]

I suspect we want:

        protos = 0;
        if (enabledVersions.min <= SSL_LIBRARY_VERSION_TLS_1_0
            && enabledVersions.max >= SSL_LIBRARY_VERSION_TLS_1_0) {
            protos += SSL_CBP_TLS1_0;
        }
        if (enabledVersions.min <= SSL_LIBRARY_VERSION_3_0
            && enabledVersions.max >= SSL_LIBRARY_VERSION_3_0) {
            protos += SSL_CBP_SSL3;
        }

Comment 3

[Kai Engert [:KaiE:]]

Attachment: patch v1

(no help output yet)

Comment 4

[Kai Engert [:KaiE:]]

Attachment: patch v2

Comment 5

[Elio Maldonado]

I see a bit a of a problem with the location of the changes in the as far as basicutil.{c|h} portion is concerned. 

In mozilla/security/nss/cmd/lib/basicutil.c we now have
+#include "sslproto.h"
+#include "sslt.h"
plus SECU_GetVersionIDFromName(const char *input, PRUint16 *v) and other functions.

In mozilla/security/nss/cmd/lib/basicutil.h the #include "sslt.t". 

The problem is that would defeat what was accomplished earlier with the split of secutil.a test/tool support library sources/headers into low and high level parts. We basically created a subset of the library, basicutil, that didn't rely of higher levels of nss so we could run the crypto-only tests even when the higher layers aren't available at build time as is the case in Fedora & RHEL 6 and above.  Kaie, Could these portions be moved from basicutil.{c|h} to secutil.{c|h} ?

Thank you,

Elio

Comment 6

[Kai Engert [:KaiE:]]

(In reply to Elio Maldonado from comment #5)
> Could these portions be moved from basicutil.{c|h} to secutil.{c|h} ?

ok

Comment 7

[Kai Engert [:KaiE:]]

In this bug, in addition to changing the tools's parameters, I want to include the changes to the test suite, that will use the new parameters instead of the old ones.

My goal is a successful run of all tests, while still using the old tests
(not yet running anything in TLS 1.1 mode).

I will attach a patch that works for me for this purpose.

Comment 8

[Kai Engert [:KaiE:]]

Also, while touching the tools anyway, I would like to include an unrelated change.

I would like to change the usage output of tstclnt and selfserv, with the intention to have to scroll up less often.

Instead of always printing the long list of cipher suites,
I want to omit them by default.

The list of cipher paramters will only be printed if the user requests it with "-H".

Comment 9

[Kai Engert [:KaiE:]]

Attachment: patch v10

Wan-Teh, as this is a precondition for testing TLS 1.1, are you interested to review this patch?

Comment 10

[Elio Maldonado]

Comment on attachment 660839 [details] [diff] [review]
patch v10

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

An oversight, basicutil.h got the "include sslt.h" which it shouldn't for the reasons stated earlier. secutil.c already includes sslt.h. I have tested this patch (minus the first section) in fedora and Mac OS X and all tests passed.

Comment 11

[Elio Maldonado]

Comment on attachment 660839 [details] [diff] [review]
patch v10

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

::: mozilla/security/nss/cmd/lib/secutil.c
@@ +3564,5 @@
> +{
> +    if (!input || !v)
> +        return SECFailure;
> +    
> +    if (!strcasecmp(input, "ssl2")

strcasecmp wouldn't compile on Windows 7. strcasecmp is defined on <strings.h> which isn't available on Windows. You could try something similar to

#if defined _MSC_VER && !defined strcasecmp
 #define strcasecmp stricmp
#endif

::: mozilla/security/nss/cmd/lib/secutil.h
@@ +16,5 @@
>  #include <stdio.h>
>  
>  #include "basicutil.h"
>  #include "sslerr.h"
> +#include "sslt.h"

Remove this line.

Comment 12

[Ryan Sleevi]

Comment on attachment 660839 [details] [diff] [review]
patch v10

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

Thanks for writing this patch, Kai. I had just a few "drive by" comments regarding it, in addition to what Elio highlighted.

::: mozilla/security/nss/cmd/lib/secutil.c
@@ +3564,5 @@
> +{
> +    if (!input || !v)
> +        return SECFailure;
> +    
> +    if (!strcasecmp(input, "ssl2")

Or use the NSPR routine - PL_strcasecmp

@@ +3587,5 @@
> +    }
> +    else if (!strcasecmp(input, "ssl3.2")
> +             || !strcasecmp(input, "tls1.1")
> +             || !strcasecmp(input, "3.2")) {
> +        *v = SSL_LIBRARY_VERSION_TLS_1_1;

I don't believe it's good or necessary to support all of these forms. It seems better that each value should have a single canonical form.

ssl2.0
ssl3.0
tls1.0
tls1.1

(and eventually)
dtls1.0


It's better to be strict in what you'll accept here when dealing with command-line arguments.

@@ +3635,5 @@
> +        vr->max = SSL_LIBRARY_VERSION_TLS_1_1;
> +    else if (SECU_GetVersionIDFromName(maxStr, &vr->max) != SECSuccess)
> +        return SECFailure;
> +    
> +    return SECSuccess;

nit: It would seem good not to introduce new functions with inconsistent whitespace.

::: mozilla/security/nss/cmd/selfserv/selfserv.c
@@ +2291,5 @@
> +        }
> +        if (enabledVersions.min <= SSL_LIBRARY_VERSION_3_0
> +            && enabledVersions.max >= SSL_LIBRARY_VERSION_3_0) {
> +            protos += SSL_CBP_SSL3;
> +        }

nit: It seems more readable to order these in an incrementing order (ssl3 -> tls1 -> tls1.1) rather than in a decrementing order

::: mozilla/security/nss/cmd/strsclnt/strsclnt.c
@@ +1171,2 @@
>      rv = SSL_OptionSet(model_sock, SSL_SECURITY,
> +        enabledSSL2 || !(!enabledVersions.min && !enabledVersions.max));

Rather than using negation here (especially double-plus-conjunctive negation), it seems like it would be better as

enabledSSL2 || (enabledVersions.min > 0 && enabledVersions.max > 0)

Comment 13

[Kai Engert [:KaiE:]]

> @@ +3587,5 @@
> > +    }
> > +    else if (!strcasecmp(input, "ssl3.2")
> > +             || !strcasecmp(input, "tls1.1")
> > +             || !strcasecmp(input, "3.2")) {
> > +        *v = SSL_LIBRARY_VERSION_TLS_1_1;
> 
> I don't believe it's good or necessary to support all of these forms. It
> seems better that each value should have a single canonical form.
> 
> ssl2.0
> ssl3.0
> tls1.0
> tls1.1
> 
> (and eventually)
> dtls1.0


I personally don't have any preference - any style works for me.

I added this flexibility because of my reading of the other bugs.
I was under the impression that people had different preferences, and by introducing the flexibility I tried to avoid a discussion.


Can everyone agree with Ryan's proposal?

Comment 14

[Kai Engert [:KaiE:]]

> @@ +3635,5 @@
> > +        vr->max = SSL_LIBRARY_VERSION_TLS_1_1;
> > +    else if (SECU_GetVersionIDFromName(maxStr, &vr->max) != SECSuccess)
> > +        return SECFailure;
> > +    
> > +    return SECSuccess;
> 
> nit: It would seem good not to introduce new functions with inconsistent
> whitespace.


Ryan, it's not clear to me what inconsistency you see and what change you suggest, can you please clarify? Is it that you don't like "else if" on the same line?

Comment 15

[Ryan Sleevi]

(In reply to Kai Engert (:kaie) from comment #14)
> > @@ +3635,5 @@
> > > +        vr->max = SSL_LIBRARY_VERSION_TLS_1_1;
> > > +    else if (SECU_GetVersionIDFromName(maxStr, &vr->max) != SECSuccess)
> > > +        return SECFailure;
> > > +    
> > > +    return SECSuccess;
> > 
> > nit: It would seem good not to introduce new functions with inconsistent
> > whitespace.
> 
> 
> Ryan, it's not clear to me what inconsistency you see and what change you
> suggest, can you please clarify? Is it that you don't like "else if" on the
> same line?

The inconsistent whitespace is the inclusion of leading spaces on blank lines. At least when I view this in splinter, it's nice and highlighted in red whenever there is extra whitespace after content.

That's what my comment was related to (and it applies in general throughout the patch, but this area was just particularly visible in splinter)

Comment 16

[Elio Maldonado]

(In reply to Ryan Sleevi from comment #12)
> Or use the NSPR routine - PL_strcasecmp
Yes, PL_strcasecmp is the way to go. Built okay and got all.sh to pass in Windows. Thanks

Comment 17

[Kai Engert [:KaiE:]]

Attachment: patch v15

(In reply to Kai Engert (:kaie) from comment #13)
> Can everyone agree with Ryan's proposal?

As nobody objected, let's go with this style, I've made the change and adjusted the usage/help output, and addressed all other change requests, too.

Comment 18

[Kai Engert [:KaiE:]]

Attachment: patch v16

it was also necessary to adjust the test script to use the new parameters that Ryan asked for

Comment 19

[Elio Maldonado]

Comment on attachment 661809 [details] [diff] [review]
patch v16

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

::: mozilla/security/nss/cmd/selfserv/selfserv.c
@@ +1650,3 @@
>      /* do SSL configuration. */
>      rv = SSL_OptionSet(model_sock, SSL_SECURITY,
> +        !(!enabledVersions.min && !enabledVersions.max));

oops, another one that would read better as
(enabledVersions.min || enabledVersions.max));

Comment 20

[Kai Engert [:KaiE:]]

Attachment: patch v17

(In reply to Elio Maldonado from comment #19)
> oops, another one that would read better as

Fixed so that both places use the same test
  (enabledVersions.min > 0 && enabledVersions.max > 0)

Comment 21

[Wan-Teh Chang]

Comment on attachment 662205 [details] [diff] [review]
patch v17

Kai,

I took a quick look at your patch last night. In general
your approach looks good. I'd like to suggest some changes
first.

1. Make SECU_GetVersionIDFromName static, because it is
only used by the SECU_ParseVersionRangeString function.

2. Rename SECU_GetVersionIDFromName to SECU_GetSSLVersionFromName.
It is important to have "SSL" in the function name.

3. Rename SECU_ParseVersionRangeString to
SECU_ParseSSLVersionRangeString. It is important to have
"SSL" in the function name.

4. Add a PRBool *enableSSL2 output argument to
SECU_ParseVersionRangeString, so that the SSLVersionRange *vr
output argument is only used for SSL3 and TLS. (SSLVersionRange
is designed for SSL3 and TLS/DTLS only.) This avoids the need
to have the apiVersions, enabledVersions pair. 

5. In SECU_ParseVersionRangeString, we have:

>+    if (!input || !vr)
>+        return SECFailure;

Add PORT_SetError(SEC_ERROR_INVALID_ARGS).

>+    if (!strcmp(input, ":")) {
>+        /* special value, disable all versions */
>+        vr->min = 0;
>+        vr->max = 0;
>+        return SECSuccess;
>+    }

I would interpret ":" to mean "enable all versions", because
omitting min and max should mean there are no min and max.

6. In secutil.h, we have:

>+ * By default NSS tools enable all protocol versions.

This statement is too general -- NSS tools also include
certutil, vfyserv, etc. Please remove it, and then change
the following:

>+ * The following syntax is used to disable versions:

  s/disable/enable
or
  s/disable versions/specify the enabled protocol version

>+ * and all implemented versions including und larger than min will be enabled.

including and smaller than => less than or equal to
including und larger than => greater than or equal to

7. In selfserv.c, we have:

>+"-V [min]:[max] restricts the set of enabled SSL/TLS protocols versions.\n"
>+"   Possible values for min/max: ssl2.0 ssl3.0 tls1.0 tls1.1\n"
>+"   Example: \"-V ssl3.0:\" enables SSL 3 and newer.\n"

You use lower case version strings in the usage message,
but in the test scripts you use upper case version strings.
Although version strings are case-insensitive, we should
standardize on either upper case or lower case in our own
usage.

8. I suggest shortening some of the version strings:
  ssl2.0 => ssl2
  ssl3.0 => ssl3
  tls1.0 => tls1
This more terse style is also taken in Apache:
http://httpd.apache.org/docs/2.2/mod/mod_ssl.html#sslprotocol

and in the openssl command-line tool (e.g., the s_client comnmand):

	BIO_printf(bio_err," -ssl2         - just use SSLv2\n");
	BIO_printf(bio_err," -ssl3         - just use SSLv3\n");
	BIO_printf(bio_err," -tls1_2       - just use TLSv1.2\n");
	BIO_printf(bio_err," -tls1_1       - just use TLSv1.1\n");
	BIO_printf(bio_err," -tls1         - just use TLSv1\n");
	BIO_printf(bio_err," -dtls1        - just use DTLSv1\n");    
	BIO_printf(bio_err," -mtu          - set the link layer MTU\n");
	BIO_printf(bio_err," -no_tls1_2/-no_tls1_1/-no_tls1/-no_ssl3/-no_ssl2 - turn off that protocol\n");

The only downside is that tls1 could be misinterpreted to
mean "All TLS 1.x versions", but I think that's a small
risk.

Comment 22

[Wan-Teh Chang]

Comment on attachment 662205 [details] [diff] [review]
patch v17

Two more comments.

1. If the SSL tools (selfserv, strsclnt, and tstclnt) enable
all protocol versions by default, that fact should be documented
in the usage message of the individual tools, rather than in the
comment for the SECU_ParseVersionRangeString function.  The
SECU_ParseVersionRangeString function should be documented in
a "tool-neutral" way.

2. Re: upper case vs. lower case version strings: in the
command line, I think using lower case is more convenient
and seems more common. But I don't have a strong preference,
as long as we are consistent in our own usage.

Comment 23

[Kai Engert [:KaiE:]]

Agreement made in the conf call:

We'll use the 4 strings:
- ssl2
- ssl3
- tls1.0
- tls1.1

All tests shall consistently use the lowercase variants, but the tools will accept uppercase, too.

Comment 24

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #21)
> >+    if (!strcmp(input, ":")) {
> >+        /* special value, disable all versions */
> >+        vr->min = 0;
> >+        vr->max = 0;
> >+        return SECSuccess;
> >+    }

I proposed this meaning in order to keep backwards compatibility, because
the old style parameters made it possible to disable all the protocol versions.

But I don't mind taking away this ability.

Comment 25

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #21)

I addressed your change requests:
  1, 2, 3, 4, 5, 6, 7

I changed the meaning of "-V :" in the way you prefer it, thereby removing the ability to disable all protocol versions.

I adjusted the help messages and the input for tests accordingly.

Regarding your request number 8, as I said in comment 23, we had discussed this detail in the NSS call, and a majority was in favor of that style, so I diverged from your request, in accordance with the majority decision.


(In reply to Wan-Teh Chang from comment #22)
> 
> 1. If the SSL tools (selfserv, strsclnt, and tstclnt) enable
> all protocol versions by default, that fact should be documented
> in the usage message of the individual tools, rather than in the
> comment for the SECU_ParseVersionRangeString function.  The
> SECU_ParseVersionRangeString function should be documented in
> a "tool-neutral" way.

Comment removed, as you had requested in change request (6).

The tools have always behaved in the way of "all protocol versions enabled by default", so I don't see a need to start mentioning. Nevertheless, the new usage output explains the -V parameter as "RESTRICT the set of enabled versions", that could be seen as sufficient hinting about the default behavior.


> 2. Re: upper case vs. lower case version strings: in the
> command line, I think using lower case is more convenient
> and seems more common. But I don't have a strong preference,
> as long as we are consistent in our own usage.

All our internal usage changed to lowercase.

Comment 26

[Kai Engert [:KaiE:]]

Attachment: patch v21

Comment 27

[Wan-Teh Chang]

Comment on attachment 664133 [details] [diff] [review]
patch v21

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

r=wtc. Please make the suggested changes and upload a new patch
before you check this in.

I reviewed the patch carefully, so there are many comments. It
is easier to read my comments in Splinter Review.

::: mozilla/security/nss/cmd/lib/secutil.c
@@ +3562,5 @@
> + * defined by the SSL_LIBRARY_VERSION_* constants,
> + * while accepting a flexible set of case-insensitive identifiers.
> + */
> +static SECStatus
> +SECU_GetSSLVersionFromName(const char *input, PRUint16 *v)

Please rename "v" to "version".

In general one-letter variable names should be avoided.
(They are appropriate as array indexes.)

@@ +3571,5 @@
> +    if (!PL_strcasecmp(input, "ssl2")) {
> +        *v = SSL_LIBRARY_VERSION_2;
> +        return SECSuccess;
> +    }
> +    else if (!PL_strcasecmp(input, "ssl3")) {

Don't use "else" after a return statement6.  All instances of
"else" in this function can be removed.

@@ +3584,5 @@
> +        *v = SSL_LIBRARY_VERSION_TLS_1_1;
> +        return SECSuccess;
> +    }
> +    else
> +        return SECFailure;

Please call PORT_SetError(SEC_ERROR_INVALID_ARGS) before returning
SECFailure. Note that there are two "return SECFailure" statements
in this function.

@@ +3589,5 @@
> +}
> +
> +SECStatus
> +SECU_ParseSSLVersionRangeString(const char *input, SSLVersionRange *vr,
> +                                PRBool *v2)

NOTE: I suggest several changes below that require calling
SSL_VersionRangeGetSupported(). I suggest you declare a

    SSLVersionRange vsupported;

variable at the beginning of this function, and call

    SSL_VersionRangeGetSupported(ssl_variant_stream, &vsupported);

just once to initialize it. Then you'll have vsupported.min and
vsupported.max available to you in the rest of the function.

@@ +3606,5 @@
> +    if (!strcmp(input, ":")) {
> +        /* special value, enable all versions */
> +        *v2 = PR_TRUE;
> +        vr->min = SSL_LIBRARY_VERSION_3_0;
> +        vr->max = SSL_LIBRARY_VERSION_TLS_1_1;

Please replace these two lines with
        SSL_VersionRangeGetSupported(ssl_variant_stream, vr);

This way you don't need to update this code when we add TLS 1.2
support.

@@ +3610,5 @@
> +        vr->max = SSL_LIBRARY_VERSION_TLS_1_1;
> +        return SECSuccess;
> +    }
> +
> +    colonPos = strstr(input, ":");

strchr() is more efficient:
    colonPos = strchr(input, ':');

@@ +3612,5 @@
> +    }
> +
> +    colonPos = strstr(input, ":");
> +    if (!colonPos)
> +        return SECFailure;

Add PORT_SetError(SEC_ERROR_INVALID_ARGS).

@@ +3617,4 @@
>  
> +    colonIndex = colonPos - input;
> +
> +    minCopy = strdup(input);

This function is not freeing the return value of strdup().

We can avoid the strdup() call by passing the string length to
SECU_GetSSLVersionFromName() and using PL_strncasecmp() instead of
PL_strcasecmp().

@@ +3626,5 @@
> +    if (!*minStr) {
> +        *v2 = PR_TRUE;
> +        vr->min = SSL_LIBRARY_VERSION_3_0;
> +    }
> +    else {

Please put the closing curly brace '}' and "else" on the same line:
    } else {

@@ +3627,5 @@
> +        *v2 = PR_TRUE;
> +        vr->min = SSL_LIBRARY_VERSION_3_0;
> +    }
> +    else {
> +        PRUint16 v;

Rename this variable "version".

@@ +3633,5 @@
> +            return SECFailure;
> +
> +        if (v == SSL_LIBRARY_VERSION_2) {
> +            *v2 = PR_TRUE;
> +            vr->min = SSL_LIBRARY_VERSION_3_0;

Replace these two lines with:
    SSLVersionRange vsupported;
    *v2 = PR_TRUE;
    SSL_VersionRangeGetSupported(ssl_variant_stream, &vsupported);
    vr->min = vsupported.min;

This way we don't need to update this code when we remove SSL 3.0
support.

Note that we should also do this in the "missing {min} value" case
above.

@@ +3642,5 @@
> +        }
> +    }
> +
> +    if (!*maxStr) {
> +        vr->max = SSL_LIBRARY_VERSION_TLS_1_1;

Replace this line with the following:
    SSLVersionRange vsupported;
    SSL_VersionRangeGetSupported(ssl_variant_stream, &vsupported);
    vr->max = vsupported.max;

::: mozilla/security/nss/cmd/lib/secutil.h
@@ +372,5 @@
>  SECU_SECItemHexStringToBinary(SECItem* srcdest);
>  
> +/* Parse a version range string, with "min" and "max" version numbers,
> + * separated by colon (":"), and return the result in vr and v2.
> + * 

Nit: splinter seems to show there is an extra space at the end of this line.

@@ +378,5 @@
> + * The following syntax is used to specify the enabled protocol versions:
> + * A string with only a max value is expected as ":{max}",
> + * and all implemented versions less than or equal to max will be enabled.
> + * A string with only a min value is expected as "{min}:",
> + * and all implemented versions greater than and equal to min will be enabled.

"and equal to" => "or equal to"

@@ +379,5 @@
> + * A string with only a max value is expected as ":{max}",
> + * and all implemented versions less than or equal to max will be enabled.
> + * A string with only a min value is expected as "{min}:",
> + * and all implemented versions greater than and equal to min will be enabled.
> + * A string consisting of a colon only means "all versions disabled".

The comment should say "all versions enabled".

@@ +382,5 @@
> + * and all implemented versions greater than and equal to min will be enabled.
> + * A string consisting of a colon only means "all versions disabled".
> + *
> + * Because output parameter type SSLVersionRange doesn't allow to set
> + * version 2 values, we use a separate boolean output parameter.

Add "to return whether SSL 2 is enabled".

@@ +386,5 @@
> + * version 2 values, we use a separate boolean output parameter.
> + */
> +SECStatus
> +SECU_ParseSSLVersionRangeString(const char *input, SSLVersionRange *vr,
> +                                PRBool *v2);

The variable "vr" should be renamed "vrange".

The variable "v2" should be renamed "enableSSL2".

::: mozilla/security/nss/cmd/selfserv/selfserv.c
@@ +5,1 @@
>  /* -r flag is interepreted as follows:

NOTE: many of my comments on selfserv.c also apply to strsclnt.c and tstclnt.c.
Please remember to make the same changes to strsclnt.c and tstclnt.c.

@@ +11,5 @@
>  #include <stdio.h>
>  #include <string.h>
>  
>  #include "secutil.h"
> +#include "basicutil.h"

I don't see any new change to this file that requires including
"basicutil.h". The new SECU_ParseSSLVersionRangeString function
is declared in "secutil.h", not "basicutil.h".

@@ +154,5 @@
> +
> +static void
> +PrintParameterUsage()
> +{
> +    fprintf(stderr, "%s",

This can be replaced with a fputs() call, which is more efficient.
Note that stderr would be the second argument to fputs().

@@ +155,5 @@
> +static void
> +PrintParameterUsage()
> +{
> +    fprintf(stderr, "%s",
> +"-V [min]:[max] restricts the set of enabled SSL/TLS protocols versions.\n"

protocols => protocol

@@ +157,5 @@
> +{
> +    fprintf(stderr, "%s",
> +"-V [min]:[max] restricts the set of enabled SSL/TLS protocols versions.\n"
> +"   Possible values for min/max: ssl2 ssl3 tls1.0 tls1.1\n"
> +"   Example: \"-V ssl3:\" enables SSL 3 and newer.\n"

If all versions are enabled by default, that's also important to document.

@@ +188,5 @@
>  "    of size 16kb to the client, 0 means unlimited (default=0)\n"
>  "-j means measure TCP throughput (for use with -g option)\n"
>  "-C SSLCacheEntries sets the maximum number of entries in the SSL\n" 
>  "    session cache\n"
> +"-c Restrict ciphers (use -H to view the cipher list)\n"

I think the -H option should also be documented on its own, rather than
as part of the -c option.

@@ +203,5 @@
> +static void
> +PrintCipherUsage(const char *progName)
> +{
> +    PrintUsageHeader(progName);
> +    fprintf(stderr, "%s",

Same here: this can be a fputs("...", stderr) call.

@@ +779,5 @@
>  **************************************************************************/
>  
>  PRBool useModelSocket  = PR_FALSE;
> +static SSLVersionRange enabledVersions = {
> +  SSL_LIBRARY_VERSION_3_0, SSL_LIBRARY_VERSION_TLS_1_1

We should avoid hardcoded min and max values. These should be
initialized with SSL_VersionRangeGetSupported(ssl_variant_stream, &enabledVersions).

@@ +781,5 @@
>  PRBool useModelSocket  = PR_FALSE;
> +static SSLVersionRange enabledVersions = {
> +  SSL_LIBRARY_VERSION_3_0, SSL_LIBRARY_VERSION_TLS_1_1
> +};
> +PRBool enabledV2       = PR_TRUE;

Please rename this variable enableSSL2.

@@ +1642,5 @@
>      }
>  
>      /* do SSL configuration. */
>      rv = SSL_OptionSet(model_sock, SSL_SECURITY,
> +        (enabledVersions.min > 0 && enabledVersions.max > 0));

This should be
    enableSSL2 || !SSL3_ALL_VERSIONS_DISABLED(&enabledVersions)

The SSL3_ALL_VERSIONS_DISABLED macro is internal:
http://mxr.mozilla.org/security/ident?i=SSL3_ALL_VERSIONS_DISABLED

So we need to inline it as follows:
    enableSSL2 || enabledVersions.min != 0

@@ +2279,5 @@
>  		savecipher(*cipherSuites);		    
>  	}
> +        protos = 0;
> +        if (enabledVersions.min <= SSL_LIBRARY_VERSION_3_0
> +            && enabledVersions.max >= SSL_LIBRARY_VERSION_3_0) {

Nit: I suggest putting && at the end of the previous line.
Please make the same change to the next "if" statement.

@@ +2280,5 @@
>  	}
> +        protos = 0;
> +        if (enabledVersions.min <= SSL_LIBRARY_VERSION_3_0
> +            && enabledVersions.max >= SSL_LIBRARY_VERSION_3_0) {
> +            protos += SSL_CBP_SSL3;

Let's use |= instead of += to turn on a bit in 'protos'.

@@ +2286,5 @@
> +        if (enabledVersions.min <= SSL_LIBRARY_VERSION_TLS_1_0
> +            && enabledVersions.max >= SSL_LIBRARY_VERSION_TLS_1_0) {
> +            protos += SSL_CBP_TLS1_0;
> +        }
> +        /* TODO: SSL_CBP_TLS1_1 */

Note: the reason we didn't add SSL_CBP_TLS1_1 is that TLS 1.1
has the exact same requirements as TLS 1.0 for SSL Bypass mode.

So I suggest we resolve this TODO as follows:

    /* TLS 1.1 has the same SSL Bypass mode requirements as TLS 1.0 */
    if (enabledVersions.min <= SSL_LIBRARY_VERSION_TLS_1_1 &&
        enabledVersions.max >= SSL_LIBRARY_VERSION_TLS_1_1) {
        protos |= SSL_CBP_TLS1_0;
    }

::: mozilla/security/nss/cmd/tstclnt/tstclnt.c
@@ +567,5 @@
>      PRInt32            filesReady;
>      int                npds;
>      int                override = 0;
> +    SSLVersionRange enabledVersions = {
> +      SSL_LIBRARY_VERSION_3_0, SSL_LIBRARY_VERSION_TLS_1_1

Indent by four spaces.

But this should be initialized with
SSL_VersionRangeGetSupported(ssl_variant_stream, &enabledVersions), so
you should remove the structure initializer here.

@@ +612,5 @@
>         }
>      }
>  
>      optstate = PL_CreateOptState(argc, argv,
> +                                 "23BHOSTV:W:a:c:d:fgh:m:n:op:qr:st:uvw:xz");

"23" and "T" should be removed from this string.

::: mozilla/security/nss/tests/ssl/ssl.sh
@@ +285,5 @@
>        else
>            # Do not enable SSL2 for non-SSL2-specific tests. SSL2 is disabled by
>            # default in libssl but it is enabled by default in tstclnt; we want
>            # to test the libssl default whenever possible.
> +          SSL2_FLAGS=""

I think the "" can be omitted in a shell script.

::: mozilla/security/nss/tests/ssl/sslauth.txt
@@ +30,5 @@
> +  noECC     0       -r_-r_-r     -V_ssl3:ssl3_-n_TestUser_-w_bogus SSL3 Request don't require client auth on 2nd hs (bad password)
> +  noECC     0       -r_-r_-r     -V_ssl3:ssl3_-n_TestUser_-w_nss SSL3 Request don't require client auth on 2nd hs (client auth)
> +  noECC     1       -r_-r_-r_-r  -V_ssl3:ssl3_-w_nss_-n_none     SSL3 Require client auth on 2nd hs (client does not provide auth)
> +  noECC     1       -r_-r_-r_-r  -V_ssl3:ssl3_-n_TestUser_-w_bogus SSL3 Require client auth on 2nd hs (bad password)
> +  noECC     0       -r_-r_-r_-r  -V_ssl3:ssl3_-n_TestUser_-w_nss SSL3 Require client auth on 2nd hs (client auth)

To summarize, the flags are changed as follows:
  -T    =>   -V :ssl3
  -2    =>   -V ssl3:
  -2 -T =>   -V ssl3:ssl3

I think the mapping of old flags to the new -V flags is correct.

::: mozilla/security/nss/tests/ssl/sslstress.txt
@@ +15,5 @@
> +  noECC     0      -u        -V_ssl3:_-c_1000_-C_c_-u            Stress TLS  RC4 128 with MD5 (session ticket)
> +  noECC     0      -z        -V_ssl3:_-c_1000_-C_c_-z            Stress TLS  RC4 128 with MD5 (compression)
> +  noECC     0      -u_-z     -V_ssl3:_-c_1000_-C_c_-u_-z         Stress TLS  RC4 128 with MD5 (session ticket, compression)
> +  noECC     0      -u_-z     -V_ssl3:_-c_1000_-C_c_-u_-z_-g      Stress TLS  RC4 128 with MD5 (session ticket, compression, false start)
> +  SNI       0      -u_-a_Host-sni.Dom -V_tls1.0:_-c_1000_-C_c_-u Stress TLS RC4 128 with MD5 (session ticket, SNI)

This file had the following version flags before:
  -T
  -2
  -2 -3

I agree that -2 -3 should be mapped to -V tls1.0:

Comment 28

[Kai Engert [:KaiE:]]

I run into a linker error with your suggested changes.

Some of the tools use static library linking. By calling SSL_VersionRangeGetSupported from secutil.c, we introduce a dependency from libsectool to libssl.

Affected are multiple tools, e.g. ocspclnt

I was worried it's a circular dependency, but to my surprise the linker is able to resolve it, with one more requirement, though.

Because libssl requires zlib, all tools using libsectool will have to link with zlib, too, I think.

But that's not yet sufficient. We must reorder some of the command lines when linking statically, so that libsectool is followed by libssl.


Wan-Teh, should we really introduce the new dependency and make all these changes, or is it better to avoid the dependency?

Comment 29

[Kai Engert [:KaiE:]]

Ok, here is a different solution to this problem and to avoid the new dependency.

We extend SECU_ParseSSLVersionRangeString with another parameter
  const SSLVersionRange defaultVersionRange

The tools (who link with ssl anyway) can call SSL_VersionRangeGetSupported
and pass the result to SECU_ParseSSLVersionRangeString,
and secutil doesn't need to call into libssl.

Comment 30

[Elio Maldonado]

I have often wondered, why is it that our tools use static linking?

Comment 31

[Kai Engert [:KaiE:]]

Attachment: patch v22

I believe this patch addresses all of your change requests.

Because you asked to document -H explicitly, I realized it's better to keep unused -H for potential future help commands, and have changed it to -Y.

You already gave r+, but I'll wait until after today's conference call.

Comment 32

[Kai Engert [:KaiE:]]

I've checked in the patch.

Let me know if you disagree with anything and I'll make a follow-up patch prior to the release.

Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.123; previous revision: 1.122
done
Checking in cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.48; previous revision: 1.47
done
Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.102; previous revision: 1.101
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.73; previous revision: 1.72
done
Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.72; previous revision: 1.71
done
Checking in tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.112; previous revision: 1.111
done
Checking in tests/ssl/sslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslauth.txt,v  <--  sslauth.txt
new revision: 1.16; previous revision: 1.15
done
Checking in tests/ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.21; previous revision: 1.20
done

Comment 33

[Wan-Teh Chang]

Comment on attachment 665495 [details] [diff] [review]
patch v22

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

Kai,

It is fine to use -Y for printing the cipher suite list.

I suggest a change to the SECU_ParseSSLVersionRangeString
function prototype below.

I won't be able to review this patch further today. Please
make the changes, upload a new version of this patch, and
check this in. I can review your new patch after checkin.

Thanks.

::: mozilla/security/nss/cmd/lib/secutil.h
@@ +395,5 @@
> +SECU_ParseSSLVersionRangeString(const char *input,
> +                                const SSLVersionRange defaultVersionRange,
> +                                const PRBool defaultEnableSSL2,
> +                                SSLVersionRange *vrange,
> +                                PRBool *enableSSL2);

This function prototype seems complicated.

Please revert to the previous prototype of this function, with
these input arguments:
    const char *input,
    SSLVersionRange *vrange,
    PRBool *enableSSL2)
and ignore my suggestion of calling SSL_VersionRangeGetSupported.
For now, if the {min} or {max} value is missing, just
set vr->min to SSL_LIBRARY_VERSION_3_0
or vr->max to SSL_LIBRARY_VERSION_TLS_1_1.

I will help you deal with the libssl static linking problem
later.  Thanks.

Comment 34

[Kai Engert [:KaiE:]]

Given that SECU_ParseSSLVersionRangeString is not a part of the NSS library (not exported in any .def file), but only of the NSS tools, I think the requirements for pretty Apis should be less strict.

I don't like the idea to revert to the previouos prototype, that means I'll have to spend time again to change all the callers. I would like to avoid this work.

Here is an alternative proposal to avoid the new parameters.

We could define that vrange enableSSL2 are both input and output parameters, and that it's the callers responsibility to init the parameters with the default values desired by the caller.

Do you like that proposal?

Comment 35

[Kai Engert [:KaiE:]]

Attachment: incremental patch 23

Wan-Teh, this simplifies the API.