linux-kernel - Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2937432.1648647840@warthog.procyon.org.uk>
Date:   Wed, 30 Mar 2022 14:44:00 +0100
From:   David Howells <dhowells@...hat.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>
Cc:     dhowells@...hat.com,
        =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= <mic@...ikod.net>,
        David Woodhouse <dwmw2@...radead.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Snowberg <eric.snowberg@...cle.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        James Morris <jmorris@...ei.org>,
        =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= <mic@...ux.microsoft.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Tyler Hicks <tyhicks@...ux.microsoft.com>,
        keyrings@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

Jarkko Sakkinen <jarkko@...nel.org> wrote:

> >  /*
> >   * Initialise the blacklist
> >   */
> >  static int __init blacklist_init(void)
> >  {
> >  	const char *const *bl;
> > +	struct key_restriction *restriction;
> >  
> >  	if (register_key_type(&key_type_blacklist) < 0)
> >  		panic("Can't allocate system blacklist key type\n");
> >  
> > +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > +	if (!restriction)
> > +		panic("Can't allocate blacklist keyring restriction\n");
> 
> 
> This prevents me from taking this to my pull request. In moderns standards,
> no new BUG_ON(), panic() etc. should never added to the kernel.

I would argue that in this case, though, it is reasonable.  This should only
be called during kernel initialisation and, as Mickaël points out, if you
can't allocate that small amount of memory, the kernel isn't going to boot
much further.

> I missed this in my review.
> 
> This should rather be e.g.
> 
>         restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> 	if (!restriction) {
> 		pr_err("Can't allocate blacklist keyring restriction\n");
>                 return 0;
>         }

You can't just return 0.  That indicates success - but if by some miracle, the
kernel actually gets to a point where userspace can happen, it could mean that
we're missing the security restrictions of the blacklist.

Now, we could defer the panic to add_key_to_revocation_list(), but if you
can't set in place the required security restrictions, I think it's arguable
that the kernel either needs to panic or it needs to blacklist everything.

David