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 . 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. 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? 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? 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. 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. 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? -- 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
