On Monday 04 January 2010 04:11:37 pm Stephen Smalley wrote:
> On Fri, 2009-12-18 at 13:30 -0500, Paul Moore wrote:
> > It is possible security_compute_av() to return -EINVAL, even when in
> > permissive mode, due to unknown object classes and SIDs. This patch
> > fixes this by doing away with the return value for security_compute_av()
> > and treating unknown classes and SIDs as permission denials.
> >
> > NOTE: This patch has only been compile tested, and is only intended for
> > review. However, if someone wants to give it a try I would appreciate it
> > as my time is somewhat limited since I'm technically on "vacation" as of
> > a few hours ago :)
> >
> > Reported-by: Andrew Worsley <amworsley@xxxxxxxxx>
> > Signed-off-by: Paul Moore <paul.moore@xxxxxx>
>
> NAK.
>
> Sorry, but I think we need to reconsider this patch, as separating the
> initialization of the avd from the remainder of the computation inside
> of context_struct_compute_av is fragile, as is separating the printk
> error message from the actual error checking (and allow_unknown isn't
> relevant in that particular case - we have already covered it after we
> unmap the class).
Okay, no problem, you know these bits of code far better than I do.
However, since I seem to keep missing the mark here some very explicit
directions on the fix you would like to see would be helpful (I thought I had
followed your previous suggestions but evidently I was off) or feel free to
cobble something up yourself ... ;)
> > ---
> > security/selinux/avc.c | 14 ++--
> > security/selinux/include/security.h | 8 +-
> > security/selinux/selinuxfs.c | 4 +
> > security/selinux/ss/services.c | 113
> > +++++++++++++---------------------- 4 files changed, 54 insertions(+), 85
> > deletions(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index f2dde26..7471a0a 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -731,7 +731,6 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > struct avc_node *node;
> > struct av_decision avd_entry, *avd;
> > int rc = 0;
> > - u32 denied;
> >
> > BUG_ON(!requested);
> >
> > @@ -746,9 +745,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > else
> > avd = &avd_entry;
> >
> > - rc = security_compute_av(ssid, tsid, tclass, requested, avd);
> > - if (rc)
> > - goto out;
> > + security_compute_av(ssid, tsid, tclass, avd);
> > rcu_read_lock();
> > node = avc_insert(ssid, tsid, tclass, avd);
> > } else {
> > @@ -757,12 +754,11 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > avd = &node->ae.avd;
> > }
> >
> > - denied = requested & ~(avd->allowed);
> > -
> > - if (denied) {
> > + if (requested & ~(avd->allowed)) {
> > if (flags & AVC_STRICT)
> > rc = -EACCES;
> > - else if (!selinux_enforcing || (avd->flags &
AVD_FLAGS_PERMISSIVE))
> > + else if (!selinux_enforcing ||
> > + (avd->flags & AVD_FLAGS_PERMISSIVE))
> > avc_update_node(AVC_CALLBACK_GRANT, requested, ssid,
> > tsid, tclass, avd->seqno);
> > else
> > @@ -770,7 +766,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > }
> >
> > rcu_read_unlock();
> > -out:
> > +
> > return rc;
> > }
> >
> > diff --git a/security/selinux/include/security.h
> > b/security/selinux/include/security.h index 2553266..609334d 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -96,12 +96,10 @@ struct av_decision {
> > /* definitions of av_decision.flags */
> > #define AVD_FLAGS_PERMISSIVE 0x0001
> >
> > -int security_compute_av(u32 ssid, u32 tsid,
> > - u16 tclass, u32 requested,
> > - struct av_decision *avd);
> > +void security_compute_av(u32 ssid, u32 tsid, u16 tclass,
> > + struct av_decision *avd);
> >
> > -int security_compute_av_user(u32 ssid, u32 tsid,
> > - u16 tclass, u32 requested,
> > +int security_compute_av_user(u32 ssid, u32 tsid, u16 tclass,
> > struct av_decision *avd);
> >
> > int security_transition_sid(u32 ssid, u32 tsid,
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index fab36fd..a45ac7c 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -511,6 +511,8 @@ static ssize_t sel_write_access(struct file *file,
> > char *buf, size_t size) if (!tcon)
> > goto out;
> >
> > + /* note: we don't currently use "req" anymore but we'll leave it here
> > + * just to preserve/enforce the existing userspace API */
> > length = -EINVAL;
> > if (sscanf(buf, "%s %s %hu %x", scon, tcon, &tclass, &req) != 4)
> > goto out2;
> > @@ -522,7 +524,7 @@ static ssize_t sel_write_access(struct file *file,
> > char *buf, size_t size) if (length < 0)
> > goto out2;
> >
> > - length = security_compute_av_user(ssid, tsid, tclass, req, &avd);
> > + length = security_compute_av_user(ssid, tsid, tclass, &avd);
> > if (length < 0)
> > goto out2;
> >
> > diff --git a/security/selinux/ss/services.c
> > b/security/selinux/ss/services.c index b3efae2..e789b34 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -90,7 +90,6 @@ static int context_struct_to_string(struct context
> > *context, char **scontext, static int context_struct_compute_av(struct
> > context *scontext,
> > struct context *tcontext,
> > u16 tclass,
> > - u32 requested,
> > struct av_decision *avd);
> >
> > struct selinux_mapping {
> > @@ -196,23 +195,6 @@ static u16 unmap_class(u16 tclass)
> > return tclass;
> > }
> >
> > -static u32 unmap_perm(u16 tclass, u32 tperm)
> > -{
> > - if (tclass < current_mapping_size) {
> > - unsigned i;
> > - u32 kperm = 0;
> > -
> > - for (i = 0; i < current_mapping[tclass].num_perms; i++)
> > - if (tperm & (1<<i)) {
> > - kperm |= current_mapping[tclass].perms[i];
> > - tperm &= ~(1<<i);
> > - }
> > - return kperm;
> > - }
> > -
> > - return tperm;
> > -}
> > -
> > static void map_decision(u16 tclass, struct av_decision *avd,
> > int allow_unknown)
> > {
> > @@ -532,7 +514,6 @@ out:
> > static void type_attribute_bounds_av(struct context *scontext,
> > struct context *tcontext,
> > u16 tclass,
> > - u32 requested,
> > struct av_decision *avd)
> > {
> > struct context lo_scontext;
> > @@ -553,7 +534,6 @@ static void type_attribute_bounds_av(struct context
> > *scontext, context_struct_compute_av(&lo_scontext,
> > tcontext,
> > tclass,
> > - requested,
> > &lo_avd);
> > if ((lo_avd.allowed & avd->allowed) == avd->allowed)
> > return; /* no masked permission */
> > @@ -569,7 +549,6 @@ static void type_attribute_bounds_av(struct context
> > *scontext, context_struct_compute_av(scontext,
> > &lo_tcontext,
> > tclass,
> > - requested,
> > &lo_avd);
> > if ((lo_avd.allowed & avd->allowed) == avd->allowed)
> > return; /* no masked permission */
> > @@ -586,7 +565,6 @@ static void type_attribute_bounds_av(struct context
> > *scontext, context_struct_compute_av(&lo_scontext,
> > &lo_tcontext,
> > tclass,
> > - requested,
> > &lo_avd);
> > if ((lo_avd.allowed & avd->allowed) == avd->allowed)
> > return; /* no masked permission */
> > @@ -610,7 +588,6 @@ static void type_attribute_bounds_av(struct context
> > *scontext, static int context_struct_compute_av(struct context *scontext,
> > struct context *tcontext,
> > u16 tclass,
> > - u32 requested,
> > struct av_decision *avd)
> > {
> > struct constraint_node *constraint;
> > @@ -622,20 +599,8 @@ static int context_struct_compute_av(struct context
> > *scontext, struct ebitmap_node *snode, *tnode;
> > unsigned int i, j;
> >
> > - /*
> > - * Initialize the access vectors to the default values.
> > - */
> > - avd->allowed = 0;
> > - avd->auditallow = 0;
> > - avd->auditdeny = 0xffffffff;
> > - avd->seqno = latest_granting;
> > - avd->flags = 0;
> > -
> > - if (unlikely(!tclass || tclass > policydb.p_classes.nprim)) {
> > - if (printk_ratelimit())
> > - printk(KERN_WARNING "SELinux: Invalid class %hu\n",
tclass);
> > + if (unlikely(!tclass || tclass > policydb.p_classes.nprim))
> > return -EINVAL;
> > - }
> >
> > tclass_datum = policydb.class_val_to_struct[tclass - 1];
> >
> > @@ -704,8 +669,7 @@ static int context_struct_compute_av(struct context
> > *scontext, * constraint, lazy checks have to mask any violated
> > * permission and notice it to userspace via audit.
> > */
> > - type_attribute_bounds_av(scontext, tcontext,
> > - tclass, requested, avd);
> > + type_attribute_bounds_av(scontext, tcontext, tclass, avd);
> >
> > return 0;
> > }
> > @@ -890,11 +854,11 @@ out:
> > static int security_compute_av_core(u32 ssid,
> > u32 tsid,
> > u16 tclass,
> > - u32 requested,
> > + int allow_unknown,
> > struct av_decision *avd)
> > {
> > struct context *scontext = NULL, *tcontext = NULL;
> > - int rc = 0;
> > + int rc;
> >
> > scontext = sidtab_search(&sidtab, ssid);
> > if (!scontext) {
> > @@ -909,8 +873,9 @@ static int security_compute_av_core(u32 ssid,
> > return -EINVAL;
> > }
> >
> > - rc = context_struct_compute_av(scontext, tcontext, tclass,
> > - requested, avd);
> > + rc = context_struct_compute_av(scontext, tcontext, tclass, avd);
> > + if (rc == -EINVAL && !allow_unknown && printk_ratelimit())
> > + printk(KERN_WARNING "SELinux: Invalid class %hu\n", tclass);
> >
> > /* permissive domain? */
> > if (ebitmap_get_bit(&policydb.permissive_map, scontext->type))
> > @@ -924,70 +889,78 @@ static int security_compute_av_core(u32 ssid,
> > * @ssid: source security identifier
> > * @tsid: target security identifier
> > * @tclass: target security class
> > - * @requested: requested permissions
> > * @avd: access vector decisions
> > *
> > * Compute a set of access vector decisions based on the
> > * SID pair (@ssid, @tsid) for the permissions in @tclass.
> > - * Return -%EINVAL if any of the parameters are invalid or %0
> > - * if the access vector decisions were computed successfully.
> > */
> > -int security_compute_av(u32 ssid,
> > - u32 tsid,
> > - u16 orig_tclass,
> > - u32 orig_requested,
> > - struct av_decision *avd)
> > +void security_compute_av(u32 ssid,
> > + u32 tsid,
> > + u16 orig_tclass,
> > + struct av_decision *avd)
> > {
> > u16 tclass;
> > - u32 requested;
> > int rc;
> >
> > read_lock(&policy_rwlock);
> >
> > + /* Initialize the access vectors to the default values. */
> > + avd->allowed = 0;
> > + avd->auditallow = 0;
> > + avd->auditdeny = 0xffffffff;
> > + avd->seqno = latest_granting;
> > + avd->flags = 0;
> > +
> > if (!ss_initialized)
> > goto allow;
> >
> > - requested = unmap_perm(orig_tclass, orig_requested);
> > tclass = unmap_class(orig_tclass);
> > - if (unlikely(orig_tclass && !tclass)) {
> > - if (policydb.allow_unknown)
> > - goto allow;
> > - rc = -EINVAL;
> > - goto out;
> > - }
> > - rc = security_compute_av_core(ssid, tsid, tclass, requested, avd);
> > + if (unlikely(orig_tclass && !tclass))
> > + goto error;
> > + rc = security_compute_av_core(ssid, tsid, tclass,
> > + policydb.allow_unknown, avd);
> > + if (unlikely(rc != 0))
> > + goto error;
> > map_decision(orig_tclass, avd, policydb.allow_unknown);
> > out:
> > read_unlock(&policy_rwlock);
> > - return rc;
> > + return;
> > allow:
> > avd->allowed = 0xffffffff;
> > + goto out;
> > +error:
> > + if (policydb.allow_unknown)
> > + goto allow;
> > + avd->allowed = 0;
> > avd->auditallow = 0;
> > avd->auditdeny = 0xffffffff;
> > - avd->seqno = latest_granting;
> > - avd->flags = 0;
> > - rc = 0;
> > goto out;
> > }
> >
> > int security_compute_av_user(u32 ssid,
> > u32 tsid,
> > u16 tclass,
> > - u32 requested,
> > struct av_decision *avd)
> > {
> > int rc;
> >
> > + read_lock(&policy_rwlock);
> > +
> > + /* Initialize the access vectors to the default values. */
> > + avd->allowed = 0;
> > + avd->auditallow = 0;
> > + avd->auditdeny = 0xffffffff;
> > + avd->seqno = latest_granting;
> > + avd->flags = 0;
> > +
> > if (!ss_initialized) {
> > avd->allowed = 0xffffffff;
> > - avd->auditallow = 0;
> > - avd->auditdeny = 0xffffffff;
> > - avd->seqno = latest_granting;
> > - return 0;
> > + rc = 0;
> > + goto out;
> > }
> >
> > - read_lock(&policy_rwlock);
> > - rc = security_compute_av_core(ssid, tsid, tclass, requested, avd);
> > + rc = security_compute_av_core(ssid, tsid, tclass, 0, avd);
> > +out:
> > read_unlock(&policy_rwlock);
> > return rc;
> > }
> >
> >
> > --
> > This message was distributed to subscribers of the selinux mailing list.
> > If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx
> > with the words "unsubscribe selinux" without quotes as the message.
>
--
paul moore
linux @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
