Patch for FIPS 140 mode - take 3

[Steve Marquess]

Roumen Petrov wrote:

> Hi All,
>
> Steve, patch look very nice and simple.

Thanks for taking the time to look it over.  I write it knowing there 
would be
room for improvement, hoping to stimulate discussion by those more familiar
with OpenSSH than I.  All of your suggestions look good to me; more detailed
comments follow.

> My suggestions about patch are as follows:
>
> 1.) What about to allow fips mode to be set in config files or with -o 
> command line option ?

Works for me.  Usually there would be no no reason to run sshd in a 
non-FIPS mode
if you need FIPS mode at all, but it wouldn't hurt.

>
> 2.) File fips.h is only one line.
> Instead of '#include "fips.h"' we can put 'extern int fips_mode;' in 
> mac.c, cipher.c and etc.
>
> 3.) Where is best plase to put variable fips_mode ? Might is better to 
> put it in cipher.c ?

I wasn't sure of the cleanest way to implement fips_mode, so just went 
for the obvious.

> 4.1.) What about when config files or command line explicitly set macs 
> option and FIPS is disabled from command line or config file{s} ?
> Sample: ssh{d} ... -o macs=... -y ...
> I cannot agree with changes in myproposal.h/mac.c/readconf.c/servconf.c.
> Steve, in ssh.c/sshd.c/ you must check allowed macs after 
> fill_default_[server_]options.
> Might is better to check at end of the fill methods.
> Only in fips mode when option.macs is NULL you musts set explicitly to 
> "hmac-sha1,hmac-sha1-96".
>
> 4.2)  Same as 5.1. for ciphers.
>
> Proposed solution is to add fips_mode to option structures(readconf.h 
> rservconf.h).
> a.) Initialize it as to -1.
> b.) Use lines like following
> #ifdef OPENSSL_FIPS
> { "fipsmode", {s|o}FipsMode }
> #else
> { "fipsmode", {s|o}Unsupported }
> #endif
> c.) in fill_default .... methods
>    if (options->fipsmode == -1)
> #ifdef OPENSSL_FIPS
>        options->fipsmode = 1;
> #else
>        options->fipsmode = 0;
> #endif
>    fipsmode = options->fipsmode;
> d.) at end of fill... methods to validate macs and ciphers and to set 
> them explicitly to non-NULL only in fipsmode.

Looks like a cleaner approach.

> 5.) What hapen with key fingerprints when MD5 is disabled ?

MD5 is a problem; NIST doesn't like it.  MD5 can be used only where 
plain text would be
allowable.  It is permitted in TLS, for example, because SHA1 is used in 
addition to MD5.
The passphrases for keys can't be used in FIPS mode, for example.

The simple generation and display of MD5 fingerprints isn't necessarily 
forbidden, but the
_use_ of the result for any security related function is.

Ideally I'd like to see a SHA1 option for passphrases and fingerprints.  
That's a big and
intrusive mod worthy of more discussion.  Any chance it could happen?

> 6.) Should ssh-keyscan be FIPS 140 aware and when yes what's happen 
> when server has rsa1 key ?

Probably, as should ssh-add and ssh-keygen.  I was being lazy there, 
plus my immediate
requirement is for ssh and sshd only.

> 7.) Might in FIPS mode ssh protocol version 1 must be always disabled ?
> Please see sshd.c and servconf.c.

Well, I considered protocol version 2 only because version 1 is 
essentially forbidden
for government use, i.e. wherever you would care about FIPS mode.

> 8.) Should we clear datafellows flag SSH_BUG_RSASIGMD5 in method 
> compat_datafellows from compat.c or when flag is set should we accept 
> connection ?
> Please see ssh-rsa.c.

Over my head, I'll have to study this one.

> 9.) What about to use configure option --with-ssl-static(fips build 
> request must set implicitly static_crypto_lib="yes") ?
> As example in configure.ac we can use (note following lines are writen 
> from scratch):
> =================================================
> ....
> #request explicit link with static crypto lib.
> static_crypto_lib="no"
> AC_ARG_WITH(ssl-static,
>    [  --with-ssl-static         <FIXME: appropriate message>],
>    [
>        if test "x$withval" != "xno" ; then              
> static_crypto_lib="yes"
>        fi
>    ]
> )
> ....
> if test "x$ac_cv_fips" = "xyes" ; then
>    static_crypto_lib="yes"
> fi
> if  "x$static_crypto_lib="xyes"; then
> case "$host" in
> *-*-hpux11*)      LIBS=`echo $LIBS | sed 's/-lcrypto /-Wl,-Bstatic 
> -lcrypto -Wl,-Bdynamic /'`
>    ;;
> *-*-linux*)
>    LIBS=`echo $LIBS | sed 's/-lcrypto /-Wl,-aarchive -lcrypto 
> -Wl,-adefault /'`
>    ;;
> *)
>    AC_MSG_ERROR([FIXME: crypto lib static linking])
> esac
> fi
> ....
> =================================================

Yep, looks cleaner.

> 10.) At end of configure is good to show that FIPS is enabled.

Agreed!

Guys, please let me know if you'd like me to work some more on cleaning 
up my
patch, or if someone else would like to take this and run with it.

Thanks,

-Steve M.