| Message ID | 20200214234203.7086-7-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Fri, Feb 14, 2020 at 6:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Change security_secctx_to_secid() to fill in a lsmblob instead > of a u32 secid. Multiple LSMs may be able to interpret the > string, and this allows for setting whichever secid is > appropriate. In some cases there is scaffolding where other > interfaces have yet to be converted. > > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: John Johansen <john.johansen@canonical.com> > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 5 +++-- > kernel/cred.c | 4 +--- > net/netfilter/nft_meta.c | 12 +++++++----- > net/netfilter/xt_SECMARK.c | 5 ++++- > net/netlabel/netlabel_unlabeled.c | 14 ++++++++------ > security/security.c | 18 +++++++++++++++--- > 6 files changed, 38 insertions(+), 20 deletions(-) ... > diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c > index 951b6e87ed5d..e12125b85035 100644 > --- a/net/netfilter/nft_meta.c > +++ b/net/netfilter/nft_meta.c > @@ -811,21 +811,23 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = { > > static int nft_secmark_compute_secid(struct nft_secmark *priv) > { > - u32 tmp_secid = 0; > + struct lsmblob blob; > int err; > > - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid); > + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob); > if (err) > return err; > > - if (!tmp_secid) > + if (!lsmblob_is_set(&blob)) > return -ENOENT; > > - err = security_secmark_relabel_packet(tmp_secid); > + /* Using le[0] is scaffolding */ > + err = security_secmark_relabel_packet(blob.secid[0]); > if (err) > return err; At the very least it looks like the comment above needs an update. However, I would really like to see an explanation in this patch, either in the comments or in the commit description, about how you plan to handle secmarks. If your plan is to always have it be the first LSM, let's admit that and document it appropriately. If there is something much grander coming later in the patchset I guess "scaffolding" is an okay term, but it would be good to mention in the commit description that this will be replaced with something better later in the patchset. I'm worried about the case five years from know when we are changing this code, either due to bugs or new features, and we stumble across this commit. Was it always intended to be this way? Or was this temporary? Right now I don't know. > - priv->secid = tmp_secid; > + /* Using le[0] is scaffolding */ > + priv->secid = blob.secid[0]; > return 0; > } ... > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > index d2e4ab8d1cb1..7a5a87f15736 100644 > --- a/net/netlabel/netlabel_unlabeled.c > +++ b/net/netlabel/netlabel_unlabeled.c > @@ -881,7 +881,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb, > void *addr; > void *mask; > u32 addr_len; > - u32 secid; > + struct lsmblob blob; > struct netlbl_audit audit_info; > > /* Don't allow users to add both IPv4 and IPv6 addresses for a > @@ -905,12 +905,13 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb, > ret_val = security_secctx_to_secid( > nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]), > nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]), > - &secid); > + &blob); > if (ret_val != 0) > return ret_val; > > + /* scaffolding with the [0] */ > return netlbl_unlhsh_add(&init_net, > - dev_name, addr, mask, addr_len, secid, > + dev_name, addr, mask, addr_len, blob.secid[0], > &audit_info); > } Same as above, although this time with the peer label.
On 3/6/2020 4:58 PM, Paul Moore wrote: > On Fri, Feb 14, 2020 at 6:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Change security_secctx_to_secid() to fill in a lsmblob instead >> of a u32 secid. Multiple LSMs may be able to interpret the >> string, and this allows for setting whichever secid is >> appropriate. In some cases there is scaffolding where other >> interfaces have yet to be converted. >> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> Reviewed-by: John Johansen <john.johansen@canonical.com> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 5 +++-- >> kernel/cred.c | 4 +--- >> net/netfilter/nft_meta.c | 12 +++++++----- >> net/netfilter/xt_SECMARK.c | 5 ++++- >> net/netlabel/netlabel_unlabeled.c | 14 ++++++++------ >> security/security.c | 18 +++++++++++++++--- >> 6 files changed, 38 insertions(+), 20 deletions(-) > ... > >> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c >> index 951b6e87ed5d..e12125b85035 100644 >> --- a/net/netfilter/nft_meta.c >> +++ b/net/netfilter/nft_meta.c >> @@ -811,21 +811,23 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = { >> >> static int nft_secmark_compute_secid(struct nft_secmark *priv) >> { >> - u32 tmp_secid = 0; >> + struct lsmblob blob; >> int err; >> >> - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid); >> + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob); >> if (err) >> return err; >> >> - if (!tmp_secid) >> + if (!lsmblob_is_set(&blob)) >> return -ENOENT; >> >> - err = security_secmark_relabel_packet(tmp_secid); >> + /* Using le[0] is scaffolding */ >> + err = security_secmark_relabel_packet(blob.secid[0]); >> if (err) >> return err; > At the very least it looks like the comment above needs an update. I can see that. > However, I would really like to see an explanation in this patch, > either in the comments or in the commit description, about how you > plan to handle secmarks. Yes. It should probably be spread between here, 0017 and the introduction. > If your plan is to always have it be the > first LSM, let's admit that and document it appropriately. If there > is something much grander coming later in the patchset I guess > "scaffolding" is an okay term, but it would be good to mention in the > commit description that this will be replaced with something better > later in the patchset. You are correct. > I'm worried about the case five years from know when we are changing > this code, either due to bugs or new features, and we stumble across > this commit. Was it always intended to be this way? Or was this > temporary? Right now I don't know. > >> - priv->secid = tmp_secid; >> + /* Using le[0] is scaffolding */ >> + priv->secid = blob.secid[0]; >> return 0; >> } > ... > >> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c >> index d2e4ab8d1cb1..7a5a87f15736 100644 >> --- a/net/netlabel/netlabel_unlabeled.c >> +++ b/net/netlabel/netlabel_unlabeled.c >> @@ -881,7 +881,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb, >> void *addr; >> void *mask; >> u32 addr_len; >> - u32 secid; >> + struct lsmblob blob; >> struct netlbl_audit audit_info; >> >> /* Don't allow users to add both IPv4 and IPv6 addresses for a >> @@ -905,12 +905,13 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb, >> ret_val = security_secctx_to_secid( >> nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]), >> nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]), >> - &secid); >> + &blob); >> if (ret_val != 0) >> return ret_val; >> >> + /* scaffolding with the [0] */ >> return netlbl_unlhsh_add(&init_net, >> - dev_name, addr, mask, addr_len, secid, >> + dev_name, addr, mask, addr_len, blob.secid[0], >> &audit_info); >> } > Same as above, although this time with the peer label. >
diff --git a/include/linux/security.h b/include/linux/security.h index 2ad58e2900e7..65adcffafee9 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -498,7 +498,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value, int security_netlink_send(struct sock *sk, struct sk_buff *skb); int security_ismaclabel(const char *name); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid); +int security_secctx_to_secid(const char *secdata, u32 seclen, + struct lsmblob *blob); void security_release_secctx(char *secdata, u32 seclen); void security_inode_invalidate_secctx(struct inode *inode); int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); @@ -1304,7 +1305,7 @@ static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *secle static inline int security_secctx_to_secid(const char *secdata, u32 seclen, - u32 *secid) + struct lsmblob *blob) { return -EOPNOTSUPP; } diff --git a/kernel/cred.c b/kernel/cred.c index fae63c0a1b36..e654a5a84408 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -756,14 +756,12 @@ EXPORT_SYMBOL(set_security_override); int set_security_override_from_ctx(struct cred *new, const char *secctx) { struct lsmblob blob; - u32 secid; int ret; - ret = security_secctx_to_secid(secctx, strlen(secctx), &secid); + ret = security_secctx_to_secid(secctx, strlen(secctx), &blob); if (ret < 0) return ret; - lsmblob_init(&blob, secid); return set_security_override(new, &blob); } EXPORT_SYMBOL(set_security_override_from_ctx); diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c index 951b6e87ed5d..e12125b85035 100644 --- a/net/netfilter/nft_meta.c +++ b/net/netfilter/nft_meta.c @@ -811,21 +811,23 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = { static int nft_secmark_compute_secid(struct nft_secmark *priv) { - u32 tmp_secid = 0; + struct lsmblob blob; int err; - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid); + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob); if (err) return err; - if (!tmp_secid) + if (!lsmblob_is_set(&blob)) return -ENOENT; - err = security_secmark_relabel_packet(tmp_secid); + /* Using le[0] is scaffolding */ + err = security_secmark_relabel_packet(blob.secid[0]); if (err) return err; - priv->secid = tmp_secid; + /* Using le[0] is scaffolding */ + priv->secid = blob.secid[0]; return 0; } diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c index 2317721f3ecb..2d68416b4552 100644 --- a/net/netfilter/xt_SECMARK.c +++ b/net/netfilter/xt_SECMARK.c @@ -45,13 +45,14 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par) static int checkentry_lsm(struct xt_secmark_target_info *info) { + struct lsmblob blob; int err; info->secctx[SECMARK_SECCTX_MAX - 1] = '\0'; info->secid = 0; err = security_secctx_to_secid(info->secctx, strlen(info->secctx), - &info->secid); + &blob); if (err) { if (err == -EINVAL) pr_info_ratelimited("invalid security context \'%s\'\n", @@ -59,6 +60,8 @@ static int checkentry_lsm(struct xt_secmark_target_info *info) return err; } + /* scaffolding during the transition */ + info->secid = blob.secid[0]; if (!info->secid) { pr_info_ratelimited("unable to map security context \'%s\'\n", info->secctx); diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c index d2e4ab8d1cb1..7a5a87f15736 100644 --- a/net/netlabel/netlabel_unlabeled.c +++ b/net/netlabel/netlabel_unlabeled.c @@ -881,7 +881,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb, void *addr; void *mask; u32 addr_len; - u32 secid; + struct lsmblob blob; struct netlbl_audit audit_info; /* Don't allow users to add both IPv4 and IPv6 addresses for a @@ -905,12 +905,13 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb, ret_val = security_secctx_to_secid( nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]), nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]), - &secid); + &blob); if (ret_val != 0) return ret_val; + /* scaffolding with the [0] */ return netlbl_unlhsh_add(&init_net, - dev_name, addr, mask, addr_len, secid, + dev_name, addr, mask, addr_len, blob.secid[0], &audit_info); } @@ -932,7 +933,7 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb, void *addr; void *mask; u32 addr_len; - u32 secid; + struct lsmblob blob; struct netlbl_audit audit_info; /* Don't allow users to add both IPv4 and IPv6 addresses for a @@ -954,12 +955,13 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb, ret_val = security_secctx_to_secid( nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]), nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]), - &secid); + &blob); if (ret_val != 0) return ret_val; + /* scaffolding with the [0] */ return netlbl_unlhsh_add(&init_net, - NULL, addr, mask, addr_len, secid, + NULL, addr, mask, addr_len, blob.secid[0], &audit_info); } diff --git a/security/security.c b/security/security.c index 2fa826000889..e0f9287483aa 100644 --- a/security/security.c +++ b/security/security.c @@ -2002,10 +2002,22 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen) } EXPORT_SYMBOL(security_secid_to_secctx); -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) +int security_secctx_to_secid(const char *secdata, u32 seclen, + struct lsmblob *blob) { - *secid = 0; - return call_int_hook(secctx_to_secid, 0, secdata, seclen, secid); + struct security_hook_list *hp; + int rc; + + lsmblob_init(blob, 0); + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) { + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot)) + continue; + rc = hp->hook.secctx_to_secid(secdata, seclen, + &blob->secid[hp->lsmid->slot]); + if (rc != 0) + return rc; + } + return 0; } EXPORT_SYMBOL(security_secctx_to_secid);