367037 - Strsclnt doesn't inform user when incorrect cipher set, just prints usage info.

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Slavomir Katuscak]

Start selfserv:
selfserv -D -p 8443 -d /share/builds/mccrel3/security/securitytip/builds/20070111.1/wozzeck_Solaris8/mozilla/tests_results/security/mace.5/server_memleak -n mace.red.iplanet.com -e mace.red.iplanet.com-ec -w nss -c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014cdefgijklmnvyz -t 5 

Test strsclnt with correct cipher:
strsclnt -q -p 8443 -d /share/builds/mccrel3/security/securitytip/builds/20070111.1/wozzeck_Solaris8/mozilla/tests_results/security/mace.5/client_memleak -w nss -c 1000 -C y mace.red.iplanet.com
strsclnt: -- SSL: Server Certificate Validated.
strsclnt: 0 cache hits; 1 cache misses, 0 cache not reusable
strsclnt: 999 cache hits; 1 cache misses, 0 cache not reusable

Test strsclnt with incorrect cipher:
strsclnt -q -p 8443 -d /share/builds/mccrel3/security/securitytip/builds/20070111.1/wozzeck_Solaris8/mozilla/tests_results/security/mace.5/client_memleak -w nss -c 1000 -C z mace.red.iplanet.com
Usage: strsclnt [-n nickname] [-p port] [-d dbdir] [-c connections]
          [-23BDNTovqs] [-f filename] [-N | -P percentage]
          [-w dbpasswd] [-C cipher(s)] [-t threads] hostname
 where -v means verbose
       -o flag is interpreted as follows:
          1 -o   means override the result of server certificate validation.
          2 -o's mean skip server certificate validation altogether.
       -D means no TCP delays
       -q means quit when server gone (timeout rather than retry forever)
       -s means disable SSL socket locking
       -N means no session reuse
       -P means do a specified percentage of full handshakes (0-100)
       -2 means disable SSL2
       -3 means disable SSL3
       -T means disable TLS
       -U means enable throttling up threads
       -B bypasses the PKCS11 layer for SSL encryption and MACing

Info about cipher z from sslcov.txt:
  noECC  noTLS   z    SSL3_RSA_WITH_NULL_SHA
  noECC   TLS    z    TLS_RSA_WITH_NULL_SHA

This cipher isn't normally tested as part of ssl tests. I don't know the reason why it is there, but my opinion is that there should be some error message informing that incorrect cipher is set, instead of usage page.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The cause of this bug is also the cause of bug 367133.
The fix to this bug will also fix bug 367133,

strsclnt should NEVER call the Usage() function after initializing NSS.
The Usage() function calls exit() without shutting down NSS and NSPR first.

The fix for this bug will be to change function client_main so that it
never calls Usage() or exit(), but instead to print a meaningful error 
message (one that identifies the specific bad cipher suite), set the 
global boolean failed_already to true, and returns.  Then the code will
continue to shutdown NSS and NSPR, plugging the leaks in bug 367133.

We should make sure this bug is fixed before we fix bug 370957.
This bug blocks bug 370957.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - improve error msgs for bad cipher values

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 255742 [details] [diff] [review]
patch v1 - improve error msgs for bad cipher values

Found one bug in my patch.
The format string "(0x%04)"
should be "(0x%04x)".  
I'll fix that before checkin.

Comment 4

[Neil Williams]

Comment on attachment 255742 [details] [diff] [review]
patch v1 - improve error msgs for bad cipher values

That's better.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Please reviwe this week.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Alexei, please review ASAP for 3.11.7

Comment 9

[Alexei Volkov]

Comment on attachment 255742 [details] [diff] [review]
patch v1 - improve error msgs for bad cipher values

r=alexei.volkov

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Checking in strsclnt.c;  new revision: 1.54; previous revision: 1.53

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

committed on 3.11 branch,
Checking in strsclnt.c; new revision: 1.43.2.8; previous revision: 1.43.2.7