linux-kernel - Re: [PATCH] ima: drop vla in ima_audit

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]

Date:   Thu, 8 Mar 2018 12:04:31 -0700
From:   Tycho Andersen <tycho@...ho.ws>
To:     Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        linux-integrity@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH] ima: drop vla in ima_audit_measurement()

On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote:
> On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote:
> > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen <tycho@...ho.ws> wrote:
> > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> > > > from ima_audit_measurement(). We need to adjust the return type of
> > > > ima_audit_measurement, because now this function can fail if an allocation
> > > > fails.
> > > 
> > > 
> > > 
> > > > +       algo_hash_len = hash_len + strlen(algo_name) + 2;
> > > > +       algo_hash = kzalloc(algo_hash_len, GFP_KERNEL);
> > > 
> > > > -       snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> > > > +       snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > > 
> > > kasprintf() ?
> > 
> > Sure, in fact I think we could just do:
> > 
> > -	snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash);
> > -	audit_log_untrustedstring(ab, algo_hash);
> > +	audit_log_untrustedstring(ab, algo_name);
> > +	audit_log_format(ab, ":");
> > +	audit_log_untrustedstring(ab, hash);
> > 
> > and get rid of the allocation entirely. I'll test and make sure it
> > works and then re-send.
> 
> The hash algorithm name is an enumeration that comes from the kernel.
>  It's defined in crypto/hash_info.c: hash_algo_name.  Why do we need
> to use audit_log_untrustedstring()?

Yes, I suppose we don't need it for the hash either, since we're
generating that and we know it's just hex digits and not any audit
control characters or "s or anything.

It looks like we could get rid of the other allocation too by just
using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the
hex_byte_pack that's currently in use in this function. Is that too
much of a breakage?

Tycho