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

From: William Roberts <bill.c.roberts@gmail.com>
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Paul Moore <paul@paul-moore.com>,
	omosnace@redhat.com, selinux@vger.kernel.org,
	selinux <selinux@tycho.nsa.gov>
Subject: Re: [PATCH] libsepol: fix endianity in ibpkey range checks
Date: Wed, 17 Oct 2018 15:05:00 -0700	[thread overview]
Message-ID: <CAFftDdqsDxYh=QrBXhtsgnRhwSR42KGKjU+Ry_8Ygq+0kc-W4A@mail.gmail.com> (raw)
In-Reply-To: <ae98be54-7bcd-b501-a1ff-b9ab0ae3aa95@tycho.nsa.gov>

On Wed, Oct 17, 2018 at 2:21 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 10/17/2018 05:18 PM, Paul Moore wrote:
> > 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.
>
> The kernel patch has the same bug, so it will also need a re-spin.  Good
> catch.

Don't thank me, thank GCC and Travis. This compiled on my local machine
and ran the test suite just fine. I had clang set up, I guess this re-iterates
the need and benefit of Travis testing in different environments.

>
> >
> >>> +
> >>> +                               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)
> >
> >
> >
>