On 05.02.2013 23:58:30, +0100, Kyle McMartin <k...@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 <k...@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 */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
