Hi Martin,
My apology for the delay. I had a morning full of meetings and I needed to
look at the code to answer.
I have addressed the first half of your email and will continue with the
second half next. Will send another email
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:
>
hmmm... for non-fips mode, which is regular openssl, this should not change
anything.
According to the if statement, when in non-fips mode (regular openssl), the
!FIPS_mode() will evaluate to TRUE, thus causing the if statement to
always evaluate to TRUE and set the dsa_doit[i] = 1; just at it normally
would have. The other part, the new part, "i != R_DSA_512"
is for fips because it does not allow small DSA modulus bits size, such as
512 bits.
So I think this would evaluate out as, if not in fips mode then do as
normally and dsa_doit[R_DSA_512] = 1, but if in fips mode and i =
R_DSA_512, then
do not set dsa_doit[R_DSA_512] = 1. So I think should be ok.
>
> - idea_set_encrypt_key(key16, &idea_ks);
> + if (doit[D_CBC_IDEA]) {
> + idea_set_encrypt_key(key16, &idea_ks);
> + }
>
> I think this should be ok. D_CBC_IDEA is defined to be 10. So, is index 10
in the "doit" array.
So if "idea" or "idea-cbc" is passed in as an argument, doit[D_CBC_IDEA] =
1; otherwise is 0.
So the change above sets the key only if we plan on doing idea crypto.
Later on, further down we use the key in following code snippet,
Notice when we use the key to encrypt, we do check first for
doit[D_CBC_IDEA]. So may actually be a general code improvement.
# ifndef OPENSSL_NO_IDEA
if (doit[D_CBC_IDEA]) {
for (j = 0; j < SIZE_NUM; j++) {
print_message(names[D_CBC_IDEA], c[D_CBC_IDEA][j], lengths[j]);
Time_F(START);
for (count = 0, run = 1; COND(c[D_CBC_IDEA][j]); count++)
idea_cbc_encrypt(buf, buf,
(unsigned long)lengths[j], &idea_ks,
iv, IDEA_ENCRYPT);
d = Time_F(STOP);
print_result(D_CBC_IDEA, j, count, d);
}
}
# endif
It also removes some of the upstream OpenSSL FIPS code, in
> crypto/cmac/cmac.c.
>
Yes, true. First, OPENSSL_FIPS was not previously defined, so this code was
never compiled nor executed before.
Over time, the FIPS standards have changed and so some of the prior code is
no longer applicable.
In the case of CMAC, those FIPS_* calls it was making within the
OPENSSL_FIPS define no longer existed.
>
> > 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?
>
> hmmmmm.... this next part I am going to mention is going to sound weird.
The open source openssl (upstream) has developed a separate
openssl-fips module that acts as a plugin to the regular openssl. It was
decided before I got here, that we (Canonical for Ubuntu) would not use
this method.
This openssl-fips module was recently granted fips certification, so that
means it has most recent fips-compliant and fips certified crypto, not the
regular openssl. The openssl-fips module, as a plugin replaces or overlays
(not sure correct word to use), some of the crypto in regular openssl.
That does not mean the crypto in regular openssl is incorrect, just that
the crypto in openssl-fips module is fips compliant and therefore
supersedes the
crypto in regular openssl when in fips mode. The openssl-fips module is not
only bigger than this patch, but is separate and a bit more complex.
Since it is separate, it would almost be akin to maintaining 2 versions of
openssl. One advantage though is that it is maintained upstream. :-)
Again before I got here, Canonical consulted with external security
consultants who recommended we pursue the method that redhat and suse did.
Thus
this big patch. But for a more positive spin, redhat and suse used same
patch to achieve fips certification on openssl. :-)
Also, if you are referring to the upstream tests in the tests directory of
the source, we are using those. I noticed when I submitted this patch to be
built on my ppa, I viewed the build log and noticed all those tests were
run and succeeded. The build logs are still there if you want to view them.
I also ran many of these tests myself by hand. And I also utilized
testcases from the security team that tested openssl itself, and openssl
with apache, with wget, and with certificates. They all succeeded.
> 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.
>
Yes! What you have stated is true in general. Fortunately in the case you
pointed to above, this should not be a problem. Those are error codes. When
adding new error codes in openssl, is standard practice that you run "make
errors" which in turn creates all those defines for errors. That is how the
above happened.
>
> There is some pointless whitespace change in e. g. crypto/evp/c_alld.c
> which further blow up the patch.
>
> Sorry about that, I thought I had caught all the whitespaces. I can
correct that.
> 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?
>
This routine had concerned me too when I first saw it, so I had stepped
thru it with gdb while running several different testcases to better
understand it and how it flowed.
I stepped through it both in fips mode and non-fips mode.
void __attribute__ ((constructor)) OPENSSL_init_library(void)
{
static int done = 0;
if (done)
return;
done = 1;
I notice we only step through this upon initialization of the library,
because done is zero. After first time, done = 1, and any additional calls
to this will return.
> 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 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
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
