| Message ID | 20181017144659.10960-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | libsepol: fix endianity in ibpkey range checks | expand |
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]); > + > + 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 This seems to line up with what I have been following on the kernel side. But since Stephen committed the patch this fixes up and is way more involved, ill defer to him. Soft ack from me, but I would imagine we would want to see an ack on the kernel side before pulling this in. > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
On 10/17/2018 10:46 AM, Ondrej Mosnacek 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> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > 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]); > + > + 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; >
On Wed, Oct 17, 2018 at 8:30 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 10/17/2018 10:46 AM, Ondrej Mosnacek 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> > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> Stephen, Ill stage this as a PR. Do you want to wait until the kernel changes are in or just the normal 24 hours? Bill > > > --- > > 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]); > > + > > + 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; > > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
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 (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)
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)
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. > >>> + >>> + 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) > > >
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) > > > > > > >
On Wed, Oct 17, 2018 at 6: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. Oh right, I didn't realize those fields are 16-bit... Let me fix it and re-spin. > > > + > > + 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)
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]); + + 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;
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(-)