Hi Martin, My responses below. Thanks!
regards, Joy On Thu, Apr 7, 2016 at 6:29 AM, Martin Pitt <martin.p...@ubuntu.com> wrote: > I reviewed the remainder of the patch: > > crypto/evp/evp_locl.h > -# define SHA1_Init private_SHA1_Init > -# define SHA224_Init private_SHA224_Init > -# define SHA256_Init private_SHA256_Init > -# define SHA384_Init private_SHA384_Init > -# define SHA512_Init private_SHA512_Init > -# define DES_set_key_unchecked private_DES_set_key_unchecked > > This looks like an API break. E. g. SHA1_Init is being used by tons of > stuff in https://codesearch.debian.net/results/SHA1_Init . > Those defines are within an OPENSSL_FIPS so were never used in regular openssl. The SHA ones were removed because the private_* routines above do not exist. And the private_DES_set_key_unchecked was removed so doesn't exist either. So, this should be ok. > > The changes in crypto/evp/p_sign.c and crypto/evp/p_verify.c don't look > FIPS related, change the default behaviour, and should probably be split > out into a separate patch with justification/origin and at least > proposed upstream. > > I did not think these change the default behaviour. They are adding PSS or X931 padding to rsa if requested via a flag. Let me investigate further as to why it was not upstreamed. My original guess was because the upstream openssl-fips module offers new FIPS_rsa_* routines that do this padding. crypto/fips/fips.c, verify_checksums() : This dynamically swaps out a > dlopen()ed libssl.so to libcrypto.so. This smells like a portion of the > upstream OpenSSL approach with using a plugin? As this patch patches the > original library source code, is that still actually needed? > > Yes, this is needed. This is part of the integrity check that is required by fips. We dlopen libcrypto.so and dlsym FIPS_mode_set. I interpreted this as a check to ensure we are running a fips capable libcrypto.so. We get the path to the file and then dlclose. We then compute the hmac of path/libcrypto.so and compare the results against the hmac we ship to verify this is our binary. Same thing is done for libssl.so. We do not yet include the hmac file to check against, so the integrity check fails and prevents running in fips mode. The openssl-fips module does this hmac integrity check of itself and libcrypto.so differently. > Note: I mostly skipped over the fips/*_selftest.c bits, they are both > structurally rather simple and also not verifiable at all for non- > experts in the algorithms. The same goes for crypto/fips/fips_drbg_ctr.c > and similar algorithms. There is some rather fiddly pointer arithmetic > and assumptions about buffer sizes there -- has there been some vetting > of this with running the tests both in FIPS and in normal mode through > valgrind? > No, at least not yet, but is a good idea. > It also concerns me that crypto/fips/ seems to reimplement RNG, HMAC, > and RSA algorithms which should already be in openssl itself. Yes, there > might be politics involved, but have there been any attemps to at least > consolidate this parts and work with upstream to unify the algorithms? > It's certainly fine if some of them get disabled in FIPS mode, or > augmented with extra runtime tests, but a complete reimplementation > seems dubious -- it wouldn't be the first time that an US government > promoted/approved RNG turned out to be a complete fraud, so some > references about the origin of this to lower the scepticism would be > appreciated. If that was really written by Steven Henson there should be > little doubts about his credentials -- but again, it's not at all clear > where these patches originate from. But particularly the reimplemented > RNG (crypto/fips/fips_rand.c) has no author information at all. > Openssl community implements a lot of the fips approved algorithms into the openssl-fips module, rather than into regular openssl. This means for us to acquire some of these fips approved algorithms, we must take them from the openssl-fips module source. The fips_rand* and fips_drbg_* are from the upstream openssl-fips module. They are fips compliant RNGs not offered in regular openssl. When running in non-fips mode can still use regular openssl RNGs. However, when running in fips mode, you must use the fips approved RNGs only. hmmm... latest fips specification update (Jan 2016), only allows DRBG for random number generators now. The PRNGs are no longer allowed in fips mode. So perhaps I should remove fips_rand.c now and not later. So I will remove. crypto/fips does not change hmac, but rather offers an hmac based DRBG. As for RSA, if you are referring to fips_rsa_x931g.c, it is from the upstream openssl-fips module. It provides rsa x931 key generation which is not offered in regular openssl. > crypto/fips/fips_utl.h contains the full definition of functions. This > is rather unclean, and could lead to linker errors or at least > duplicated symbols. It's only being included by two tests, but this is a > potential bug if the file gets included into production code some day. > > fips_utl.h is from the upstream openssl-fips module. It is a local header file that is not exported into /usr/include/openssl. But if you prefer I can move the routines into fips_test_suite.c where they are being used. Let me know if you feel strongly about this and I will move them. > What is this doing in crypto/opensslconf.h.in? > > +#ifndef OPENSSL_FIPS > +#define OPENSSL_FIPS > +#endif > > I thought OPENSSL_FIPS was meant to be defined (or not) by configure to > enable/disable FIPS support. This looks like it would now enable support > even if it's disabled, and this depends on the order of #include in .c > files that use opensslconf.h. > > crypto/o_init.c disables checking for $OPENSSL_FORCE_FIPS_MODE. What's > the rationale for this? > > Oh wow! Yeah, that is very odd... carried over from the fedora patch. I will remove that. > -- > You received this bug notification because you are subscribed to the bug > report. > https://bugs.launchpad.net/bugs/1553309 > > Title: > [FFe]: Include FIPS 140-2 into openssl package > > Status in openssl package in Ubuntu: > Incomplete > > Bug description: > This is a request for a Feature Freeze Exception to include FIPS 140-2 > selftest into the openssl package in preparation for the FIPS 140-2 > compliance for 16.0.4. > This patchset will : > - add ability to config, compile, run with fips option enabled > - add the selftest files to crypto/fips directory. > - minor changes to several algorithms in crypto directory to ensure the > selftest compile successfully when fips is enabled. > > The selftest will be initiated externally at this point and not > internally. > Hope to have a test package ready early next week. > > To manage notifications about this bug go to: > > https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1553309/+subscriptions > -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1553309 Title: [FFe]: Include FIPS 140-2 into openssl package To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1553309/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
