333559 - strsclnt needs option to disable SSL2 compatible client hellos

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Without the ability to disable SSL2 compatible client hellos, strsclnt 
cannot test TLS hello extensions.  Testing of hello extensions cannot be
done until this bug/RFE is fixed.

tstclnt has a -2 option that disables SSL2 and SSL2 compatible client hellos.
strsclnt has a -2 option that does something else.  This is confusing.

strsclnt's -2 option takes a file name option argument, and sends the content 
of that file, rather than its usual canned GET /abc http request.  
I claim that strsclnt's -2 option is completely unused.  Therefore, I propose
to change strsclnt's -2 option to work the same as tstclnt's -2 option, 
disabling SSL2 and SSL2-compatible client hellos.  I propose to add a new -f
option to take the place of the old -2 option in strsclnt.  

patch forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1, use -2 to disable SSL2 hellos

Please review.
After this is approved, another patch will be needed to fix 
nss/tests/ssl/sslstress.txt to invoke strsclnt with this new option
for some of the tests.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Change stress test script to use new -2 option

These two patches can be considered a set, to be granted or denied together.

Comment 3

[Julien Pierre]

Comment on attachment 218012 [details] [diff] [review]
patch v1, use -2 to disable SSL2 hellos

Two problems with this patch :

1) You did not disable SSL_ENABLE_SSL2 when -2 is passed in, only SSL_V2_COMPATIBLE_HELLO

2) The usage function is incomplete - the short description should include 2 in the list of options like this :
[-23BDNTovqs]

Comment 4

[Julien Pierre]

Comment on attachment 218015 [details] [diff] [review]
Change stress test script to use new -2 option

Nelson,

1) This patch doesn't only add the -2 option, it also divides by 10 the number of interation in the stress test. I don't think we should do that. This stress test has found race conditions. With only 10 or 100 iterations, it might no longer do that. This is the main reason for r-.

2) You only pass the -2 option for 3 of the 5 ECC stress tests - the TLS ones, but not the SSL3 ones. Was that intended and if so, why ?

3) If you want patches reviewed together, it's easier for the reviewer if you do a single cvs diff on multiple directories from a high level, and ask for a single review request.

Comment 5

[Robert Relyea]

Comment on attachment 218012 [details] [diff] [review]
patch v1, use -2 to disable SSL2 hellos

r+ assuming you fix the usage message Julien pointed out.

I concur that the Disable code is correct since disabling SSL_V2_COMPATIBLE_HELLO also disables SSL2 as a side effect. Perhaps a comment would be in order , however.

Comment 6

[Julien Pierre]

Bob,

I didn't know disabling SSL_V2_COMPATIBLE_HELLO automatically disabled SSL_ENABLE_SSL2 . tstclnt already explicitly disables both, however, and as one of the example programs, it would be better for strsclnt to do the same .

Comment 7

[Julien Pierre]

Comment on attachment 218015 [details] [diff] [review]
Change stress test script to use new -2 option

Actually the patch multiplies the number of connections, not divides ;) I don't object to that, but I still don't think that was supposed to be part of the patch for this bug.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I don't understand comment 7 at all.  Please explain.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218015 [details] [diff] [review]
Change stress test script to use new -2 option

You'll notice that in my patch to sslstress.txt, I removed 3 lines of comments

-# Currently, session reuse does not work for ECDH-ECDSA and ECDHE-ECDSA 
-# ciphers (see Bug 238051). Setting up 1000 connections without session
-# reuse would take too long, so use only 10 connections

That comments explains one of the two reasons why the connection counts were
so low:  (a) no server session cache for EC cipher suites, and 
(b) really slow EC code for some curves.  

I removed that comment and increased the number of connections because those
problems have recently been fixed.  
a) Bob checked in a fix for the server session cache for EC cipher suites.  
b) some significant performance gains on some of the EC curves have been 
recently achieved.  

The new cache code explains why the counts were increased to 1000 on tests 
that do session restart (e.g. the tests that do NOT use the -N option).

The improved EC performance explains why the counts were increased to 100 
for the tests that DO use the -N option (do only full handshakes).

libSSL only sends hello extensions when it attempts to negotiate TLS (for
the client) or when it DOES negotiate TLS (for the server).  It does not 
use hello extensions when negotiating SSL 3.0.  That is why the -2 option
is only used with TLS tests, and not with SSL3 tests.  Since the SSL3 
tests never use the TLS hello extensions, there's no reason to suppress 
the v2 compatible hello for the SSL3 tests.

I think this explanation addresses all the concerns expressed about the
script change patch.  I think there are no errors in that patch that need
to be changed.  So I ask you to re-review this patch in that light.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: use -2 to disable SSL2 hellos, v2

Address previous review comments to v1 of this patch

Comment 11

[Julien Pierre]

Comment on attachment 218015 [details] [diff] [review]
Change stress test script to use new -2 option

Nelson,

Yes, I agree with those changes. They only surprised me during the review because you hadn't given any notice that you intended to do it. So it looked like a change that might have accidentally slipped into this patch.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Checked in on trunk

cmd/strsclnt/strsclnt.c; new revision: 1.50; previous revision: 1.49
tests/ssl/sslstress.txt; new revision: 1.9; previous revision: 1.8

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Backported to NSS_3_11_BRANCH
strsclnt/strsclnt.c; new revision: 1.43.2.6; previous revision: 1.43.2.5