Re: [PATCH] libsepol: fix endianity in ibpkey range checks

From: Paul Moore <paul@paul-moore.com>
To: bill.c.roberts@gmail.com
Cc: omosnace@redhat.com, selinux@vger.kernel.org,
	Stephen Smalley <sds@tycho.nsa.gov>,
	selinux@tycho.nsa.gov
Subject: Re: [PATCH] libsepol: fix endianity in ibpkey range checks
Date: Wed, 17 Oct 2018 17:18:35 -0400	[thread overview]
Message-ID: <CAHC9VhT_npm4ix7kmN8GOOxUp8z7ySz0Z6LLKqd0VOMJPKcibA@mail.gmail.com> (raw)
In-Reply-To: <CAFftDdqKAHN3+Mb3QB2n=555PFKYntpKf_sXzWWppQ4tP=AdvQ@mail.gmail.com>

On Wed, Oct 17, 2018 at 12:07 PM William Roberts
<bill.c.roberts@gmail.com> wrote:
> On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > We need to convert from little-endian before dong range checks on the
> > ibpkey port numbers, otherwise we would be checking a wrong value.
> >
> > Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  libsepol/src/policydb.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > index a6d76ca3..dc201e2f 100644
> > --- a/libsepol/src/policydb.c
> > +++ b/libsepol/src/policydb.c
> > @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct policydb_compat_info *info,
> >                                 break;
> >                         case OCON_IBPKEY:
> >                                 rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > -                               if (rc < 0 || buf[2] > 0xffff || buf[3] > 0xffff)
> > +                               if (rc < 0)
> >                                         return -1;
> >
> > +                               c->u.ibpkey.low_pkey  = le32_to_cpu(buf[2]);
> > +                               c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
>
> Ahh you're assigning a 32 bit value to a 16 bit variable low|high_pkey. I think
> you need to assign them to a uint32_t if you want to actually range check them.

If you can, give me a chance to look over the kernel changes first.  I
doubt I'll see anything objectionable given the review the patches
have already seen, but one never knows.

> > +
> > +                               if (c->u.ibpkey.low_pkey  > 0xffff ||
> > +                                   c->u.ibpkey.high_pkey > 0xffff)
> > +                                       return -1;
> > +
> > +                               /* we want c->u.ibpkey.subnet_prefix in network
> > +                                * (big-endian) order, just memcpy it */
> >                                 memcpy(&c->u.ibpkey.subnet_prefix, buf,
> >                                        sizeof(c->u.ibpkey.subnet_prefix));
> >
> > -                               c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]);
> > -                               c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
> > -
> >                                 if (context_read_and_validate
> >                                     (&c->context[0], p, fp))
> >                                         return -1;
> > --
> > 2.17.2
> >
> See job: https://travis-ci.org/SELinuxProject/selinux/jobs/442750208
>
> Build fail with gcc:
>
> policydb.c:2839:31: error: comparison is always false due to limited
> range of data type [-Werror=type-limits]
>      if (c->u.ibpkey.low_pkey  > 0xffff ||
>                                ^
> policydb.c:2840:31: error: comparison is always false due to limited
> range of data type [-Werror=type-limits]
>          c->u.ibpkey.high_pkey > 0xffff)

-- 
paul moore
www.paul-moore.com