Re: [PATCH] Revert "crypto: pkcs7 - remove sha1 support"

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Thu Mar 14, 2024 at 1:32 AM EET, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> This reverts commit 16ab7cb5825fc3425c16ad2c6e53d827f382d7c6 because it
> broke iwd.  iwd uses the KEYCTL_PKEY_* UAPIs via its dependency libell,
> and apparently it is relying on SHA-1 signature support.  These UAPIs
> are fairly obscure, and their documentation does not mention which
> algorithms they support.  iwd really should be using a properly
> supported userspace crypto library instead.  Regardless, since something
> broke we have to revert the change.
>
> It may be possible that some parts of this commit can be reinstated
> without breaking iwd (e.g. probably the removal of MODULE_SIG_SHA1), but
> for now this just does a full revert to get things working again.
>
> Reported-by: Karel Balej <balejk@matfyz.cz>
> Closes: https://lore.kernel.org/r/CZSHRUIJ4RKL.34T4EASV5DNJM@matfyz.cz
> Cc: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/asymmetric_keys/mscode_parser.c    |  3 +
>  crypto/asymmetric_keys/pkcs7_parser.c     |  4 ++
>  crypto/asymmetric_keys/public_key.c       |  3 +-
>  crypto/asymmetric_keys/signature.c        |  2 +-
>  crypto/asymmetric_keys/x509_cert_parser.c |  8 +++
>  crypto/testmgr.h                          | 80 +++++++++++++++++++++++
>  include/linux/oid_registry.h              |  4 ++
>  kernel/module/Kconfig                     |  5 ++
>  8 files changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/mscode_parser.c b/crypto/asymmetric_keys/mscode_parser.c
> index 05402ef8964e..8aecbe4637f3 100644
> --- a/crypto/asymmetric_keys/mscode_parser.c
> +++ b/crypto/asymmetric_keys/mscode_parser.c
> @@ -73,10 +73,13 @@ int mscode_note_digest_algo(void *context, size_t hdrlen,
>  	char buffer[50];
>  	enum OID oid;
>  
>  	oid = look_up_OID(value, vlen);
>  	switch (oid) {
> +	case OID_sha1:
> +		ctx->digest_algo = "sha1";
> +		break;

I fully agree with the change BUT...

IMHO it would make sense to e.g either add inline comment about iwd
dependency or link to the bug report here.

I'd like to think that there is common will to eventually get rid of
all of SHA-1, and thus in cases where it is not yet possible it would
make sense to guide what to needs to be done to make it happen, right?

BR, Jarkko