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 Touch seeded packages, which is subscribed to openssl in Ubuntu. 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 -- Mailing list: https://launchpad.net/~touch-packages Post to : touch-packages@lists.launchpad.net Unsubscribe : https://launchpad.net/~touch-packages More help : https://help.launchpad.net/ListHelp
