Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932298Ab3CDSlK (ORCPT ); Mon, 4 Mar 2013 13:41:10 -0500 Received: from mail-oa0-f47.google.com ([209.85.219.47]:35027 "EHLO mail-oa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932262Ab3CDSlF (ORCPT ); Mon, 4 Mar 2013 13:41:05 -0500 MIME-Version: 1.0 In-Reply-To: <87boazgh5j.fsf@xmission.com> References: <20130303005700.GA32213@austin.hallyn.com> <20130303035608.GA2703@austin.hallyn.com> <20130304082959.GA22087@r00tworld.net> <87boazgh5j.fsf@xmission.com> Date: Mon, 4 Mar 2013 10:41:04 -0800 Message-ID: Subject: Re: user ns: arbitrary module loading From: Kees Cook To: "Eric W. Biederman" Cc: Mathias Krause , "Serge E. Hallyn" , LKML , Serge Hallyn , Brad Spengler , Al Viro , Eric Paris , Rusty Russell , PaX Team , Herbert Xu , "David S. Miller" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3891 Lines: 115 On Mon, Mar 4, 2013 at 10:21 AM, Eric W. Biederman wrote: > Kees Cook writes: > >> On Mon, Mar 4, 2013 at 12:29 AM, Mathias Krause 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 >>> #include >>> #include >>> #include >>> #include >>> #include >>> >>> #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 Something like that, but that'll break some things that do stuff like %s-suffix. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/