Joy Latten [2016-04-08 5:07 -0000]: > > -# define SHA1_Init private_SHA1_Init > Those defines are within an OPENSSL_FIPS so were never used in regular > openssl.
Ah, I see that this doesn't actually get shipped in libssl-dev, so sorry for the noise. > > 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. > > > > > I did not think these change the default behaviour. They are adding PSS or > X931 padding to rsa > if requested via a flag. Right, and both flags are already exported in usr/include/openssl/evp.h in the current (unpatched) libssl. So, while this code looks correct, it looks like a backported patch from upstream which is unrelated to the FIPS changes. Again, I just noticed that during review. If that's part of the original RedHat/SUSE patch etc., then by all means keep it (taking unmodified patches from known, reliable, and declared origins trumps pretty much everything else). But if that was one of the changes from Ubuntu/you, it should be split out and sent upstream (or say which upstream commit it was). > > It also concerns me that crypto/fips/ seems to reimplement RNG, > > HMAC, and RSA algorithms which should already be in openssl > > itself. [...] The reimplemented RNG (crypto/fips/fips_rand.c) has > > no author information at all. > > Openssl community implements a lot of the fips approved algorithms into the > openssl-fips module, rather than into regular openssl. > This means for us to acquire some of these fips approved algorithms, we > must take them from the openssl-fips module source. Ah, so that's where they are coming from? I seems a bit dubious that fips_rand.c is one of the very few files which does *not* have an author information. So I guess this is another case of "as a reviewer of this big patch I have not the slightest idea where this came from" (cf. "split and declare patches by origin" again) > fips_utl.h is from the upstream openssl-fips module. It is a local header > file that is not exported into /usr/include/openssl. > But if you prefer I can move the routines into fips_test_suite.c where they > are being used. Let me know if you feel strongly about this and I will move > them. No, I don't feel strongly about it, it just jumped my eye as a potential trap. Again, if that's in the Ubuntu modified portion it'd be nice to clean up, but do prefer unmodified patches over cleanup like this. > > crypto/o_init.c disables checking for $OPENSSL_FORCE_FIPS_MODE. What's > > the rationale for this? > > > > > Oh wow! Yeah, that is very odd... carried over from the fedora patch. I > will remove that. Just FTR, this would be a good example of keeping the fedora patch as-is, and putting back the env check would then go into the Ubuntu followup patch. Thanks! -- 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
