Re: user ns: arbitrary module loading

[ebiederm@xmission.com (Eric W. Biederman)]

Kees Cook <keescook@google.com> writes:

> On Mon, Mar 4, 2013 at 12:29 AM, Mathias Krause <minipli@googlemail.com> wrote:
>> On Sun, Mar 03, 2013 at 09:48:50AM -0800, Kees Cook wrote:
>>> Several subsystems already have an implicit subsystem restriction
>>> because they load with aliases. (e.g. binfmt-XXXX, net-pf=NNN,
>>> snd-card-NNN, FOO-iosched, etc). This isn't the case for filesystems
>>> and a few others, unfortunately:
>>>
>>> $ git grep 'request_module("%.*s"' | grep -vi prefix
>>> crypto/api.c:           request_module("%s", name);
>>>
>>> [...]
>>>
>>> Several of these come from hardcoded values, though (e.g. crypto, chipreg).
>>
>> Well, crypto does not. Try the code snippet below on a system with
>> CONFIG_CRYPTO_USER_API=y. It'll abuse the above request_module() call
>> to load any module the user requests -- iregardless of being contained
>> in a user ns or not.
>
> Oh ew. Yeah, I must have missed the path through the user api. Arg.

I will let someone else write the patch that adds the module aliases to
crypto.

It seems worth doing even outside of any security concerns as it just
makes the reqest to modprobe make more sense, and allows the existing
modprobe policy controls to work.

Whereas an ill-formed string just doesn't tell modprobe enough to really
act intelligently.

>> ---8<---
>> /* Loading arbitrary modules using crypto api since v2.6.38
>>  *
>>  * - minipli
>>  */
>> #include <linux/if_alg.h>
>> #include <sys/socket.h>
>> #include <unistd.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <stdio.h>
>>
>> #ifndef AF_ALG
>> #define AF_ALG 38
>> #endif
>>
>>
>> int main(int argc, char **argv) {
>>         struct sockaddr_alg sa_alg = {
>>                 .salg_family = AF_ALG,
>>                 .salg_type = "hash",
>>         };
>>         int sock;
>>
>>         if (argc != 2) {
>>                 printf("usage: %s MODULE_NAME\n", argv[0]);
>>                 exit(1);
>>         }
>>
>>         sock = socket(AF_ALG, SOCK_SEQPACKET, 0);
>>         if (sock < 0) {
>>                 perror("socket(AF_ALG)");
>>                 exit(1);
>>         }
>>
>>         strncpy((char *) sa_alg.salg_name, argv[1], sizeof(sa_alg.salg_name));
>>         bind(sock, (struct sockaddr *) &sa_alg, sizeof(sa_alg));
>>         close(sock);
>>
>>         return 0;
>> }
>> --->8---
>>
>> If people care about unprivileged users not being able to load arbitrary
>> modules, could someone please fix this in crypto API, then? Herbert?
>
> So, should this get a prefix too?  Maybe we need to change the
> request_module primitive to request_module(prefix, fmt, args) to stop
> these request_module("%s", name) things from continuing to exist...

Something like the patch below?

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 56dd349..859aa3a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -131,6 +131,10 @@ int __request_module(bool wait, const char *fmt, ...)
 #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */
        static int kmod_loop_msg;
 
+       /* Require that calls to request module have a little structure */
+       if (fmt[0] == '%')
+               return -EINVAL;
+
        /*
         * We don't allow synchronous module loading from async.  Module
         * init may invoke async_synchronize_full() which will end up