linux-kernel - Re: [RFC] module: signature infrastructure

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: <CAKi4VA+R0EY9OteOCc=JGOqtshAJd5_CWu9tyJqjjkvG6qZUjw@mail.gmail.com>
Date:	Wed, 5 Sep 2012 20:41:44 -0300
From:	Lucas De Marchi <lucas.de.marchi@...il.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	David Howells <dhowells@...hat.com>, dmitry.kasatkin@...el.com,
	zohar@...ux.vnet.ibm.com, jmorris@...ei.org,
	keyrings@...ux-nfs.org, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC] module: signature infrastructure

On Tue, Sep 4, 2012 at 9:19 PM, Rusty Russell <rusty@...tcorp.com.au> wrote:
> Lucas De Marchi <lucas.de.marchi@...il.com> writes:
>> Hi Rusty,
>>
>> On Tue, Sep 4, 2012 at 2:55 AM, Rusty Russell <rusty@...tcorp.com.au> wrote:
>>> @@ -2399,7 +2437,50 @@ static inline void kmemleak_load_module(const struct module *mod,
>>>  }
>>>  #endif
>>>
>>> -/* Sets info->hdr and info->len. */
>>> +#ifdef CONFIG_MODULE_SIG
>>> +static int module_sig_check(struct load_info *info,
>>> +                           void *mod, unsigned long *len)
>>> +{
>>> +       int err;
>>> +       unsigned long i, siglen;
>>> +       char *sig = NULL;
>>> +
>>> +       /* This is not a valid module: ELF header is larger anyway. */
>>> +       if (*len < sizeof(MODULE_SIG_STRING))
>>> +               return -ENOEXEC;
>>> +
>>> +       for (i = 0; i < *len - (sizeof(MODULE_SIG_STRING)-1); i++) {
>>> +               /* Our memcmp is dumb, speed it up a little. */
>>> +               if (((char *)mod)[i] != MODULE_SIG_STRING[0])
>>> +                       continue;
>>
>> Since the signature is appended to the module, why don't you go
>> backwards, starting from *len - strlen(sizeof(MODULE_SIG_STRING)) and
>> making this first comparison?
>
> We've had this discussion multiple times.  Simple wins.  It's so
> marginal, I don't really care, but I've changed it to:

Sorry to come up with this suggestion only now (and after you have
already talked to me at LPC). Only after seeing this implementation I
thought about the implications of having the module signed in this
format.

I'm worried about performance here. Module loading can take a fair
amount of boot time. It may not be critical for servers or desktops
that we rarely boot, but it is for embedded uses.

>         int err;
>         unsigned long i, siglen, markerlen;
>         char *sig = NULL;
>
>         markerlen = strlen(MODULE_SIG_STRING);
>         /* This is not a valid module: ELF header is larger anyway. */
>         if (*len < markerlen)
>                 return -ENOEXEC;
>
>         for (i = *len - markerlen; i > 0; i--) {
>                 /* Our memcmp is dumb, speed it up a little. */
>                 if (((char *)mod)[i] != MODULE_SIG_STRING[0])
>                         continue;
>                 if (memcmp(mod+i, MODULE_SIG_STRING, markerlen))
>                         continue;
>
>                 sig = mod + i + markerlen;
>                 siglen = *len - i - markerlen;
>                 *len = i;
>                 break;
>         }
>
> We could also implement memrchr(), or memrmem().  Hell, if we had
> memmem() in the kernel I'd gladly use it.
>
>> Or let the magic string as the last thing in the module and store the
>> signature length, too. In this case no scanning is needed
>
> Yes, they did that too, but append is simpler.  I don't even have to
> think about endianness (Dmitry chose be32) or parsing (David chose
> 5-digit ascii numeric encoding).

Letting it in be32 is the simplest solution IMO. it's way simpler then
the loop above. You have to check exactly 1 byte to have a first
decision if module is not signed (as opposed to scanning the entire
module). Then you compare the memory area with MODULE_SIG_STRING and
have a final decision. To get the signature length it is just a matter
of converting it to host endiannes.  And we do have functions for
doing that. If it's for simplicity in kernel side, this one could be
implemented in half of the code above.

>
> Scanning the module is the least of our issues since we've just copied
> it and we're about to SHA it.

Yeah, but I don't think we need to scan it one more time. On every
boot. For every module


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/