402058 - Different return values for the same problem in FIPS and non-FIPS mode.

Status: REOPENED

(NSS :: Test, defect, P5)

Description

[Slavomir Katuscak]

Test:
Selfserv set to require authentication on 2nd handshake (4x -r flag).
Tstclnt trying to connect to selfserv using wrong password (negative test expected to fail).

When client DB used by tstclnt is in FIPS mode, return value is 254, in non-FIPS mode it's 1. For the same tests when selfserv is set to require authentication on 1st handshake, return values are 254 in both cases.

Tests for FIPS mode are not part of standard nightly tests. I found this issue while working on those tests, they are failing now, because expected return value in sslauth.txt (list of auth tests) is set to 1. 

Workaround for ssl testing script (bug 401001) is to change both expected value and return value to the same non-zero value if they are not 0.

Comment 1

[glen beasley]

Attachment: change error code to 254 to indicate 2nd SSL handshake failed

Bug 86528 back in 2001 modified tstclnt to return "0" for success, "1" for almost all errors, and "254" if the SSL handshake fails envoked by first read/write to server. 
In this test selfserv is configured with 4 -r's which mean request  and require, cert on second handshake.
The value 254 is needed after the second read/write that tstclnt performs. 
 
When in FIPS mode tstclnt will always fail on the first handshake.
FIP mode:
tstclnt:
....
Incorrect password/PIN entered.
tstclnt: write to SSL socket failed: Peer's certificate has an invalid signature.
tstclnt: exiting with return code 254
and selfServ reports:
selfserv: HDX PR_Read returned error -12271:
SSL peer cannot verify your certificate

since tstclnt has not authenticated to NSS cryptographic module no 
crypto operations are allowed to be performed. Therefore when tstclnt
attempts to validate the signature of selfserv's server certificate the operation fails on the first handshake, even though we tried to test test
failure on the second handshake. This is valid.

In Non-FIPS mode tstclnt is allowed to perform cryptographic operations with public keys, but it is not allowed to access it's own private key since it has not authenticated to NSS cryptographic module. Therefore while the first hand shake completes the second fails.

tstclnt:
Incorrect password/PIN entered.
....
tstclnt: read from socket failed: SSL peer cannot verify your certificate.
tstclnt: exiting with return code 254  <--- after patch
and selfServ reports:
selfserv: SSL_ForceHandshake returned error -12285:
Unable to find the certificate or key necessary for authentication.

Comment 2

[glen beasley]

removed from FIPS tracking bug, and set to TEST component

Comment 3

[glen beasley]

Attachment: set the expected return value for ssl.sh tests

forgot to modify ssl.sh and sslauth.txt in the first patch.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Before proceding with this patch, we need a clear statement of definition 
of error 254.  
What exactly does it mean?  
Under what circumstances, exactly is it supposed to be returned?  
A reviewer should be able to determine if it meets the definition or not,
and can't do that without a definition.  

A Statement to the effect that "this test also needs to return error 254"
is somewhat unjustifiable without such a definition.  

Please define the meaning of that return value.

Comment 5

[glen beasley]

Attachment: define expected error for tstclnt run by ssl.sh/sslauth.txt

Thanks Nelson. 

tstclnt returns the error code 254 to distinguish to ssl.sh/sslauth.txt
known expected error for negative test case.

1. An NSS test program for use by NSS testing shell scripts
    must exit with a code in the range of 0-255.
2. The convention is that an exit code of 0 means success
   and a nonzero exit code means failure.  Unless we need
   to distinguish between different failures, we should
   use the exit code 1 for failures.
3. tstclnt uses the error code 254 to distinguish to ssl.sh/sslauth.txt
   that the error is expected for the test run.

currently the only negative tests run by ssl.sh/sslauth.txt is

1) attempt to initialize NSS with a bad password and ensure that
   tstclnt is then not able to perform client auth.
2) initiialize NSS with a good password but provide an invalid certificate
   nickname and ensure that tstclnt is then not able to perform client auth

these tests are run in FIPS mode and non FIPS mode we want tstclnt to return the same general expected error in both modes even though there are different expected errors.

If tstclnt is in FIPS mode and provides a bad password the expected error is SEC_ERROR_BAD_SIGNATURE due to tstclnt will not be able to validate the server.

if tstclnt attempts to use a invalid certificate with good password in FIPS MODE
the expected error is SSL_ERROR_BAD_CERT_ALERT. In non FIPS mode the expected error is SSL_ERROR_BAD_CERT_ALERT for both bad password or bad cert nickname.

Comment 6

[glen beasley]

I meant EXPECTED_ERROR_SSL_H to be EXPECTED_ERROR_SSL_SH

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The present tstclnt code (without any patch from this bug) returns 254 for 
ANY failure (with any error code) to ANY PR_Send operation on the SSL socket
at any time.  Presently error 254 means "Send failed", and nothing more.
It means that in all tests with all combinations of command line options.

Your patch would change that as follows:
In any and all test runs of tstclnt, ANY PR_Send operation on the SSL socket that fails with either SSL_ERROR_BAD_CERT_ALERT or SEC_ERROR_BAD_SIGNATURE would cause tstclnt to return 254, and 

In any and all test runs of testclnt, any PR_Recv operation on the SSL socket 
that fails with SSL_ERROR_BAD_CERT_ALERT would cause tstclnt to return 254.

Any other cause of failure would cause tstclnt to return 1.

I can live with those definitions.  

Have you tested this patch on Windows as well as on some Unix-ish platform?

Comment 8

[glen beasley]

Comment on attachment 369718 [details] [diff] [review]
define expected error for tstclnt run by ssl.sh/sslauth.txt

this patch is not ready for review.

Comment 9

[glen beasley]

Attachment: modify tstclnt.c to return 254 for certain errors

This patch modifies tstclnt in the following manner:

In any and all test runs of testclnt ANY PR_Send operation on the SSL socket that fails with either SSL_ERROR_BAD_CERT_ALERT, SSL_ERROR_REVOKED_CERT_ALERT, or SEC_ERROR_BAD_SIGNATURE would cause tstclnt to return 254

In any and all test runs of testclnt, any PR_Recv operation on the SSL socket
that fails with SSL_ERROR_BAD_CERT_ALERT or SSL_ERROR_REVOKED_CERT_ALERT would cause tstclnt to return 254.

Any other cause of failure would cause tstclnt to return 1.

This patch modifies ssl.sh/sslauth.txt in the following manner:

ssl.sh/sslauth.txt (envoked by all.sh) will check for the expected error value 254 for current negative test runs.

This current patch has been tested on Open Solaris 32/64bit.
Mac OS X 32/64 bit, Ubuntu 64 bit, Windows Vista 32 bit.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370523 [details] [diff] [review]
modify tstclnt.c to return 254 for  certain errors

Thank you, Glen, for that excellent patch description in Comment #9.

r+=nelson for NSS 3.12.4.  Please wait until the tree is open for 
checkings for 3.12.4 before committing this patch.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Glen, please commit this patch at your soonest convenience.

Comment 12

[glen beasley]

Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.55; previous revision: 1.54
done
Checking in tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.101; previous revision: 1.100
done
Checking in tests/ssl/sslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslauth.txt,v  <--  sslauth.txt
new revision: 1.8; previous revision: 1.7
done

Comment 13

[Slavomir Katuscak]

Latest patch causes some failures like:
ssl.sh: #2162: TLS Require client auth on 2nd hs (client does not provide auth). Client params: -w nss -n none produced a returncode of 1, expected is 254 - FAILED

Tinderboxes are orange again.

Comment 14

[glen beasley]

Attachment: ssl pkix test

when I run all.sh locally my test run passes because my test run of all.sh is not running all same tinderbox runs. tinderbox all.sh is running extra SSL pkix tests. How do I run these SSL pkix as well?

I believe this patch will make tinderbox to go green... but I haven't tested locally.

Comment 15

[Julien Pierre]

Glen,

I don't think tinderbox will go green until there is some resolution for bug 412468. See comments 49 - 58 .

Comment 16

[glen beasley]

(In reply to comment #15)
> Glen,
> 
> I don't think tinderbox will go green until there is some resolution for bug
> 412468. See comments 49 - 58 .

While the tinderbox is orange due to bug 412468, I believe tinderbox is also orange due to this bug.

I was mistaken when I though this issue was due to new pkix tests.
When I tested I did not run the ssl interoperability tests where our tstclnt talks to a running web server. One needs to set IOPR_HOSTADDR_LIST=<activeserver:port> before running all.sh

I am now running the interoperability tests and working on a patch for tstclnt.

Comment 17

[Julien Pierre]

Glen,

You are right, the tinderboxes have been orange due to both bug 412468 and this bug. Hopefully the former problem has been fixed now.

Comment 18

[glen beasley]

Attachment: backout of patch

I am backing out this patch. should of did it immediately apologize to anyone watching tinderbox. 

Checking in cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.57; previous revision: 1.56
done
Checking in tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.102; previous revision: 1.101
done
Checking in tests/ssl/sslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslauth.txt,v  <--  sslauth.txt
new revision: 1.9; previous revision: 1.8
done

I need to get the correct settings figured out for a complete tinderbox run.

Comment 19

[Julien Pierre]

Thanks, Glen. Unfortunately, there is still a crash in PKITS so it won't go green yet :-(