smack: fix bug: setting task label silently ignores input garbage

This 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 various trailers after label

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 <andreev@swemel.ru>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

1 files changed, 71 insertions, 22 deletions

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;
@@ -609,6 +622,42 @@ unlockout:
 }
 
 /**
+ * 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
  *