Bug 50570 - Allow explicit use of FIPS mode in APR lifecycle listener
Summary: Allow explicit use of FIPS mode in APR lifecycle listener
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Connectors (show other bugs)
Version: unspecified
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2011-01-11 12:27 UTC by Christopher Schultz
Modified: 2014-02-17 13:52 UTC (History)
3 users (show)



Attachments
Source and properties files to add FIPS initialization call to OpenSSL (20.51 KB, application/zip)
2011-01-31 16:59 UTC, Chris Beckey
Details
Patch to implement FIPS mode setting in Tomcat 7 trunk (28.37 KB, patch)
2011-03-15 16:16 UTC, Chris Beckey
Details | Diff
patch to implement FIPS mode setting in tc native (496 bytes, patch)
2011-03-15 16:17 UTC, Chris Beckey
Details | Diff
Somewhat simpler patch for FIPS mode (against TC7 trunk) (6.68 KB, patch)
2011-06-28 20:40 UTC, Christopher Schultz
Details | Diff
A fipsModeSet implementation that is more robust than the original patch (1.15 KB, patch)
2011-06-28 21:37 UTC, Christopher Schultz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Schultz 2011-01-11 12:27:44 UTC
Both OpenSSL and JSSE allow themselves to be put into FIPS mode, and we should allow SSL connectors to request it (and have them fail if FIPS mode can't be set).

OpenSSL:
http://www.mail-archive.com/openssl-dev@openssl.org/msg20882.html
Comment 1 Christopher Schultz 2011-01-11 12:34:15 UTC
It might make more sense to put this into the AprLifecycleListener, as it appears that FIPS mode for OpenSSL is not done on a per-context basis, but per-process.

It would be nice to enable this for JSSE as well, so where would that go?
Comment 2 Mark Thomas 2011-01-11 17:04:30 UTC
Unless its changed since I last looked at it, a FIPS JSSE solution requires a new security provider and all the configuration happens in the JRE. Tomcat configuration does not change.
Comment 3 Christopher Schultz 2011-01-13 14:49:06 UTC
Changing description to reflect new requirements: NIO/BIO connectors need no such explicit configuration.

It looks like this can be done by adding a member, accessor, and mutator to AprEndpoint and then in AprEndpoint.bind, call (a new method) SSLContext.setFIPSMode which calls FIPS_mode_set.

One thing I'm unclear on is how to get the configuration from server.xml's <Connector> all the way down to the AprEndpoint object. What object has setXXX() called on it when configuring a <Connector>?
Comment 4 Chris Beckey 2011-01-31 16:59:41 UTC
Created attachment 26582 [details]
Source and properties files to add FIPS initialization call to OpenSSL

The attached zip file contains source to allow FIPS mode initialization of OpenSSL.
Comment 5 jfclere 2011-02-01 03:38:39 UTC
A patch (like diff against the existing code) would be more easy to look to,
Comment 6 Chris Beckey 2011-02-01 12:44:28 UTC
(In reply to comment #5)
> A patch (like diff against the existing code) would be more easy to look to,

The code is built from a base of Version 6.0.20.  I had assumed that a diff against the (current) 7.x codebase would be nearly worthless.  If a diff from 6.0.20 would be useful I can generate that.
Comment 7 Christopher Schultz 2011-02-01 17:51:52 UTC
(In reply to comment #6)
> The code is built from a base of Version 6.0.20.  I had assumed that a diff
> against the (current) 7.x codebase would be nearly worthless.  If a diff from
> 6.0.20 would be useful I can generate that.

A diff against 6.0.20 would be better than just a big ZIP file: we'd have to do our own diffs and figure it all out, anyway, so you'd be saving us a big step.

It might be best to give us two patches: one for the Java code and another for the C code.
Comment 8 Christopher Schultz 2011-02-01 17:53:33 UTC
(From private email from CHris Beckey):

> I just zip'd together the source and attached it to the bug report.  The changes are in:
> 
> org.apache.tomcat.core.AprLifecycleListener.java
> org.apache.tomcat.core.LocalStrings.properties
> org.apache.tomcat.jni.SSL.java
> org.apache.tomcat.jni.Library.java
> 
> and in tomcat native
> ssl.c
> 
> I modified the code in Library.java to load each of the (3) required libraries (APR, libtcnative and libeay) explicitly rather than depending on references within the libs.  This was for my debugging and is not required but it does make it explicit if one of them is missing.
> 
> The listener declaration in server.xml looks like this:
>   <Listener className="org.apache.catalina.core.AprLifecycleListener" SSLEngine="on" FIPSMode="on" />
> 
> I was unsure whether to abort startup if FIPS was requested but did not initialize. I think it is valid to refuse to continue in that case but didn't implement it that way.
Comment 9 Christopher Schultz 2011-02-01 17:54:18 UTC
Updating bug description to match reality: FIPS mode should be set in the APR listener, and not in an individual Connector.
Comment 10 Chris Beckey 2011-03-15 16:16:28 UTC
Created attachment 26775 [details]
Patch to implement FIPS mode setting in Tomcat 7 trunk

Requires TC native patch, which has also been attached to bug #50570
Comment 11 Chris Beckey 2011-03-15 16:17:55 UTC
Created attachment 26776 [details]
patch to implement FIPS mode setting in tc native 

Requires patch to Tomcat, which is attached to bug #05070
Comment 12 Chris Beckey 2011-03-15 16:54:37 UTC
(In reply to comment #9)
> Updating bug description to match reality: FIPS mode should be set in the APR
> listener, and not in an individual Connector.
Comment 13 Chris Beckey 2011-03-15 17:13:13 UTC
The first attachment (named "Source and properties files to add FIPS ...") contains source code and properties using TC 6.0.20 as the base.
The next two attached files (named "patch to implement ...") are patches against the trunk as of the 14th of March 2011, i.e. after version 7.0.11.
These were tested against OpenSSL V 0.9.8q with the OpenSSL FIPS module 1.2.2 built and linked in using the directions at 
http://www.openssl.org/docs/fips/UserGuide-1.2.pdf
Comment 14 Christopher Schultz 2011-06-24 14:47:13 UTC
Just a quick update: I'm taking a vacation from $work next week and I hope to work on this feature. Chris, if there are any updates to your patch please post them. Otherwise, I'll use the existing one.
Comment 15 Christopher Schultz 2011-06-28 20:40:40 UTC
Created attachment 27224 [details]
Somewhat simpler patch for FIPS mode (against TC7 trunk)

I'm not quite done with my adaptation, but I figured I would get some feedback on this one in the meantime.
Comment 16 Christopher Schultz 2011-06-28 21:37:31 UTC
Created attachment 27225 [details]
A fipsModeSet implementation that is more robust than the original patch

* Provide a fipsModeSet function when OPENSSL is not available.
* Provide a fipsModeSet implementation that fails gracefully when FIPS is not available.
* Provide a human-readable error message when failing to enter FIPS mode.
Comment 17 Christopher Schultz 2011-06-28 21:44:38 UTC
Comment on attachment 26582 [details]
Source and properties files to add FIPS initialization call to OpenSSL

Replaced with non-zipped patches.
Comment 18 Konstantin Kolinko 2011-07-05 21:57:06 UTC
(In reply to comment #16)

Regarding attachment 27225 [details] (Java patch) I have two comments:

1) Code formatting.
We usually have "{" on the same line as previous statement (like in Java coding style).

2) I go not get why there are two methods in SSL.java:
fipsModeSet(Integer mode) and fipsModeSet(int mode).

If you need class object of "int" to do lookup of the method, it is available as Integer.TYPE.
Comment 19 Christopher Schultz 2011-07-05 22:54:15 UTC
Thanks for the comments. Very helpful!

(In reply to comment #18)
> (In reply to comment #16)
> 
> Regarding attachment 27225 [details] (Java patch) I have two comments:
> 
> 1) Code formatting.
> We usually have "{" on the same line as previous statement (like in Java coding
> style).

I'm happy to change the use of whitespace, but "usually" is somewhat subjective in the TC code base :)

> 2) I go not get why there are two methods in SSL.java:
> fipsModeSet(Integer mode) and fipsModeSet(int mode).
> 
> If you need class object of "int" to do lookup of the method, it is available
> as Integer.TYPE.

I mostly reduced the patch provided by (the other) Chris and I remember thinking about the overloaded methods but can't remember why I decided to leave them there. Certainly the rest of the code does not require both methods.

Come to think of it, I'm not sure why the method call needs to be done using reflection at all... both AprLifecycleListener.java and SSL.java are in the Tomcat project (as opposed to tcnative) and thus should never be out of sync.

The only potential problem is if the native call fails because of a mismatched tcnative library, but that would fail in a way that reflection wouldn't solve, anyway.

I'll update the patch.
Comment 20 Mark Thomas 2011-10-26 09:33:44 UTC
Chris, did you put together an updated patch for this?
Comment 21 Christopher Schultz 2011-10-26 18:44:18 UTC
(In reply to comment #20)
> Chris, did you put together an updated patch for this?

I have a patch sitting in my local svn working copy of TC 8 trunk: I haven't gotten a chance to test it, yet (building openssl-FIPS+apr proved to be a pain in the past, but I think I've got it working). I haven't yet made the changes suggested by Konstantin.

I was a little concerned about the dependency on tcnative 1.1.23 which isn't yet released: 1.1.23 will have the native FIPS code required to make this Java patch work.

Shall I do tcnative-version-sniffing and bail if the version isn't high enough, or just let a native-method-not-found exception be thrown?
Comment 22 Mark Thomas 2011-10-26 19:09:37 UTC
Since this is security related, requesting FIPS and the native library not supporting it should prevent Tomcat from starting. You probably want to trap the exception and throw a new one with a nicer error message. I haven't looked at the code to see if anything else would be required.
Comment 23 Christopher Schultz 2011-11-09 21:54:01 UTC
Fixed in trunk, will be in 7.0.23 and later.
Proposed for 6.0.x.
Comment 24 Konstantin Kolinko 2011-12-25 19:23:18 UTC
Fixed in 6.0 by r1224628 and will be in 6.0.36.