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 (st...@openssl.org) 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.
** Changed in: openssl (Ubuntu)
Status: Confirmed => New
--
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
