[PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[Kyle McMartin]

Commit 1d0059f3a added a test to check if the system is booted in fips
mode, and if so, panic the system if an unsigned module is loaded.
However the wording of the changelog "in signature enforcing mode" leads
one to assume that sig_enforce should be set for the panic to occur and
that these two tests are transposed.

Move the test for -ENOKEY && !sig_enforce before the test of fips_mode,
so that err will be 0, and the panic will not trigger unless we've
explicitly disabled unsigned modules with sig_enforce set, so that
systemtap and 3rd party modules will work in fips mode. (This also
matches the behaviour by Red Hat Enterprise Linux 6.)

Things which need to deny module loading such as secure boot already set
sig_enforce, so there's no issue here.

Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Kyle McMartin <kyle@redhat.com>

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2460,11 +2460,11 @@ static int module_sig_check(struct load_info *info)
 	}
 
 	/* Not having a signature is only an error if we're strict. */
+	if (err == -ENOKEY && !sig_enforce)
+		err = 0;
 	if (err < 0 && fips_enabled)
 		panic("Module verification failed with error %d in FIPS mode\n",
 		      err);
-	if (err == -ENOKEY && !sig_enforce)
-		err = 0;
 
 	return err;
 }

Re: [PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[Rusty Russell]

Kyle McMartin <kyle@redhat.com> writes:
> Commit 1d0059f3a added a test to check if the system is booted in fips
> mode, and if so, panic the system if an unsigned module is loaded.
> However the wording of the changelog "in signature enforcing mode" leads
> one to assume that sig_enforce should be set for the panic to occur and
> that these two tests are transposed.
>
> Move the test for -ENOKEY && !sig_enforce before the test of fips_mode,
> so that err will be 0, and the panic will not trigger unless we've
> explicitly disabled unsigned modules with sig_enforce set, so that
> systemtap and 3rd party modules will work in fips mode. (This also
> matches the behaviour by Red Hat Enterprise Linux 6.)
>
> Things which need to deny module loading such as secure boot already set
> sig_enforce, so there's no issue here.
>
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Kyle McMartin <kyle@redhat.com>

Seems reasonable, but I'll want David Howells' Ack.

Thanks,
Rusty.

> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2460,11 +2460,11 @@ static int module_sig_check(struct load_info *info)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> +	if (err == -ENOKEY && !sig_enforce)
> +		err = 0;
>  	if (err < 0 && fips_enabled)
>  		panic("Module verification failed with error %d in FIPS mode\n",
>  		      err);
> -	if (err == -ENOKEY && !sig_enforce)
> -		err = 0;
>  
>  	return err;
>  }

Re: [PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[David Howells]

Kyle McMartin <kyle@redhat.com> wrote:

> Commit 1d0059f3a added a test to check if the system is booted in fips
> mode, and if so, panic the system if an unsigned module is loaded.
> However the wording of the changelog "in signature enforcing mode" leads
> one to assume that sig_enforce should be set for the panic to occur and
> that these two tests are transposed.
> 
> Move the test for -ENOKEY && !sig_enforce before the test of fips_mode,
> so that err will be 0, and the panic will not trigger unless we've
> explicitly disabled unsigned modules with sig_enforce set, so that
> systemtap and 3rd party modules will work in fips mode. (This also
> matches the behaviour by Red Hat Enterprise Linux 6.)
> 
> Things which need to deny module loading such as secure boot already set
> sig_enforce, so there's no issue here.
> 
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Kyle McMartin <kyle@redhat.com>

Fine by me, but adding Stephan Mueller for his input.

David

> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2460,11 +2460,11 @@ static int module_sig_check(struct load_info *info)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> +	if (err == -ENOKEY && !sig_enforce)
> +		err = 0;
>  	if (err < 0 && fips_enabled)
>  		panic("Module verification failed with error %d in FIPS mode\n",
>  		      err);
> -	if (err == -ENOKEY && !sig_enforce)
> -		err = 0;
>  
>  	return err;
>  }

Re: [PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[Stephan Mueller]

On 23.01.2013 12:26:15, +0100, David Howells <dhowells@redhat.com> wrote:

Hi David,

> Kyle McMartin <kyle@redhat.com> wrote:
> 
>> Commit 1d0059f3a added a test to check if the system is booted in fips
>> mode, and if so, panic the system if an unsigned module is loaded.
>> However the wording of the changelog "in signature enforcing mode" leads
>> one to assume that sig_enforce should be set for the panic to occur and
>> that these two tests are transposed.
>>
>> Move the test for -ENOKEY && !sig_enforce before the test of fips_mode,
>> so that err will be 0, and the panic will not trigger unless we've
>> explicitly disabled unsigned modules with sig_enforce set, so that
>> systemtap and 3rd party modules will work in fips mode. (This also
>> matches the behaviour by Red Hat Enterprise Linux 6.)
>>
>> Things which need to deny module loading such as secure boot already set
>> sig_enforce, so there's no issue here.
>>
>> Reported-by: Jan Stancek <jstancek@redhat.com>
>> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> 
> Fine by me, but adding Stephan Mueller for his input.

FIPS requires the module (in our case the static kernel binary with its
kernel crypto API plus all the crypto kernel modules) to be unavailable
if the module signature fails. That is an unconditional requirement.

Now, all is a name game from here on. With your patch, the FIPS mode is
only enabled if both flags, i.e. fips_enabled *and* sig_enforce are set!
IMHO this is very misleading of the fips_enabled flag which is intended
to be the only trigger for the FIPS mode.

Hence, I would NACK the patch -- only from this point of view (i.e. I do
not have a technical argument against the patch)!

The solution to the problem IMHO is rather to somehow identify crypto
modules, i.e. modules that hook themselves into the kernel crypto API
and only panic the kernel when those integrity checks fail. Therefore,
to remove the panic() call in the module loading function when
fips_enabled is set would entail to:

1. load and sig check the module as it is done now

2. remember whether the signature check passed or failed for the loaded
module

3. in the cipher initialization code of the crypto API (i.e. the one
behind crypto_register_alg()), you check the signature check flag --
panic the kernel when the flag shows that the signature check failed

This way you limit the panic on signature checks in FIPS mode to the
crypto modules.

> 
> David
> 
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2460,11 +2460,11 @@ static int module_sig_check(struct load_info *info)
>>  	}
>>  
>>  	/* Not having a signature is only an error if we're strict. */
>> +	if (err == -ENOKEY && !sig_enforce)
>> +		err = 0;
>>  	if (err < 0 && fips_enabled)
>>  		panic("Module verification failed with error %d in FIPS mode\n",
>>  		      err);
>> -	if (err == -ENOKEY && !sig_enforce)
>> -		err = 0;
>>  
>>  	return err;
>>  }
> 

Re: [PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[Kyle McMartin]

On Wed, Jan 23, 2013 at 04:18:32PM +0100, Stephan Mueller wrote:
> 3. in the cipher initialization code of the crypto API (i.e. the one
> behind crypto_register_alg()), you check the signature check flag --
> panic the kernel when the flag shows that the signature check failed
> 
> This way you limit the panic on signature checks in FIPS mode to the
> crypto modules.
> 

I was hoping we could just do what we do for driver/staging and set a
flag in modpost for crypto modules, but it looks like since we have
crypto modules outside of crypto/ for things like aesni, that won't
work. Maybe that is a better choice, but it seems like an awful kludge.

--Kyle

Re: [PATCH] MODSIGN: only panic in fips mode if sig_enforce is set

[Stephan Mueller]

On 24.01.2013 15:59:07, +0100, Kyle McMartin <kmcmarti@redhat.com> wrote:

Hi Kyle,
> On Wed, Jan 23, 2013 at 04:18:32PM +0100, Stephan Mueller wrote:
>> 3. in the cipher initialization code of the crypto API (i.e. the one
>> behind crypto_register_alg()), you check the signature check flag --
>> panic the kernel when the flag shows that the signature check failed
>>
>> This way you limit the panic on signature checks in FIPS mode to the
>> crypto modules.
>>
> 
> I was hoping we could just do what we do for driver/staging and set a
> flag in modpost for crypto modules, but it looks like since we have
> crypto modules outside of crypto/ for things like aesni, that won't
> work. Maybe that is a better choice, but it seems like an awful kludge.

There is a completely different possibility though:

The reason for the panic is the requirement that if one of the integrity
tests for the crypto components fail in FIPS mode, the crypto module
(i.e. the entire kernel crypto API) must be unavailable.

Thus, another possibility is to add a global flag that indicates whether
such a test failed.

That flag is evaluated by *every* kernel crypto API interface function
and this function rejects to do anything when that flag is set.

This way, we can get rid of the panic.

But then, we still have the problem of loading unsigned kernel modules
in FIPS mode that are not related to crypto: those would still trigger
setting that global flag if the module loader is unable to identify a
crypto module.

Hence, you idea with a flag may be the way to go: add such a crypto flag
similar to the MODULE_LICENSE("GPL") for modules. Now, the module loader
should only panic or set such a global flag for modules that are marked
with something like MODULE_TYPE("CRYPTO").

Now, all modules in crypto/ and arch/*/crypto/* (or whereever) would
have such a flag added.
> 
> --Kyle
> 
Ciao
Stephan

[PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Kyle McMartin]

After thinking about it a while, this seems like the best way to solve
the problem, although it does still kind of offend my delicate
sensibilities...

Doing this check in the crypto layer seems kind of like a layering
violation to me (and, to be honest, I think it'd be a gross-hack getting
from the callee back to the caller module.)

Instead, doing it in kernel/module.c and looking at the undefined symbol
list again looks to me like an ordering problem since we're doing the
signature check before we've even done the elf header check. We'd have
to move the panic to a part of the module code that may not necessarily
make sense.

Whereas checking the undefined symbol list at modpost time is fairly
logical, and adding a new MODULE_INFO entry in the CONFIG_CRYPTO_FIPS
case is a well understood thing to do, and should catch all the crypto
registrations regardless of where they exist in-tree...

Seems to build and work with both values of CONFIG_CRYPTO_FIPS.

Thoughts?

Signed-off-by: Kyle McMartin <kyle@redhat.com>

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2459,8 +2459,10 @@ static int module_sig_check(struct load_info *info)
 		return 0;
 	}
 
-	/* Not having a signature is only an error if we're strict. */
-	if (err < 0 && fips_enabled)
+	/* Not having a signature is only an error if we're strict, and
+	 * the module registers a crypto algorithm (checked in modpost)
+	 */
+	if (err < 0 && fips_enabled && !get_modinfo(info, "crypto_fips"))
 		panic("Module verification failed with error %d in FIPS mode\n",
 		      err);
 	if (err == -ENOKEY && !sig_enforce)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index ff36c50..79f46e2 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1888,6 +1888,23 @@ static void add_staging_flag(struct buffer *b, const char *name)
 		buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n");
 }
 
+static void add_crypto_flag(struct buffer *b, struct module *mod)
+{
+#if defined(CONFIG_CRYPTO_FIPS)
+	struct symbol *s;
+
+	/* iterate unresolved symbols looking for...
+	 *  - crypto_register_algs
+	 */
+	for (s = mod->unres; s; s = s->next) {
+		if (strcmp("crypto_register_algs", s->name) == 0)
+			buf_printf(b, "\nMODULE_INFO(crypto_fips, \"Y\");\n");
+	}
+#else
+	return;
+#endif /*CONFIG_CRYPTO_FIPS*/
+}
+
 /**
  * Record CRCs for unresolved symbols
  **/
@@ -2202,6 +2219,7 @@ int main(int argc, char **argv)
 		add_header(&buf, mod);
 		add_intree_flag(&buf, !external_module);
 		add_staging_flag(&buf, mod->name);
+		add_crypto_flag(&buf, mod);
 		err |= add_versions(&buf, mod);
 		add_depends(&buf, mod, modules);
 		add_moddevtable(&buf, mod);

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Kyle McMartin]

On Thu, Jan 24, 2013 at 02:06:10PM -0500, Kyle McMartin wrote:
> +	if (err < 0 && fips_enabled && !get_modinfo(info, "crypto_fips"))

Sigh, that should be get_modinfo(...)
	if (err < 0 && fips_enabled && get_modinfo(info, "crypto_fips"))

Thinko when converting from flagging things as "nocrypto" to the above,
since it touches far fewer .ko

--Kyle

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Rusty Russell]

Kyle McMartin <kyle@redhat.com> writes:
> After thinking about it a while, this seems like the best way to solve
> the problem, although it does still kind of offend my delicate
> sensibilities...

You're far too polite.  This patch was horrible, partial and ugly.

Stephan Mueller <stephan.mueller@atsec.com> wrote:
> FIPS requires the module (in our case the static kernel binary with its
> kernel crypto API plus all the crypto kernel modules) to be unavailable
> if the module signature fails. That is an unconditional requirement.

"the module signature" here being the signature of any crypto module,
I'm guessing from Kyle's awful patch.  Any crypto module, or just some?
Presumably any module used by any crypto module, too?

Because you can panic when a !sig_ok module registers a crypto
algorithm.  Or you can panic when anyone registers a crypto algorithm
after any module has failed the signature check.

But it doesn't make much sense to pick on the crypto modules, since
they're not well isolated from the rest of the kernel.

Thanks,
Rusty.

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Kyle McMartin]

On Fri, Jan 25, 2013 at 10:06:01AM +1030, Rusty Russell wrote:
> Kyle McMartin <kyle@redhat.com> writes:
> > After thinking about it a while, this seems like the best way to solve
> > the problem, although it does still kind of offend my delicate
> > sensibilities...
> 
> You're far too polite.  This patch was horrible, partial and ugly.
> 

Yeah, fair enough.

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Stephan Mueller]

On 25.01.2013 00:36:01, +0100, Rusty Russell <rusty@rustcorp.com.au> wrote:

Hi Rusty,
> Kyle McMartin <kyle@redhat.com> writes:
>> After thinking about it a while, this seems like the best way to solve
>> the problem, although it does still kind of offend my delicate
>> sensibilities...
> 
> You're far too polite.  This patch was horrible, partial and ugly.

Well, in another email I suggested you may want to think about some
marker that the code of the module would contain, similar to the GPL
flag for the module (whose absence sets the tainted flag).
> 
> Stephan Mueller <stephan.mueller@atsec.com> wrote:
>> FIPS requires the module (in our case the static kernel binary with its
>> kernel crypto API plus all the crypto kernel modules) to be unavailable
>> if the module signature fails. That is an unconditional requirement.
> 
> "the module signature" here being the signature of any crypto module,
> I'm guessing from Kyle's awful patch.  Any crypto module, or just some?
> Presumably any module used by any crypto module, too?

Any module loading into the kernel crypto API must be caught and its
signature enforced. Thus Kyle's approach to catch the kernel crypto API
register function would be appropriate, if indeed we would catch all
crypto KOs that we want to catch -- see my remark to Kyle.

> 
> Because you can panic when a !sig_ok module registers a crypto
> algorithm.  Or you can panic when anyone registers a crypto algorithm
> after any module has failed the signature check.

I do not see the difference from a FIPS perspective. The crypto KO is
unavailable to any crypto user until it called the kernel crypto API
register function.

Thus, if a KO is loaded, its signature check failed and now lingers in
the kernel, I do not see how the main FIPS requirement is offended. Only
when the code in the KO registers with the crypto API, that is the
latest point when the kernel must stop that KO whose signature check
failed -- again from a FIPS perspective.
> 
> But it doesn't make much sense to pick on the crypto modules, since
> they're not well isolated from the rest of the kernel.

Technically, I am totally with you. That business of integrity check of
a subset of code that is loaded into the kernel realm is hard to grasp,
if you want to stop an attacker.

But that is not the focus of the FIPS test here. That test shall counter
accidental modifications (how unlikely they are). And I am fully aware
of the fact that this FIPS requirement does not make too much sense in
software implementations. Note, FIPS 140-2 mainly focuses on hardware
and has some requirements which are totally bogus for software -- this
is one of them.

Well, but if we want to be FIPS 140-2 compliant, either we meet that
requirement, or, well, you are not compliant. It is that easy. :-)

Ciao
Stephan
> 
> Thanks,
> Rusty.
> 

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Rusty Russell]

Stephan Mueller <stephan.mueller@atsec.com> writes:
> On 25.01.2013 00:36:01, +0100, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> "the module signature" here being the signature of any crypto module,
>> I'm guessing from Kyle's awful patch.  Any crypto module, or just some?
>> Presumably any module used by any crypto module, too?
>
> Any module loading into the kernel crypto API must be caught and its
> signature enforced. Thus Kyle's approach to catch the kernel crypto API
> register function would be appropriate, if indeed we would catch all
> crypto KOs that we want to catch -- see my remark to Kyle.

OK, so perhaps in fips mode we should fail the various crypto register
calls if the kernel is tainted?

> But that is not the focus of the FIPS test here. That test shall counter
> accidental modifications (how unlikely they are). And I am fully aware
> of the fact that this FIPS requirement does not make too much sense in
> software implementations. Note, FIPS 140-2 mainly focuses on hardware
> and has some requirements which are totally bogus for software -- this
> is one of them.
>
> Well, but if we want to be FIPS 140-2 compliant, either we meet that
> requirement, or, well, you are not compliant. It is that easy. :-)

Two important principles here:
1) Ugliness and craziness must be contained in the subsystem which cares.
2) Minimize effort spent on craziness.

Principle #1 means I want this in the crypto subsystem, not the module
subsystem.

Cheers,
Rusty.

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Stephan Mueller]

On 25.01.2013 00:36:01, +0100, Rusty Russell <rusty@rustcorp.com.au> wrote:

Hi Rusty at al,

while we are at FIPS discussions, may I propose a slight fix because the
FIPS mode is not covering the FIPS 200 (a management system set of
requirements), but FIPS 140-2 covering implementation requirements for
cryptography.

Signed-off-by: Stephan Mueller <smueller@chronox.de>

--- Kconfig.orig        2013-01-25 13:42:54.649705380 +0100
+++ Kconfig     2013-01-25 13:43:16.737705712 +0100
@@ -22,11 +22,11 @@
 comment "Crypto core or helper"

 config CRYPTO_FIPS
-       bool "FIPS 200 compliance"
+       bool "FIPS 140-2 compliance"
        depends on CRYPTO_ANSI_CPRNG && !CRYPTO_MANAGER_DISABLE_TESTS
        help
          This options enables the fips boot option which is
-         required if you want to system to operate in a FIPS 200
+         required if you want to system to operate in a FIPS 140-2
          certification.  You should say no unless you know what
          this is.

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Stephan Mueller]

On 24.01.2013 20:06:10, +0100, Kyle McMartin <kyle@redhat.com> wrote:

Hi Kyle,
> After thinking about it a while, this seems like the best way to solve
> the problem, although it does still kind of offend my delicate
> sensibilities...
> 
> Doing this check in the crypto layer seems kind of like a layering
> violation to me (and, to be honest, I think it'd be a gross-hack getting
> from the callee back to the caller module.)
> 
> Instead, doing it in kernel/module.c and looking at the undefined symbol
> list again looks to me like an ordering problem since we're doing the
> signature check before we've even done the elf header check. We'd have
> to move the panic to a part of the module code that may not necessarily
> make sense.
> 
> Whereas checking the undefined symbol list at modpost time is fairly
> logical, and adding a new MODULE_INFO entry in the CONFIG_CRYPTO_FIPS
> case is a well understood thing to do, and should catch all the crypto
> registrations regardless of where they exist in-tree...
> 
> Seems to build and work with both values of CONFIG_CRYPTO_FIPS.
> 
> Thoughts?

This patch is absolutely ok with FIPS requirements.

But I think that patch falls short, because the crypto API offers other
register functions:

$ pwd
.../include/crypto
$ grep -r crypto_register *
algapi.h:int crypto_register_template(struct crypto_template *tmpl);
algapi.h:int crypto_register_instance(struct crypto_template *tmpl,
internal/compress.h:extern int crypto_register_pcomp(struct pcomp_alg *alg);
internal/hash.h:int crypto_register_ahash(struct ahash_alg *alg);
internal/hash.h:int crypto_register_shash(struct shash_alg *alg);
internal/hash.h:int crypto_register_shashes(struct shash_alg *algs, int
count);

$ pwd
...crypto
$ grep -r crypto_register algapi.c  | grep EXPORT
EXPORT_SYMBOL_GPL(crypto_register_alg);
EXPORT_SYMBOL_GPL(crypto_register_algs);
EXPORT_SYMBOL_GPL(crypto_register_template);
EXPORT_SYMBOL_GPL(crypto_register_instance);
EXPORT_SYMBOL_GPL(crypto_register_notifier);

So, please add these calls to your code too.

Though, I have one concern that it does not catches all circumstances.
What happens, if a crypto algo implementation consists of two KOs where
one KO has the register function and the other KO implements other
aspects of the cipher? In this case, both KOs must be covered with the
check. Can that happen in general?

Also, currently lib/sha1.c and lib/md5.c are statically compiled -- so
we are fine. Though, is it possible that they may be offloaded into KOs
in the future? If yes, they would need to be covered too.

Ciao
Stephan

> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2459,8 +2459,10 @@ static int module_sig_check(struct load_info *info)
>  		return 0;
>  	}
>  
> -	/* Not having a signature is only an error if we're strict. */
> -	if (err < 0 && fips_enabled)
> +	/* Not having a signature is only an error if we're strict, and
> +	 * the module registers a crypto algorithm (checked in modpost)
> +	 */
> +	if (err < 0 && fips_enabled && !get_modinfo(info, "crypto_fips"))
>  		panic("Module verification failed with error %d in FIPS mode\n",
>  		      err);
>  	if (err == -ENOKEY && !sig_enforce)
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index ff36c50..79f46e2 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1888,6 +1888,23 @@ static void add_staging_flag(struct buffer *b, const char *name)
>  		buf_printf(b, "\nMODULE_INFO(staging, \"Y\");\n");
>  }
>  
> +static void add_crypto_flag(struct buffer *b, struct module *mod)
> +{
> +#if defined(CONFIG_CRYPTO_FIPS)
> +	struct symbol *s;
> +
> +	/* iterate unresolved symbols looking for...
> +	 *  - crypto_register_algs
> +	 */
> +	for (s = mod->unres; s; s = s->next) {
> +		if (strcmp("crypto_register_algs", s->name) == 0)
> +			buf_printf(b, "\nMODULE_INFO(crypto_fips, \"Y\");\n");
> +	}
> +#else
> +	return;
> +#endif /*CONFIG_CRYPTO_FIPS*/
> +}
> +
>  /**
>   * Record CRCs for unresolved symbols
>   **/
> @@ -2202,6 +2219,7 @@ int main(int argc, char **argv)
>  		add_header(&buf, mod);
>  		add_intree_flag(&buf, !external_module);
>  		add_staging_flag(&buf, mod->name);
> +		add_crypto_flag(&buf, mod);
>  		err |= add_versions(&buf, mod);
>  		add_depends(&buf, mod, modules);
>  		add_moddevtable(&buf, mod);
> 

[RFC PATCH] fips: check whether a module registering an alg or template is signed

[Kyle McMartin]

fips mode needs to prevent unsigned modules from registering crypto
algorithms, and currently panics if an unsigned module is attempted to
be loaded. Instead, forbid (by returning -EINVAL) registering a crypto
alg or template if fips mode is enabled and the module signature is not
valid.

crypto_sig_check should return 1 (and allow the registration) if any
of the following are true:
 1/ fips is not enabled (but CONFIG_CRYPTO_FIPS is enabled.)
 2/ the algorithm is built into the kernel (THIS_MODULE == NULL)
 3/ the algorithm is in a module, and the module sig check passes
and fail in any of the other cases.

Checking in crypto_check_alg and crypto_register_template seems to hit
the callpoints as far as I can see.

Signed-off-by: Kyle McMartin <kyle@redhat.com>

---

rusty,

How about something like this? It keeps the FIPS mess in the
crypto/fips.c file (aside from something that goes away entirely in the
!CONFIG_CRYPTO_FIPS case.)

regards, Kyle

--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -52,6 +52,9 @@ static int crypto_check_alg(struct crypto_alg *alg)
 	if (alg->cra_priority < 0)
 		return -EINVAL;
 
+	if (!crypto_sig_check(alg->cra_module))
+		return -EINVAL;
+
 	return crypto_set_driver_name(alg);
 }
 
@@ -435,6 +438,11 @@ int crypto_register_template(struct crypto_template *tmpl)
 			goto out;
 	}
 
+	if (!crypto_sig_check(tmpl->module)) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	list_add(&tmpl->list, &crypto_template_list);
 	crypto_notify(CRYPTO_MSG_TMPL_REGISTER, tmpl);
 	err = 0;
diff --git a/crypto/fips.c b/crypto/fips.c
index 5539700..2ebbe0f 100644
--- a/crypto/fips.c
+++ b/crypto/fips.c
@@ -15,6 +15,19 @@
 int fips_enabled;
 EXPORT_SYMBOL_GPL(fips_enabled);
 
+/* forbid loading modules in fips mode if the module is not signed */
+int crypto_sig_check(struct module *m)
+{
+#if defined(CONFIG_MODULE_SIG)
+	if (!fips_enabled || !m || (m && m->sig_ok))
+		return 1;
+	else
+		return 0;
+#else
+	return 1;
+#endif
+}
+
 /* Process kernel command-line parameter at boot time. fips=0 or fips=1 */
 static int fips_enable(char *str)
 {
diff --git a/crypto/internal.h b/crypto/internal.h
index 9ebedae..937bfaf 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -139,5 +139,14 @@ static inline void crypto_notify(unsigned long val, void *v)
 	blocking_notifier_call_chain(&crypto_chain, val, v);
 }
 
+#if defined(CONFIG_CRYPTO_FIPS)
+int crypto_sig_check(struct module *m);
+#else
+static inline int crypto_sig_check(struct module *m)
+{
+	return 1;
+}
+#endif
+
 #endif	/* _CRYPTO_INTERNAL_H */
 

Re: [RFC PATCH] fips: check whether a module registering an alg or template is signed

[Stephan Mueller]

On 05.02.2013 23:58:30, +0100, Kyle McMartin <kyle@redhat.com> wrote:

Hi Kyle,

> fips mode needs to prevent unsigned modules from registering crypto
> algorithms, and currently panics if an unsigned module is attempted to
> be loaded. Instead, forbid (by returning -EINVAL) registering a crypto
> alg or template if fips mode is enabled and the module signature is not
> valid.

Just reading this paragraph, there is one missing puzzle piece: the
*entire* kernel crypto API must shut down, even if only one kernel
module with one cipher (or block chaining mode, ...) has a broken signature.

The overall requirement is: if one self test fails, the entire FIPS
140-2 crypto module must become unavailable. (please note and do not get
confused by the overload of the term "module" -- we have the KOs the
kernel loads, and we have something called a FIPS 140-2 module which is
the entire crypto "library" subject to a FIPS 140-2 validation)

This signature check is one self test required at runtime.

I added comments inline into the patch.

> 
> crypto_sig_check should return 1 (and allow the registration) if any
> of the following are true:
>  1/ fips is not enabled (but CONFIG_CRYPTO_FIPS is enabled.)
>  2/ the algorithm is built into the kernel (THIS_MODULE == NULL)
>  3/ the algorithm is in a module, and the module sig check passes
> and fail in any of the other cases.
> 
> Checking in crypto_check_alg and crypto_register_template seems to hit
> the callpoints as far as I can see.
> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> 
> ---
> 
> rusty,
> 
> How about something like this? It keeps the FIPS mess in the
> crypto/fips.c file (aside from something that goes away entirely in the
> !CONFIG_CRYPTO_FIPS case.)
> 
> regards, Kyle
> 
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -52,6 +52,9 @@ static int crypto_check_alg(struct crypto_alg *alg)
>  	if (alg->cra_priority < 0)
>  		return -EINVAL;
>  
> +	if (!crypto_sig_check(alg->cra_module))
> +		return -EINVAL;

Instead of an EINVAL, the kernel either must panic(), or a global flag
is introduced which is evaluated by every kernel crypto API call. If
that flag is, say, false, none of the kernel crypto API calls must succeed.
> +
>  	return crypto_set_driver_name(alg);
>  }
>  
> @@ -435,6 +438,11 @@ int crypto_register_template(struct crypto_template *tmpl)


I am wondering whether the modification of these two functions are
sufficient. As I wrote in a previous email, there are a number of
register functions the kernel crypto API exports and which are used.

>  			goto out;
>  	}
>  
> +	if (!crypto_sig_check(tmpl->module)) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	list_add(&tmpl->list, &crypto_template_list);
>  	crypto_notify(CRYPTO_MSG_TMPL_REGISTER, tmpl);
>  	err = 0;
> diff --git a/crypto/fips.c b/crypto/fips.c
> index 5539700..2ebbe0f 100644
> --- a/crypto/fips.c
> +++ b/crypto/fips.c
> @@ -15,6 +15,19 @@
>  int fips_enabled;
>  EXPORT_SYMBOL_GPL(fips_enabled);
>  
> +/* forbid loading modules in fips mode if the module is not signed */
> +int crypto_sig_check(struct module *m)
> +{
> +#if defined(CONFIG_MODULE_SIG)
> +	if (!fips_enabled || !m || (m && m->sig_ok))
> +		return 1;
> +	else
> +		return 0;

This code looks good.

> +#else
> +	return 1;
> +#endif
> +}
> +
>  /* Process kernel command-line parameter at boot time. fips=0 or fips=1 */
>  static int fips_enable(char *str)
>  {
> diff --git a/crypto/internal.h b/crypto/internal.h
> index 9ebedae..937bfaf 100644
> --- a/crypto/internal.h
> +++ b/crypto/internal.h
> @@ -139,5 +139,14 @@ static inline void crypto_notify(unsigned long val, void *v)
>  	blocking_notifier_call_chain(&crypto_chain, val, v);
>  }
>  
> +#if defined(CONFIG_CRYPTO_FIPS)
> +int crypto_sig_check(struct module *m);
> +#else
> +static inline int crypto_sig_check(struct module *m)
> +{
> +	return 1;
> +}
> +#endif
> +
>  #endif	/* _CRYPTO_INTERNAL_H */
>  
> 

Re: [RFC PATCH] fips: check whether a module registering an alg or template is signed

[Kyle McMartin]

On Wed, Feb 06, 2013 at 09:02:46AM +0100, Stephan Mueller wrote:
> On 05.02.2013 23:58:30, +0100, Kyle McMartin <kyle@redhat.com> wrote:
> 
> Hi Kyle,
>

Thanks for the review, Stephan.
 
> Just reading this paragraph, there is one missing puzzle piece: the
> *entire* kernel crypto API must shut down, even if only one kernel
> module with one cipher (or block chaining mode, ...) has a broken signature.
> 
> The overall requirement is: if one self test fails, the entire FIPS
> 140-2 crypto module must become unavailable. (please note and do not get
> confused by the overload of the term "module" -- we have the KOs the
> kernel loads, and we have something called a FIPS 140-2 module which is
> the entire crypto "library" subject to a FIPS 140-2 validation)
> 
> This signature check is one self test required at runtime.
> 
> I added comments inline into the patch.
> 
> > 
> > crypto_sig_check should return 1 (and allow the registration) if any
> > of the following are true:
> > +	if (!crypto_sig_check(alg->cra_module))
> > +		return -EINVAL;
> 
> Instead of an EINVAL, the kernel either must panic(), or a global flag
> is introduced which is evaluated by every kernel crypto API call. If
> that flag is, say, false, none of the kernel crypto API calls must succeed.

Returning -EINVAL means the module does not successfully load, and
nothing is registered. I don't see why you would need to taint or panic,
if nothing untoward actually occured? I don't object to it, if it's
necessary, I just didn't think it was. If Herbert doesn't object to this
patch, I'd move the panic from kernel/module.c to here.

> > +
> >  	return crypto_set_driver_name(alg);
> >  }
> >  
> > @@ -435,6 +438,11 @@ int crypto_register_template(struct crypto_template *tmpl)
> 
> 
> I am wondering whether the modification of these two functions are
> sufficient. As I wrote in a previous email, there are a number of
> register functions the kernel crypto API exports and which are used.
> 

Between these two, I believe all codepaths that could bring in a
mode, cipher, or other cryptographic algorithm are covered.

> >  			goto out;
> >  	}

regards, Kyle

Re: [RFC PATCH] fips: check whether a module registering an alg or template is signed

[Stephan Mueller]

On 06.02.2013 17:15:57, +0100, Kyle McMartin <kmcmarti@redhat.com> wrote:

Hi Kyle,
> On Wed, Feb 06, 2013 at 09:02:46AM +0100, Stephan Mueller wrote:
>> On 05.02.2013 23:58:30, +0100, Kyle McMartin <kyle@redhat.com> wrote:
>>
>> Hi Kyle,
>>
> 
> Thanks for the review, Stephan.
>  
>> Just reading this paragraph, there is one missing puzzle piece: the
>> *entire* kernel crypto API must shut down, even if only one kernel
>> module with one cipher (or block chaining mode, ...) has a broken signature.
>>
>> The overall requirement is: if one self test fails, the entire FIPS
>> 140-2 crypto module must become unavailable. (please note and do not get
>> confused by the overload of the term "module" -- we have the KOs the
>> kernel loads, and we have something called a FIPS 140-2 module which is
>> the entire crypto "library" subject to a FIPS 140-2 validation)
>>
>> This signature check is one self test required at runtime.
>>
>> I added comments inline into the patch.
>>
>>>
>>> crypto_sig_check should return 1 (and allow the registration) if any
>>> of the following are true:
>>> +	if (!crypto_sig_check(alg->cra_module))
>>> +		return -EINVAL;
>>
>> Instead of an EINVAL, the kernel either must panic(), or a global flag
>> is introduced which is evaluated by every kernel crypto API call. If
>> that flag is, say, false, none of the kernel crypto API calls must succeed.
> 
> Returning -EINVAL means the module does not successfully load, and
> nothing is registered. I don't see why you would need to taint or panic,
> if nothing untoward actually occured? I don't object to it, if it's

Unfortunately there has already something terrible happened: an
additional piece of code loaded into the FIPS 140-2 module could not be
loaded because a self test failed. This is a terrible accident in FIPS
140-2 speak. :-)

That means, the FIPS 140-2 module, aka kernel crypto API must become
unavailable. As one self test failed, the entire module must become
unavailable.

I am sorry, but there is no way around it. Just to quote the relevant
part from the FIPS 140-2 specification, section 4.9:

If a cryptographic module fails a self-test, the module shall enter an
error state and output an error indicator
via the status output interface. The cryptographic module shall not
perform any cryptographic operations
while in an error state. All data output via the data output interface
shall be inhibited when an error state
exists.


==> the signature test we are discussing here is one of these self
tests, in particular a conditional self test defined in section 4.9.2 of
the FIPS 140-2 standard.

> necessary, I just didn't think it was. If Herbert doesn't object to this
> patch, I'd move the panic from kernel/module.c to here.

I am perfectly happy with the move of the code.

> 
>>> +
>>>  	return crypto_set_driver_name(alg);
>>>  }
>>>  
>>> @@ -435,6 +438,11 @@ int crypto_register_template(struct crypto_template *tmpl)
>>
>>
>> I am wondering whether the modification of these two functions are
>> sufficient. As I wrote in a previous email, there are a number of
>> register functions the kernel crypto API exports and which are used.
>>
> 
> Between these two, I believe all codepaths that could bring in a
> mode, cipher, or other cryptographic algorithm are covered.

Ok, I believe you as I did not trace the code. I just wanted to point
out this issue :-)

But note, if a real FIPS 140-2 validation is conducted, we will trace
the code ;-)
> 
>>>  			goto out;
>>>  	}
> 
> regards, Kyle
> 
Ciao
Stephan

Re: [RFC PATCH] fips: check whether a module registering an alg or template is signed

[Kyle McMartin]

On Wed, Feb 06, 2013 at 06:45:45PM +0100, Stephan Mueller wrote:
> Unfortunately there has already something terrible happened: an
> additional piece of code loaded into the FIPS 140-2 module could not be
> loaded because a self test failed. This is a terrible accident in FIPS
> 140-2 speak. :-)
> 
> That means, the FIPS 140-2 module, aka kernel crypto API must become
> unavailable. As one self test failed, the entire module must become
> unavailable.
> 
> I am sorry, but there is no way around it. Just to quote the relevant
> part from the FIPS 140-2 specification, section 4.9:
> 
> If a cryptographic module fails a self-test, the module shall enter an
> error state and output an error indicator
> via the status output interface. The cryptographic module shall not
> perform any cryptographic operations
> while in an error state. All data output via the data output interface
> shall be inhibited when an error state
> exists.
> 

OK. If Herbert and Rusty are ok with this, I'll send an additional patch
moving the panic which should satisfy this requirement.

> 
> ==> the signature test we are discussing here is one of these self
> tests, in particular a conditional self test defined in section 4.9.2 of
> the FIPS 140-2 standard.
> 
> > necessary, I just didn't think it was. If Herbert doesn't object to this
> > patch, I'd move the panic from kernel/module.c to here.
> 
> I am perfectly happy with the move of the code.
> 

regards, Kyle

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[David Howells]

Kyle McMartin <kyle@redhat.com> wrote:

> After thinking about it a while, this seems like the best way to solve
> the problem, although it does still kind of offend my delicate
> sensibilities...
> 
> Doing this check in the crypto layer seems kind of like a layering
> violation to me (and, to be honest, I think it'd be a gross-hack getting
> from the callee back to the caller module.)
> 
> Instead, doing it in kernel/module.c and looking at the undefined symbol
> list again looks to me like an ordering problem since we're doing the
> signature check before we've even done the elf header check. We'd have
> to move the panic to a part of the module code that may not necessarily
> make sense.
> 
> Whereas checking the undefined symbol list at modpost time is fairly
> logical, and adding a new MODULE_INFO entry in the CONFIG_CRYPTO_FIPS
> case is a well understood thing to do, and should catch all the crypto
> registrations regardless of where they exist in-tree...
> 
> Seems to build and work with both values of CONFIG_CRYPTO_FIPS.
> 
> Thoughts?

You can't rely on someone trying to sneak a dodgy crypto module in to set the
flag when they build it.  The detection thus needs to be done in the kernel
during the module load.

Can you search the module image for "crypto_register_" I wonder?  If that's
there, it's a crypto module.

David

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Matthew Garrett]

On Fri, Jan 25, 2013 at 12:14:54AM +0000, David Howells wrote:

> You can't rely on someone trying to sneak a dodgy crypto module in to set the
> flag when they build it.  The detection thus needs to be done in the kernel
> during the module load.
> 
> Can you search the module image for "crypto_register_" I wonder?  If that's
> there, it's a crypto module.

If you're trying to protect against malice rather than accident, what's 
going to stop the module from just finding and modifying data structures 
itself? If you want to panic if you've just loaded something that might 
compromise your crypto implementations, you've got to panic on all 
unsigned module loads.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] MODSIGN: flag modules that use cryptoapi and only panic if those are unsigned

[Stephan Mueller]

On 25.01.2013 04:20:07, +0100, Matthew Garrett <mjg59@srcf.ucam.org> wrote:

Hi Matthew,
> On Fri, Jan 25, 2013 at 12:14:54AM +0000, David Howells wrote:
> 
>> You can't rely on someone trying to sneak a dodgy crypto module in to set the
>> flag when they build it.  The detection thus needs to be done in the kernel
>> during the module load.
>>
>> Can you search the module image for "crypto_register_" I wonder?  If that's
>> there, it's a crypto module.
> 
> If you're trying to protect against malice rather than accident, what's 
> going to stop the module from just finding and modifying data structures 
> itself? If you want to panic if you've just loaded something that might 
> compromise your crypto implementations, you've got to panic on all 
> unsigned module loads.

That is the issue here. We want to protect against accidental changes
and modifications. Malicious attacks will never be caught by the FIPS
requirements where a module is allowed to check itself.

If an attacker is able to load any kind of kernel module, we have lost.
Period.

Thus, from a FIPS point of view the latest patch from Kyle is also
appropriate, provided the concerns I raised there are covered.

Ciao
Stephan