lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <db5890d8-3a3d-4ca7-bb58-655c26164587@t-8ch.de>
Date:   Mon, 7 Nov 2022 16:55:46 +0100
From:   Thomas Weißschuh <linux@...ssschuh.net>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     David Howells <dhowells@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Jarkko Sakkinen <jarkko@...nel.org>, keyrings@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Mark Pearson <markpearson@...ovo.com>,
        Eric Snowberg <eric.snowberg@...cle.com>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH] certs: Prevent spurious errors on repeated blacklisting

On 2022-11-07 14:12+0100, Mickaël Salaün wrote:
> This is a follow-up of
> https://lore.kernel.org/r/c8c65713-5cda-43ad-8018-20f2e32e4432@t-8ch.de
> 
> Added Jarkko, Mark Pearson, Eric Snowberg and more ML in Cc.
> 
> 
> On 04/11/2022 02:47, Thomas Weißschuh wrote:
> > When the blacklist keyring was changed to allow updates from the root
> > user it gained an ->update() function that disallows all updates.
> > When the a hash is blacklisted multiple times from the builtin or
> > firmware-provided blacklist this spams prominent logs during boot:
> > 
> > [    0.890814] blacklist: Problem blacklisting hash (-13)
> > 
> > As all these repeated calls to mark_raw_hash_blacklisted() would create
> > the same keyring entry again anyways these errors can be safely ignored.
> 
> These errors can indeed be safely ignored, however they highlight issues
> with some firmware vendors not checking nor optimizing their blocked hashes.
> This raises security concerns, and it should be fixed by firmware vendors.

Thanks, I was not aware that these are worth fixing.

> > Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring")
> > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > ---
> >   certs/blacklist.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > index 41f10601cc72..5f7f2882ced7 100644
> > --- a/certs/blacklist.c
> > +++ b/certs/blacklist.c
> > @@ -191,7 +191,9 @@ static int mark_raw_hash_blacklisted(const char *hash)
> >   				   BLACKLIST_KEY_PERM,
> >   				   KEY_ALLOC_NOT_IN_QUOTA |
> >   				   KEY_ALLOC_BUILT_IN);
> > -	if (IS_ERR(key)) {
> > +
> > +	/* Blacklisting the same hash twice fails but would be idempotent */
> > +	if (IS_ERR(key) && PTR_ERR(key) != -EACCES) {
> 
> We should not hide EACCES errors. This logs issues, which is correct for
> duplicate hashes, and can help firmware vendors to fix their database. I'd
> really like to see a different log message instead: change the duplicate
> entry error code from EACCES to EEXIST, and call pr_warn for this specific
> case.

Returning EACCES would require some deeper changes to how the keyring is set up
or even changes to the keyring core itself to introduce a key_create() (without
update) function.

Is this something you would take a look at, or should I try to do it?
(I have no previous knowledge about the keyring subsystem)

In any case it probably would also be good to log the problematic hashes
themselves, so users can properly report the issue to their firmware vendors.

> >   		pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> >   		return PTR_ERR(key);
> >   	}
> > 
> > base-commit: ee6050c8af96bba2f81e8b0793a1fc2f998fcd20

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ