| Message ID | 20250306224317.416365-2-andreev@swemel.ru (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | smack: fix two bugs in setting task label | expand |
On 3/6/2025 2:43 PM, Konstantin Andreev wrote: > If an unprivileged task is allowed to relabel itself > (/smack/relabel-self is not empty), > it can freely create new labels by writing their > names into own /proc/PID/attr/smack/current > > This occurs because do_setattr() imports > the provided label in advance, > before checking "relabel-self" list. > > This change ensures that the "relabel-self" list > is checked before importing the label. > > Fixes: 38416e53936e ("Smack: limited capability for changing process label") > Signed-off-by: Konstantin Andreev <andreev@swemel.ru> > --- > security/smack/smack_lsm.c | 41 +++++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 99833168604e..95a614ae4c9c 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -3732,8 +3732,8 @@ static int do_setattr(u64 attr, void *value, size_t size) > struct task_smack *tsp = smack_cred(current_cred()); > struct cred *new; > struct smack_known *skp; > - struct smack_known_list_elem *sklep; > - int rc; > + char *labelstr; > + int rc = 0; > > if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) > return -EPERM; > @@ -3744,28 +3744,41 @@ static int do_setattr(u64 attr, void *value, size_t size) > if (attr != LSM_ATTR_CURRENT) > return -EOPNOTSUPP; > > - skp = smk_import_entry(value, size); > - if (IS_ERR(skp)) > - return PTR_ERR(skp); > + labelstr = smk_parse_smack(value, size); > + if (IS_ERR(labelstr)) > + return PTR_ERR(labelstr); > > /* > * No process is ever allowed the web ("@") label > * and the star ("*") label. > */ > - if (skp == &smack_known_web || skp == &smack_known_star) > - return -EINVAL; > + if (labelstr[1] == '\0' /* '@', '*' */) { > + const char c = labelstr[0]; > + > + if (c == *smack_known_web .smk_known || No space before ".smk_known". I can fix this if/when I take the patch. > + c == *smack_known_star.smk_known) { > + rc = -EPERM; > + goto free_labelstr; > + } > + } > > if (!smack_privileged(CAP_MAC_ADMIN)) { > - rc = -EPERM; > + const struct smack_known_list_elem *sklep; > list_for_each_entry(sklep, &tsp->smk_relabel, list) > - if (sklep->smk_label == skp) { > - rc = 0; > - break; > - } > - if (rc) > - return rc; > + if (strcmp(sklep->smk_label->smk_known, labelstr) == 0) > + goto free_labelstr; > + rc = -EPERM; > } > > +free_labelstr: > + kfree(labelstr); > + if (rc) > + return -EPERM; > + > + skp = smk_import_entry(value, size); > + if (IS_ERR(skp)) > + return PTR_ERR(skp); > + > new = prepare_creds(); > if (new == NULL) > return -ENOMEM;
Casey Schaufler, 07 Mar 2025: > On 3/6/2025 2:43 PM, Konstantin Andreev wrote: >> - if (skp == &smack_known_web || skp == &smack_known_star) >> - return -EINVAL; >> + if (labelstr[1] == '\0' /* '@', '*' */) { >> + const char c = labelstr[0]; >> + >> + if (c == *smack_known_web .smk_known || > > No space before ".smk_known". I can fix this if/when I take the patch. > >> + c == *smack_known_star.smk_known) { >> + rc = -EPERM; >> + goto free_labelstr; >> + } >> + } This is to align two ".smk_known"s in two adjacent lines. I strive to make monotypic operations visually monotypic. This catches reader's eyes and lets him recognize the pattern faster. Of course, If this spacing violates the rules, it should be corrected. -- Konstantin Andreev
On 3/7/2025 11:45 AM, Konstantin Andreev wrote: > Casey Schaufler, 07 Mar 2025: >> On 3/6/2025 2:43 PM, Konstantin Andreev wrote: >>> - if (skp == &smack_known_web || skp == &smack_known_star) >>> - return -EINVAL; >>> + if (labelstr[1] == '\0' /* '@', '*' */) { >>> + const char c = labelstr[0]; >>> + >>> + if (c == *smack_known_web .smk_known || >> >> No space before ".smk_known". I can fix this if/when I take the patch. >> >>> + c == *smack_known_star.smk_known) { >>> + rc = -EPERM; >>> + goto free_labelstr; >>> + } >>> + } > > This is to align two ".smk_known"s in two adjacent lines. > > I strive to make monotypic operations visually monotypic. > This catches reader's eyes and lets him recognize the pattern faster. > > Of course, If this spacing violates the rules, it should be corrected. Yes, the spacing violates the rules. I don't agree with all the rules, but unnecessary whitespace isn't something I generally approve of. > > -- > Konstantin Andreev >
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 99833168604e..95a614ae4c9c 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3732,8 +3732,8 @@ static int do_setattr(u64 attr, void *value, size_t size) struct task_smack *tsp = smack_cred(current_cred()); struct cred *new; struct smack_known *skp; - struct smack_known_list_elem *sklep; - int rc; + char *labelstr; + int rc = 0; if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) return -EPERM; @@ -3744,28 +3744,41 @@ static int do_setattr(u64 attr, void *value, size_t size) if (attr != LSM_ATTR_CURRENT) return -EOPNOTSUPP; - skp = smk_import_entry(value, size); - if (IS_ERR(skp)) - return PTR_ERR(skp); + labelstr = smk_parse_smack(value, size); + if (IS_ERR(labelstr)) + return PTR_ERR(labelstr); /* * No process is ever allowed the web ("@") label * and the star ("*") label. */ - if (skp == &smack_known_web || skp == &smack_known_star) - return -EINVAL; + if (labelstr[1] == '\0' /* '@', '*' */) { + const char c = labelstr[0]; + + if (c == *smack_known_web .smk_known || + c == *smack_known_star.smk_known) { + rc = -EPERM; + goto free_labelstr; + } + } if (!smack_privileged(CAP_MAC_ADMIN)) { - rc = -EPERM; + const struct smack_known_list_elem *sklep; list_for_each_entry(sklep, &tsp->smk_relabel, list) - if (sklep->smk_label == skp) { - rc = 0; - break; - } - if (rc) - return rc; + if (strcmp(sklep->smk_label->smk_known, labelstr) == 0) + goto free_labelstr; + rc = -EPERM; } +free_labelstr: + kfree(labelstr); + if (rc) + return -EPERM; + + skp = smk_import_entry(value, size); + if (IS_ERR(skp)) + return PTR_ERR(skp); + new = prepare_creds(); if (new == NULL) return -ENOMEM;
If an unprivileged task is allowed to relabel itself (/smack/relabel-self is not empty), it can freely create new labels by writing their names into own /proc/PID/attr/smack/current This occurs because do_setattr() imports the provided label in advance, before checking "relabel-self" list. This change ensures that the "relabel-self" list is checked before importing the label. Fixes: 38416e53936e ("Smack: limited capability for changing process label") Signed-off-by: Konstantin Andreev <andreev@swemel.ru> --- security/smack/smack_lsm.c | 41 +++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-)