Bug 588323

While addressing another issue I realized this is actually the same problem. I have patches prepared to better handle ECC cipher preference setting on a JSS Socket. But my proposed fix still includes emitting a message. Here is the issue:

The socket cipher list (specified in server.xml) includes ECC ciphers. By default NSS is not built to support ECC, thus if you try and set an ECC cipher NSS reports an error which then generates the error message.

However ECC support can be optionally loaded into NSS via a pkcs11 module, if that is the case setting an ECC cipher preference should be considered an error. However, it's not easy to determine if NSS should be supporting ECC or not.

My proposed solution is to special case ECC ciphers as opposed to other ciphers. If the cipher preference fails and is an ECC cipher a warning message will be emitted instead of an error with the qualification that this is only a problem if you expect ECC support is enabled. My feeling is we still need to emit a message for ECC cipher failures because it's possible ECC is supposed to be enabled. Non-ECC cipher failures will continue to produce an error message.

This is an example of the warning message:

Warning: SSL ECC cipher TLS_ECDH_ECDSA_WITH_NULL_SHA unsupported by NSS. This is probably O.K. unless ECC support has been installed.

Are you O.K. with the proposed solution?

Created attachment 460660 [details]
Fix cipher preference error handling/reporting

The major purpose of this patch is correct the error reporting related
to SSL ciphers. While addressing that some other clean up and
robustness fixes were introduced.

Errors of this type were showing up in catalina.out:

JSSSocketFactory init - exception thrown:org.mozilla.jss.ssl.SSLSocketException: Failed to disable cipher 0xc005

This would lead one to believe the socket initialization failed, when
in fact it didn't.

Background: ECC support in NSS has been disabled by default. ECC
support in NSS can be optionally enabled via a loadable pkcs11
module. We don't know if NSS has been configured to support ECC or
not. If a ECC cipher preference has been specified in the tomcat
connectors cipher list (which is often the case) and we try set that
cipher preference in NSS then JSS will throw an execption due to the
unknown cipher.

The reason why these exceptions had not been causing run time problems
is because the cipher preference setting is the last thing done in the
socket init code and the ECC ciphers have traditionally been the last
ciphers in the cipher list. Thus with the existing code and config
files the exception didn't cause necessary code to be
skipped. However the assumptions about being "last" are dangerous
assumptions and logging execptions leads on to believe there are
actual serious problems when in fact there aren't.

The basic fix to the code was to wrap the cipher preference setting
code in it's own try/catch block and not let the exception percolate
up. If NSS thows an exception we check to see if the cipher is a ECC
cipher, if so we emit a warning with the clarification that this is
probably O.K. unless you've configured NSS to support ECC. For non-ECC
ciphers we emit an error message. In either case the cipher list
continues to be processed until it's exhausted instead of taking a
non-local exit via an exception potentially leaving other ciphers
unprocessed.

The other minor fixes included:

The logic for reading a cipher as a hex value was broken, it didn't
account for a leading + or - flag, it didn't catch exceptions if
parseInt() failed, it didn't specify the radix to ParseInt().

The logic for detecting the enable/disable flag via a leading + or -
was flawed.

The toCipherID() method was poorly implemented. It consisted of 76
string equality tests. Instead it's much more efficient to perform the
lookup via a hash table. The cipher names were put into a statically
initialized hash map and now the cipher string conversion is done via
a hash lookup.

A statically initialized hash table of ECC ciphers was added. The test
to see if a cipher is a ECC cipher is performed by testing if the
cipher is in the table.

There were a number cipher values which had been hardcoded into the
Java source, those are now available in JSS and the local hardcoded
values removed.

The new version of the code now produces this instead:

Warning: SSL ECC cipher "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA" unsupported by NSS. This is probably O.K. unless ECC support has been installed.

This patch also includes one non-cipher fix. The debug log file was
not opened in append mode. Thus if there are more than one connector
the debug log file would be overwritten by every subsequent connector
in the server when the server initialized. This meant the debug log
file contained only the debug information from the last connector
initialized. Now the debug log file is opened in append mode.

Comment on attachment 460660 [details]
Fix cipher preference error handling/reporting

Changes look fine.  Just two suggestions:
1. I did not check the cipher string spellings, so please make sure they are all spelled correctly.
2. If you choose to do append mode in debug log file, please consider adding time stamp and thread id.
Finally, make sure it is well tested.
Thank you!

Created attachment 468867 [details]
Add timestamp and thread name to debug log output

As per the suggestion in the review this adds the a timestamp and thread name to the debug log file. The formatting identically matches the format used in the CS debug log file. It is tested and will be included in the final SVN commit.

re comment #6

The cipher strings were generated using an emacs text editor macro from the original constants as such the spellings are guaranteed to be correct. However, I did verify the spellings using another editor macro, all spellings are correct.

The patch was thoroughly tested prior to review submission.

The suggestion of adding a timestamp and thread name to the debug logging is an excellent suggestion. This was added (see above patch attachment). The formatting exactly matches the existing CS debug file format. It was tested. In the interest of expediency I will commit to SVN the approved patch plus the suggested logging enhancement rather than going through another round of patch review for what is a fairly trivial addition to the original patch.

Sending        src/org/apache/tomcat/util/net/jss/JSSSocketFactory.java
Transmitting file data .
Committed revision 105.

Discovered this same problem exists elsewhere, it's still showing up in the IPA server install because it's coded into the pkisilent code. This is likely the actual root cause of the original problem reported.

In HTTPClient.java are two HTTP constructors, one takes a boolean arg to enable ecc cipher setting.

	public HTTPClient()
	{
		// constructor
		// turn off ecc by default
		ecc_support = true;
	}

	
	public HTTPClient(boolean ecc)
	{
		ecc_support = ecc;
	}


If ecc_support is true then the ecc ciphers are attempted to be set which generates the error message of concern. ECC cipher support is not normally available in default installations so the error message always appears and is alarming. Note, ecc support is enabled by default despite the erroneous comment in the first constructor. 

In ConfigureCA.java ecc support is manipulated thusly:

        if (key_type.equalsIgnoreCase("ecc")) {
            boolean st = true;

            hc = new HTTPClient(st);
        } else {
            hc = new HTTPClient();
        }

The above logic seems to be based on the assumption that the default for ecc support is falue (as per the erroneous comment, I assume it probably was at one point). But the above should should probably have been more robustly coded as:

        if (key_type.equalsIgnoreCase("ecc")) {
            hc = new HTTPClient(true);
        } else {
            hc = new HTTPClient(false);
        }

In the IPA server install the key_type is set to "rsa" so the HTTPClient() constructor without the ecc_support parameter is called which defaults to true which produces the error messages.

Created attachment 471072 [details]
make signing_algorithm arg optional

This patch makes the signing_algorithm optional, defaulting to key_algorithm.

The patch also replaces the hard coded hex ECC cipher enumerated constants with their symbolic name. Those names are now exported by JSS and should be used to be programmer friendly.

Created attachment 471503 [details]
respect ECC cipher configuration

Previous patch incorrectly had more than one bug fix in it. This patch separates the issues and is what is committed.

dogtag commit: 

Sending        src/ca/ConfigureCA.java
Sending        src/http/HTTPClient.java
Transmitting file data ..
Committed revision 1681.

CS 8.1 commit:

Sending        src/ca/ConfigureCA.java
Sending        src/http/HTTPClient.java
Transmitting file data ..
Committed revision 1682.