Hi Martin,
This email addresses the second half, below.
regards,
Joy
On Wed, Apr 6, 2016 at 4:33 AM, Martin Pitt <martin.p...@ubuntu.com> wrote:
> 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.
>
I don't care for this change either. I will revert it. I tracked the
origins to the original openssl fips in openssl community, back then it was
0x400 and was changed recently to 0x4000.
> 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).
>
Yes, most of the source in this patch has openssl copyright and is from the
open source openssl community at some point in time.
>From what I have gleaned, openssl community did their fips compliance such
as we are, a "fips-patched" openssl. Over time they decided to go with a
different approach, a separate openssl-fips module that plugs into the
regular openssl. And that is what they have been updating for fips
compliance.
The FIPs standards are constantly evolving and changing as exploits and
improvements are made in cryptography. So I believe distros (redhat and
suse) are using fips-specific source from the original fips-patched openssl
(such as fips.h) that they picked up years ago when they first started
doing fips certs and incorporating the newer, compliant crypto and
selftests from the openssl-fips module source (such as fips_drbg_hash.c).
I used the fedora openssl fips patch as my base and I used the openssl-fips
module source to ensure most things were up-to-date since earlier this year
they received fips certification.
Yes, this may all sound like a big headache. :-) But, it is what it is.
:-)
>
> 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.
>
Yes, I am aware of this. My reasoning was I needed to focus on correct
functionality and mitigating regression, to get this patch ready and into
release, asap. And later, I would focus on cleaning up the dead code and
whatever else. I figured those I can do in an update as bugfixes.
> 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.
>
Please see above. I can break into smaller patches for easier reviewing and
bundle with some logic.
But in terms of maintenance, I honestly don't know if it will matter. It is
just messy to me, regardless.
Sorry, I have spent some time thinking about it. I can separate out such
that everything under crypto/fips
is in one patch, and all else in another. But they won't be independent of
each other in regards to successfully applying and building.
>
> 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.
>
> Yes, I understand.
> 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 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:
> New
>
> 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
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:
New
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
