MODSIGN: only panic in fips mode if sig_enforce is set

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 <[email protected]>

Signed-off-by Kyle McMartin <[email protected]>

kernel/module.c +2 -2

@@ -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;
 }

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 <[email protected]>

Signed-off-by Kyle McMartin <[email protected]>

kernel/module.c +2 -2

@@ -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;
 }

Quoted 12 lines · 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.)

Quoted 5 lines

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 <[email protected]>

Signed-off-by Kyle McMartin <[email protected]>

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

David

Quoted 16 lines

kernel/module.c +2 -2

@@ -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;
 }

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.

Quoted 4 lines

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

Quoted 3 lines

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

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

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...

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.

Quoted 3 lines · 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...

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

Quoted 3 lines · 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...

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

Quoted 3 lines · Stephan Mueller

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.

Quoted 3 lines · Rusty Russell

"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.

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. :-)

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 <[email protected]>

kernel/module.c +4 -2

@@ -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)

scripts/mod/modpost.c +18 -0

index ff36c50..79f46e2 100644
@@ -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);

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 <[email protected]>

---

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

crypto/algapi.c +5 -0

 			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;

crypto/fips.c +0 -0

index 5539700..2ebbe0f 100644
@@ -15,6 +15,19 @@
 int fips_enabled;

crypto/fips.c +5 -0

+#else
+	return 1;
+#endif
+}
+
 /* Process kernel command-line parameter at boot time. fips=0 or fips=1 */
 static int fips_enable(char *str)
 {

crypto/internal.h +9 -0

index 9ebedae..937bfaf 100644
@@ -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 */
 

On 05.02.2013 23:58:30, +0100, Kyle McMartin <[email protected]> wrote:

Hi Kyle,

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.

Quoted 1 line

crypto/algapi.c static int crypto_check_alg(struct crypto_alg *alg) · line 56

Quoted 4 lines

crypto_sig_check should return 1 (and allow the registration) if any of the following are true:

crypto/algapi.c +2 -0

+	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.

crypto/algapi.c int crypto_register_template(struct crypto_template *tmpl)

Quoted 5 lines

crypto/algapi.c +1 -0

+
 	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.

crypto/algapi.c int crypto_register_template(struct crypto_template *tmpl) · line 439

Quoted 2 lines

crypto/algapi.c +0 -0

 			goto out;
 	}

Quoted 4 lines · Kyle McMartin

Hi Kyle,

Thanks for the review, Stephan.

Quoted 15 lines

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.

Quoted 1 line

Quoted 8 lines

crypto/algapi.c static int crypto_check_alg(struct crypto_alg *alg) · line 56

Quoted 4 lines

crypto_sig_check should return 1 (and allow the registration) if any of the following are true:

crypto/algapi.c +2 -0

+	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.

Quoted 5 lines

Quoted 5 lines

crypto/algapi.c +1 -0

+
 	return crypto_set_driver_name(alg);
 }

Quoted 7 lines

Review int crypto_register_template(struct crypto_template *tmpl)

Quoted 1 line

(quoted diff) +0 -0

@@ -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.

Quoted 3 lines

crypto/algapi.c int crypto_register_template(struct crypto_template *tmpl) · line 439

Quoted 3 lines

crypto/algapi.c +0 -0

 			goto out;
 	}

regards, Kyle

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.

Quoted 2 lines

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.