From patchwork Mon Aug 6 21:19:32 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jann Horn X-Patchwork-Id: 10557965 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id B519E1390 for ; Mon, 6 Aug 2018 21:19:43 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9FDC728DA1 for ; Mon, 6 Aug 2018 21:19:43 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 90E1428E92; Mon, 6 Aug 2018 21:19:43 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9AE1B28DA1 for ; Mon, 6 Aug 2018 21:19:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729976AbeHFXaf (ORCPT ); Mon, 6 Aug 2018 19:30:35 -0400 Received: from mail-it0-f74.google.com ([209.85.214.74]:50932 "EHLO mail-it0-f74.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729749AbeHFXae (ORCPT ); Mon, 6 Aug 2018 19:30:34 -0400 Received: by mail-it0-f74.google.com with SMTP id n194-v6so13768363itn.0 for ; Mon, 06 Aug 2018 14:19:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:message-id:mime-version:subject:from:to:cc; bh=z1YkuEhJSa76Eh6u3qIGx1kWICWudnLuyFUEwJXWRR8=; b=X6m4ApkCej0KtfXj0K/xomJcEVwyvE3BUab/zxolddebuNo9pmkQ+yQJ9pybskcQV5 bUcREjvkgaih/xa7OkJp6eDqCC4+CzkuQDe1HTXbBEJ5DW0lzSWMfC9vUoFkFnZW0KN1 7A1XjyznOIZmvLREqoLFoaMnBRyNTrJAX814t0Xrdh/y+evhFMVRX7SwJjRithodK/9i JTJb3luhVs1lxFtw6ez0g+55isRrxNhbm1+OpuiLcezd2UrB0V4RyX1ReYsWpaHMSpmj Co8A87JMnxy2q/MsNwJwSmzdmTePJjE7XlKrQmahLWC0JRP0Bv6OZztDPPOsTdA5La5b LeAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:message-id:mime-version:subject:from:to:cc; bh=z1YkuEhJSa76Eh6u3qIGx1kWICWudnLuyFUEwJXWRR8=; b=HbZLWOIv0H1v/UwoK8WMXIEVhn+urjQ2l3It3rbRFrWTbCNdEizXV9PjUJQqNfEL/l NkvQln04Ov+4DEci0vfzE215PFGiFqNdPmlo0MNKG4XWQuDR9a+y8VH5fiMgjs6hKShV 1tCXAogFoetg6DPSN0X3UoGhF2L20plolsYe73mNknTaBnsyCIUo4OswAbCgvLZTmcy2 O4J5iUqJQu7fo/GcVof2hTa5qsVlDnIY6QIsTp3l/eRSg+dTmc9R5gME41huhZDAlI/y B4ipQPHlDxzI+vgJy33+t16GeHyN9XsnC9/rhO9/D2BW/qdLHHJHQHFwwehOy7+7WXNf u+aA== X-Gm-Message-State: AOUpUlGUwziO4v772TPVgXBpkN05a+DPoWBXR6K6euNauAWK0Yu8C7bH ZRravlmkfnd4KYjlEzEXqSEJ1v+7Nw== X-Google-Smtp-Source: AA+uWPw3+e8RGAdyw6tf8Pah9CMPbjOwRpDCivdq588qJ4+zclQU0zS0rI2HkgtYsMRzwueN2QDwtoJ8eg== X-Received: by 2002:a6b:b251:: with SMTP id b78-v6mr8303163iof.128.1533590378252; Mon, 06 Aug 2018 14:19:38 -0700 (PDT) Date: Mon, 6 Aug 2018 23:19:32 +0200 Message-Id: <20180806211932.198488-1-jannh@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.18.0.597.ga71716f1ad-goog Subject: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter From: Jann Horn To: Stephen Smalley , Paul Moore , Eric Paris , selinux@tycho.nsa.gov, jannh@google.com Cc: James Morris , "Serge E. Hallyn" , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP The intended behavior change for this patch is to reject any MLS strings that contain (trailing) garbage if p->mls_enabled is true. As suggested by Paul Moore, change mls_context_to_sid() so that the two parts of the range are extracted before the rest of the parsing. Because now we don't have to scan for two different separators simultaneously everywhere, we can actually switch to strchr() everywhere instead of the open-coded loops that scan for two separators at once. mls_context_to_sid() used to signal how much of the input string was parsed by updating `*scontext`. However, there is actually no case in which mls_context_to_sid() only parses a subset of the input and still returns a success (other than the buggy case with a second '-' in which it incorrectly claims to have consumed the entire string). Turn `scontext` into a simple pointer argument and stop redundantly checking whether the entire input was consumed in string_to_context_struct(). This also lets us remove the `scontext_len` argument from `string_to_context_struct()`. Signed-off-by: Jann Horn --- Refactored version of "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on Paul's comments. WDYT? I've thrown some inputs at it, and it seems to work. security/selinux/ss/mls.c | 178 ++++++++++++++------------------- security/selinux/ss/mls.h | 2 +- security/selinux/ss/services.c | 12 +-- 3 files changed, 82 insertions(+), 110 deletions(-) diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c index 39475fb455bc..2fe459df3c85 100644 --- a/security/selinux/ss/mls.c +++ b/security/selinux/ss/mls.c @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c) /* * Set the MLS fields in the security context structure * `context' based on the string representation in - * the string `*scontext'. Update `*scontext' to - * point to the end of the string representation of - * the MLS fields. + * the string `scontext'. * * This function modifies the string in place, inserting * NULL characters to terminate the MLS fields. @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct context *c) */ int mls_context_to_sid(struct policydb *pol, char oldc, - char **scontext, + char *scontext, struct context *context, struct sidtab *s, u32 def_sid) { - - char delim; - char *scontextp, *p, *rngptr; + char *sensitivity, *cur_cat, *next_cat, *rngptr; struct level_datum *levdatum; struct cat_datum *catdatum, *rngdatum; - int l, rc = -EINVAL; + int l, rc, i; + char *rangep[2]; if (!pol->mls_enabled) { - if (def_sid != SECSID_NULL && oldc) - *scontext += strlen(*scontext) + 1; - return 0; + if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0') + return 0; + return -EINVAL; } /* @@ -261,113 +258,94 @@ int mls_context_to_sid(struct policydb *pol, struct context *defcon; if (def_sid == SECSID_NULL) - goto out; + return -EINVAL; defcon = sidtab_search(s, def_sid); if (!defcon) - goto out; + return -EINVAL; - rc = mls_context_cpy(context, defcon); - goto out; + return mls_context_cpy(context, defcon); } - /* Extract low sensitivity. */ - scontextp = p = *scontext; - while (*p && *p != ':' && *p != '-') - p++; - - delim = *p; - if (delim != '\0') - *p++ = '\0'; + /* + * If we're dealing with a range, figure out where the two parts + * of the range begin. + */ + rangep[0] = scontext; + rangep[1] = strchr(scontext, '-'); + if (rangep[1]) { + rangep[1][0] = '\0'; + rangep[1]++; + } + /* For each part of the range: */ for (l = 0; l < 2; l++) { - levdatum = hashtab_search(pol->p_levels.table, scontextp); - if (!levdatum) { - rc = -EINVAL; - goto out; - } + /* Split sensitivity and category set. */ + sensitivity = rangep[l]; + if (sensitivity == NULL) + break; + next_cat = strchr(sensitivity, ':'); + if (next_cat) + *(next_cat++) = '\0'; + /* Parse sensitivity. */ + levdatum = hashtab_search(pol->p_levels.table, sensitivity); + if (!levdatum) + return -EINVAL; context->range.level[l].sens = levdatum->level->sens; - if (delim == ':') { - /* Extract category set. */ - while (1) { - scontextp = p; - while (*p && *p != ',' && *p != '-') - p++; - delim = *p; - if (delim != '\0') - *p++ = '\0'; - - /* Separate into range if exists */ - rngptr = strchr(scontextp, '.'); - if (rngptr != NULL) { - /* Remove '.' */ - *rngptr++ = '\0'; - } + /* Extract category set. */ + while (next_cat != NULL) { + cur_cat = next_cat; + next_cat = strchr(next_cat, ','); + if (next_cat != NULL) + *(next_cat++) = '\0'; + + /* Separate into range if exists */ + rngptr = strchr(cur_cat, '.'); + if (rngptr != NULL) { + /* Remove '.' */ + *rngptr++ = '\0'; + } - catdatum = hashtab_search(pol->p_cats.table, - scontextp); - if (!catdatum) { - rc = -EINVAL; - goto out; - } + catdatum = hashtab_search(pol->p_cats.table, cur_cat); + if (!catdatum) + return -EINVAL; - rc = ebitmap_set_bit(&context->range.level[l].cat, - catdatum->value - 1, 1); - if (rc) - goto out; - - /* If range, set all categories in range */ - if (rngptr) { - int i; - - rngdatum = hashtab_search(pol->p_cats.table, rngptr); - if (!rngdatum) { - rc = -EINVAL; - goto out; - } - - if (catdatum->value >= rngdatum->value) { - rc = -EINVAL; - goto out; - } - - for (i = catdatum->value; i < rngdatum->value; i++) { - rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1); - if (rc) - goto out; - } - } + rc = ebitmap_set_bit(&context->range.level[l].cat, + catdatum->value - 1, 1); + if (rc) + return rc; + + /* If range, set all categories in range */ + if (rngptr == NULL) + continue; + + rngdatum = hashtab_search(pol->p_cats.table, rngptr); + if (!rngdatum) + return -EINVAL; + + if (catdatum->value >= rngdatum->value) + return -EINVAL; - if (delim != ',') - break; + for (i = catdatum->value; i < rngdatum->value; i++) { + rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1); + if (rc) + return rc; } } - if (delim == '-') { - /* Extract high sensitivity. */ - scontextp = p; - while (*p && *p != ':') - p++; - - delim = *p; - if (delim != '\0') - *p++ = '\0'; - } else - break; } - if (l == 0) { + /* If we didn't see a '-', the range start is also the range end. */ + if (rangep[1] == NULL) { context->range.level[1].sens = context->range.level[0].sens; rc = ebitmap_cpy(&context->range.level[1].cat, &context->range.level[0].cat); if (rc) - goto out; + return rc; } - *scontext = ++p; - rc = 0; -out: - return rc; + + return 0; } /* @@ -379,21 +357,19 @@ int mls_context_to_sid(struct policydb *pol, int mls_from_string(struct policydb *p, char *str, struct context *context, gfp_t gfp_mask) { - char *tmpstr, *freestr; + char *tmpstr; int rc; if (!p->mls_enabled) return -EINVAL; - /* we need freestr because mls_context_to_sid will change - the value of tmpstr */ - tmpstr = freestr = kstrdup(str, gfp_mask); + tmpstr = kstrdup(str, gfp_mask); if (!tmpstr) { rc = -ENOMEM; } else { - rc = mls_context_to_sid(p, ':', &tmpstr, context, + rc = mls_context_to_sid(p, ':', tmpstr, context, NULL, SECSID_NULL); - kfree(freestr); + kfree(tmpstr); } return rc; diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h index 9a3ff7af70ad..67093647576d 100644 --- a/security/selinux/ss/mls.h +++ b/security/selinux/ss/mls.h @@ -34,7 +34,7 @@ int mls_level_isvalid(struct policydb *p, struct mls_level *l); int mls_context_to_sid(struct policydb *p, char oldc, - char **scontext, + char *scontext, struct context *context, struct sidtab *s, u32 def_sid); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index dd2ceec06fef..9212d4dd817a 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -1367,7 +1367,6 @@ int security_sid_to_context_force(struct selinux_state *state, u32 sid, static int string_to_context_struct(struct policydb *pol, struct sidtab *sidtabp, char *scontext, - u32 scontext_len, struct context *ctx, u32 def_sid) { @@ -1428,15 +1427,12 @@ static int string_to_context_struct(struct policydb *pol, ctx->type = typdatum->value; - rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid); + rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid); if (rc) goto out; - rc = -EINVAL; - if ((p - scontext) < scontext_len) - goto out; - /* Check the validity of the new context. */ + rc = -EINVAL; if (!policydb_context_isvalid(pol, ctx)) goto out; rc = 0; @@ -1491,7 +1487,7 @@ static int security_context_to_sid_core(struct selinux_state *state, policydb = &state->ss->policydb; sidtab = &state->ss->sidtab; rc = string_to_context_struct(policydb, sidtab, scontext2, - scontext_len, &context, def_sid); + &context, def_sid); if (rc == -EINVAL && force) { context.str = str; context.len = strlen(str) + 1; @@ -1959,7 +1955,7 @@ static int convert_context(u32 key, goto out; rc = string_to_context_struct(args->newp, NULL, s, - c->len, &ctx, SECSID_NULL); + &ctx, SECSID_NULL); kfree(s); if (!rc) { printk(KERN_INFO "SELinux: Context %s became valid (mapped).\n",