Comment 7 for bug 1553309

The patch changes behaviour even in !fips mode, e. g. in apps/speed.c:

         for (i = 0; i < DSA_NUM; i++)
- dsa_doit[i] = 1;
+ if (!FIPS_mode() || i != R_DSA_512)
+ dsa_doit[i] = 1;

(additional check for R_DSA_512), and it even modifies code that doesn't touch any FIPS flag:

- idea_set_encrypt_key(key16, &idea_ks);
+ if (doit[D_CBC_IDEA]) {
+ idea_set_encrypt_key(key16, &idea_ks);
+ }

It also removes some of the upstream OpenSSL FIPS code, in crypto/cmac/cmac.c.

> The FIPs patch will not be included into the upstream source.

Apparently there already is some FIPS support upstream (you pointed out https://www.openssl.org/docs/fips.html yourself, and the patch seems to disable that). Isn't there some middle ground where we could use the upstream tests and FIPS integration as much as possible, and make the patch less ridiculously big?

crypto/dsa/dsa.h:
 # define DSA_F_DSAPARAMS_PRINT_FP 101
+# define DSA_F_DSA_BUILTIN_KEYGEN 127
+# define DSA_F_DSA_BUILTIN_PARAMGEN 128
 # define DSA_F_DSA_BUILTIN_PARAMGEN2 126

Patches like this are utterly dangerous. As soon as a new upstream version defines their own new constant further down, the FIPS patch will most likely still apply, but silently introduce a conflict as two different constants now have the same value.

There is some pointless whitespace change in e. g. crypto/evp/c_alld.c which further blow up the patch.

There are a few extra calls to OPENSSL_init_library() now, without a comment. Is this guaranteed to be idempotent (it might reset seeds, counters and the like)? Is this a performance hit?

crypto/evp/evp.h:
-# define EVP_CIPH_FLAG_FIPS 0x4000
+# define EVP_CIPH_FLAG_FIPS 0x400

This also looks very dubious -- as this is a flag that is commonly passed to functions, this could be an ABI break.

The added file crypto/fips/fips.h says "Copyright (c) 2003 The OpenSSL Project.", other files like crypto/fips/fips_drbg_hash.c have "Written by Dr Stephen N Henson (<email address hidden>) for the OpenSSL project". So this does not seem to be originating from Red Hat, SUSE or Canonical, or is that just a copy&paste error? If it actually comes from upstream somewhere, can these parts please split out into a separate patch (e. g. one for stuff that is being taken from some upstream branch, one that is being taken from RedHat/SUSE, and then perhaps one with the modifications from Canonical).

There is a lot of added dead code, like do_bn_print_name(), parse_line(), or tidy_line(). I noticed these as I was looking for usage of strcpy(). These functions don't get any buffer size, don't do any overflow checking, etc.

There are no references to where these patches are taken from (RedHat/SUSE), and which changes were done relative to them. Please add them, so that it's a bit easier for someone else in the future to re-merge them. As I said above it would also be useful to split them up by origin, in order to have a realistic chance to maintain them in the future.

These are some eye-brow risers from the first 15% of the patch. I'm sorry, but I'm afraid after even this very shallow review my confidence in this patch both in terms of quality as well as long-term maintainability isn't very high. This isn't meant as a final veto from the release team, just that I personally can't ack this in good faith.

That said, I agree that *if* this is meant to land, then it's certainly better to land it now rather than in an SRU, as this kind of changes is not appropriate at all for an SRU.