From patchwork Thu Mar 6 22:43:03 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Konstantin Andreev X-Patchwork-Id: 14005467 Received: from mx.swemel.ru (mx.swemel.ru [95.143.211.150]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 16D15A2D for ; Thu, 6 Mar 2025 22:52:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.143.211.150 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741301545; cv=none; b=hOrNXCNNlfltmXGElaPN2SxRbvhRB4Vd2miQ5DBVBEKHj6vV5nEPt4bJeqIQYTGMPiPr1aPWD5k6MVjXhBBcjmk+tOdRuFfOtqwxklFdaDr+8P/LDIUmYx7vSMzw3/NEz0FzhvtKZa4ArAfxG7DQdJPV2HZPx+K0L8htYIuTQU0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741301545; c=relaxed/simple; bh=dIrmdcmdbRHcuOLJxLFhM3ssmhjjLXqMA2hmVESpURY=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=e3qp/4SdyQWoNXP15avpxrgNo08oX0FrrIMPeZqgBOqI8oYPfuHjAHHsLUBo76bIjgQXIV0QU3B/BfmMGqFrnvaxRh5f+emRnAV/1YD8tehJwd+vEuH2fsMsTO+WWZOGU4u07nSZ0/PmRwTFSHLdXWqDY4Py4tEA07VVYAQfM5k= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=swemel.ru; spf=pass smtp.mailfrom=swemel.ru; dkim=pass (1024-bit key) header.d=swemel.ru header.i=@swemel.ru header.b=sK9w1iHs; arc=none smtp.client-ip=95.143.211.150 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=swemel.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=swemel.ru Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=swemel.ru header.i=@swemel.ru header.b="sK9w1iHs" From: Konstantin Andreev DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=swemel.ru; s=mail; t=1741301007; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jLvKT26VjkdQWYjc+ogdfcQfoT1aOWvibvbjn/SOy58=; b=sK9w1iHsHbSDQS8EGPZ55r7fDcV1BP7aNLM+xQVqaaPFSKSHZ1QSsmDoJOC7YndMyCGcWf 4IFi1wS0M/9BUpXx8JYlI1tp48fT7KrMZooz31MLaGWhFmFvBj1oDng6ktuCO+iwjOzv1y 0EnvE328/lXzTwBCCVUbCv3Dh+GJeTU= To: Casey Schaufler Cc: linux-security-module@vger.kernel.org Subject: [PATCH 2/2] smack: fix bug: setting task label silently ignores input garbage Date: Fri, 7 Mar 2025 01:43:03 +0300 Message-ID: <20250306224317.416365-3-andreev@swemel.ru> In-Reply-To: <20250306224317.416365-1-andreev@swemel.ru> References: <20250306224317.416365-1-andreev@swemel.ru> Precedence: bulk X-Mailing-List: linux-security-module@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 The command: # echo foo/bar >/proc/$$/attr/smack/current gives the task a label 'foo' w/o indication that label does not match input. Setting the label with lsm_set_self_attr() syscall behaves identically. This occures because: 1) smk_parse_smack() is used to convert input to a label 2) smk_parse_smack() takes only that part from the beginning of the input that looks like a label. 3) `/' is prohibited in labels, so only "foo" is taken. (2) is by design, because smk_parse_smack() is used for parsing strings which are more than just a label. Silent failure is not a good thing, and there are two indicators that this was not done intentionally: (size >= SMK_LONGLABEL) ~> invalid clause at the beginning of the do_setattr() and the "Returns the length of the smack label" claim in the do_setattr() description. So I fixed this by adding one tiny check: the taken label length == input length. Since input length is now strictly controlled, I changed the two ways of setting label smack_setselfattr(): lsm_set_self_attr() syscall smack_setprocattr(): > /proc/.../current to accommodate the divergence in what they understand by "input length": smack_setselfattr counts mandatory \0 into input length, smack_setprocattr does not. smack_setprocattr allows one trailing \n after label by common agreement for /proc/... interfaces. Related changes: * fixed description for smk_parse_smack * allow unprivileged tasks validate label syntax. * extract smk_parse_label_len() from smk_parse_smack() so parsing may be done w/o string allocation. * extract smk_import_valid_label() from smk_import_entry() to avoid repeated parsing. * smk_parse_smack(): scan null-terminated strings for no more than SMK_LONGLABEL(256) characters * smack_setselfattr(): require struct lsm_ctx . flags == 0 to reserve them for future. Fixes: e114e473771c ("Smack: Simplified Mandatory Access Control Kernel") Signed-off-by: Konstantin Andreev --- security/smack/smack.h | 3 + security/smack/smack_access.c | 93 ++++++++++++++++++++++-------- security/smack/smack_lsm.c | 103 ++++++++++++++++++++-------------- 3 files changed, 136 insertions(+), 63 deletions(-) diff --git a/security/smack/smack.h b/security/smack/smack.h index bf6a6ed3946c..759343a6bbae 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -286,9 +286,12 @@ int smk_tskacc(struct task_smack *, struct smack_known *, int smk_curacc(struct smack_known *, u32, struct smk_audit_info *); int smack_str_from_perm(char *string, int access); struct smack_known *smack_from_secid(const u32); +int smk_parse_label_len(const char *string, int len); char *smk_parse_smack(const char *string, int len); int smk_netlbl_mls(int, char *, struct netlbl_lsm_secattr *, int); struct smack_known *smk_import_entry(const char *, int); +struct smack_known *smk_import_valid_label(const char *label, int label_len, + gfp_t gfp); void smk_insert_entry(struct smack_known *skp); struct smack_known *smk_find_entry(const char *); bool smack_privileged(int cap); diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c index 2e4a0cb22782..a289cb6672bd 100644 --- a/security/smack/smack_access.c +++ b/security/smack/smack_access.c @@ -443,19 +443,19 @@ struct smack_known *smk_find_entry(const char *string) } /** - * smk_parse_smack - parse smack label from a text string - * @string: a text string that might contain a Smack label - * @len: the maximum size, or zero if it is NULL terminated. + * smk_parse_label_len - calculate the length of the starting segment + * in the string that constitutes a valid smack label + * @string: a text string that might contain a Smack label at the beginning + * @len: the maximum size to look into, may be zero if string is null-terminated * - * Returns a pointer to the clean label or an error code. + * Returns the length of the segment (0 < L < SMK_LONGLABEL) or an error code. */ -char *smk_parse_smack(const char *string, int len) +int smk_parse_label_len(const char *string, int len) { - char *smack; int i; - if (len <= 0) - len = strlen(string) + 1; + if (len <= 0 || len > SMK_LONGLABEL) + len = SMK_LONGLABEL; /* * Reserve a leading '-' as an indicator that @@ -463,7 +463,7 @@ char *smk_parse_smack(const char *string, int len) * including /smack/cipso and /smack/cipso2 */ if (string[0] == '-') - return ERR_PTR(-EINVAL); + return -EINVAL; for (i = 0; i < len; i++) if (string[i] > '~' || string[i] <= ' ' || string[i] == '/' || @@ -471,6 +471,25 @@ char *smk_parse_smack(const char *string, int len) break; if (i == 0 || i >= SMK_LONGLABEL) + return -EINVAL; + + return i; +} + +/** + * smk_parse_smack - copy the starting segment in the string + * that constitutes a valid smack label + * @string: a text string that might contain a Smack label at the beginning + * @len: the maximum size to look into, may be zero if string is null-terminated + * + * Returns a pointer to the copy of the label or an error code. + */ +char *smk_parse_smack(const char *string, int len) +{ + char *smack; + int i = smk_parse_label_len(string, len); + + if (i < 0) return ERR_PTR(-EINVAL); smack = kstrndup(string, i, GFP_NOFS); @@ -554,31 +573,25 @@ int smack_populate_secattr(struct smack_known *skp) } /** - * smk_import_entry - import a label, return the list entry - * @string: a text string that might be a Smack label - * @len: the maximum size, or zero if it is NULL terminated. + * smk_import_valid_allocated_label - import a label, return the list entry + * @smack: a text string that is a valid Smack label and may be kfree()ed. + * It is consumed: either becomes a part of the entry or kfree'ed. * - * Returns a pointer to the entry in the label list that - * matches the passed string, adding it if necessary, - * or an error code. + * Returns: see description of smk_import_entry() */ -struct smack_known *smk_import_entry(const char *string, int len) +static struct smack_known * +smk_import_allocated_label(char *smack, gfp_t gfp) { struct smack_known *skp; - char *smack; int rc; - smack = smk_parse_smack(string, len); - if (IS_ERR(smack)) - return ERR_CAST(smack); - mutex_lock(&smack_known_lock); skp = smk_find_entry(smack); if (skp != NULL) goto freeout; - skp = kzalloc(sizeof(*skp), GFP_NOFS); + skp = kzalloc(sizeof(*skp), gfp); if (skp == NULL) { skp = ERR_PTR(-ENOMEM); goto freeout; @@ -608,6 +621,42 @@ struct smack_known *smk_import_entry(const char *string, int len) return skp; } +/** + * smk_import_entry - import a label, return the list entry + * @string: a text string that might contain a Smack label at the beginning + * @len: the maximum size to look into, may be zero if string is null-terminated + * + * Returns a pointer to the entry in the label list that + * matches the passed string, adding it if necessary, + * or an error code. + */ +struct smack_known *smk_import_entry(const char *string, int len) +{ + char *smack = smk_parse_smack(string, len); + + if (IS_ERR(smack)) + return ERR_CAST(smack); + + return smk_import_allocated_label(smack, GFP_NOFS); +} + +/** + * smk_import_valid_label - import a label, return the list entry + * @label a text string that is a valid Smack label, not null-terminated + * + * Returns: see description of smk_import_entry() + */ +struct smack_known * +smk_import_valid_label(const char *label, int label_len, gfp_t gfp) +{ + char *smack = kstrndup(label, label_len, gfp); + + if (!smack) + return ERR_PTR(-ENOMEM); + + return smk_import_allocated_label(smack, gfp); +} + /** * smack_from_secid - find the Smack label associated with a secid * @secid: an integer that might be associated with a Smack label diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 95a614ae4c9c..4c8434503de5 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3664,7 +3664,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) * @attr: which attribute to fetch * @ctx: buffer to receive the result * @size: available size in, actual size out - * @flags: unused + * @flags: reserved, currently zero * * Fill the passed user space @ctx with the details of the requested * attribute. @@ -3725,57 +3725,52 @@ static int smack_getprocattr(struct task_struct *p, const char *name, char **val * Sets the Smack value of the task. Only setting self * is permitted and only with privilege * - * Returns the length of the smack label or an error code + * Returns zero on success or an error code */ -static int do_setattr(u64 attr, void *value, size_t size) +static int do_setattr(unsigned int attr, void *value, size_t size) { struct task_smack *tsp = smack_cred(current_cred()); struct cred *new; struct smack_known *skp; - char *labelstr; - int rc = 0; - - if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) - return -EPERM; + int label_len; + /* + * let unprivileged user validate input, check permissions later + */ if (value == NULL || size == 0 || size >= SMK_LONGLABEL) return -EINVAL; - if (attr != LSM_ATTR_CURRENT) - return -EOPNOTSUPP; - - labelstr = smk_parse_smack(value, size); - if (IS_ERR(labelstr)) - return PTR_ERR(labelstr); + label_len = smk_parse_label_len(value, size); + if (label_len < 0 || label_len != size) + return -EINVAL; /* * No process is ever allowed the web ("@") label * and the star ("*") label. */ - if (labelstr[1] == '\0' /* '@', '*' */) { - const char c = labelstr[0]; + if (label_len == 1 /* '@', '*' */) { + const char c = *(const char *)value; if (c == *smack_known_web .smk_known || - c == *smack_known_star.smk_known) { - rc = -EPERM; - goto free_labelstr; - } + c == *smack_known_star.smk_known) + return -EPERM; } if (!smack_privileged(CAP_MAC_ADMIN)) { const struct smack_known_list_elem *sklep; - list_for_each_entry(sklep, &tsp->smk_relabel, list) - if (strcmp(sklep->smk_label->smk_known, labelstr) == 0) - goto free_labelstr; - rc = -EPERM; + list_for_each_entry(sklep, &tsp->smk_relabel, list) { + const char *cp = sklep->smk_label->smk_known; + + if (strlen(cp) == label_len && + strncmp(cp, value, label_len) == 0) + goto in_relabel; + } + return -EPERM; +in_relabel: + ; } -free_labelstr: - kfree(labelstr); - if (rc) - return -EPERM; - - skp = smk_import_entry(value, size); + skp = smk_import_valid_label(value, label_len, GFP_KERNEL); if (IS_ERR(skp)) return PTR_ERR(skp); @@ -3791,7 +3786,7 @@ static int do_setattr(u64 attr, void *value, size_t size) smk_destroy_label_list(&tsp->smk_relabel); commit_creds(new); - return size; + return 0; } /** @@ -3799,7 +3794,7 @@ static int do_setattr(u64 attr, void *value, size_t size) * @attr: which attribute to set * @ctx: buffer containing the data * @size: size of @ctx - * @flags: unused + * @flags: reserved, must be zero * * Fill the passed user space @ctx with the details of the requested * attribute. @@ -3809,12 +3804,26 @@ static int do_setattr(u64 attr, void *value, size_t size) static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx, u32 size, u32 flags) { - int rc; + if (attr != LSM_ATTR_CURRENT) + return -EOPNOTSUPP; - rc = do_setattr(attr, ctx->ctx, ctx->ctx_len); - if (rc > 0) - return 0; - return rc; + if (ctx->flags) + return -EINVAL; + /* + * string must have \0 terminator, included in ctx->ctx + * (see description of struct lsm_ctx) + */ + if (ctx->ctx_len == 0) + return -EINVAL; + + if (ctx->ctx[ctx->ctx_len - 1] != '\0') + return -EINVAL; + /* + * other do_setattr() caller, smack_setprocattr(), + * does not count \0 into size, so + * decreasing length by 1 to accommodate the divergence. + */ + return do_setattr(attr, ctx->ctx, ctx->ctx_len - 1); } /** @@ -3830,11 +3839,23 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx, */ static int smack_setprocattr(const char *name, void *value, size_t size) { - int attr = lsm_name_to_attr(name); + size_t realsize = size; + unsigned int attr = lsm_name_to_attr(name); - if (attr != LSM_ATTR_UNDEF) - return do_setattr(attr, value, size); - return -EINVAL; + switch (attr) { + case LSM_ATTR_UNDEF: return -EINVAL; + default: return -EOPNOTSUPP; + case LSM_ATTR_CURRENT: + ; + } + + /* + * excuse one and only one trailing \n in value if exists. + */ + if (size && (((const char *)value)[size - 1] == '\n')) + realsize = size - 1; + + return do_setattr(attr, value, realsize) ? : size; } /**