* [PATCH] libsepol: fix endianity in ibpkey range checks
@ 2018-10-17 14:46 Ondrej Mosnacek
2018-10-17 15:04 ` William Roberts
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2018-10-17 14:46 UTC (permalink / raw)
To: selinux; +Cc: selinux, Stephen Smalley, Daniel Jurgens, Ondrej Mosnacek
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 14:46 [PATCH] libsepol: fix endianity in ibpkey range checks Ondrej Mosnacek
@ 2018-10-17 15:04 ` William Roberts
2018-10-17 15:31 ` Stephen Smalley
2018-10-17 16:06 ` William Roberts
2 siblings, 0 replies; 9+ messages in thread
From: William Roberts @ 2018-10-17 15:04 UTC (permalink / raw)
To: omosnace; +Cc: selinux, Stephen Smalley, selinux
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 14:46 [PATCH] libsepol: fix endianity in ibpkey range checks Ondrej Mosnacek
2018-10-17 15:04 ` William Roberts
@ 2018-10-17 15:31 ` Stephen Smalley
2018-10-17 15:34 ` William Roberts
2018-10-17 16:06 ` William Roberts
2 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2018-10-17 15:31 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux; +Cc: selinux, Daniel Jurgens
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;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 15:31 ` Stephen Smalley
@ 2018-10-17 15:34 ` William Roberts
0 siblings, 0 replies; 9+ messages in thread
From: William Roberts @ 2018-10-17 15:34 UTC (permalink / raw)
To: Stephen Smalley; +Cc: omosnace, selinux, selinux
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 14:46 [PATCH] libsepol: fix endianity in ibpkey range checks Ondrej Mosnacek
2018-10-17 15:04 ` William Roberts
2018-10-17 15:31 ` Stephen Smalley
@ 2018-10-17 16:06 ` William Roberts
2018-10-17 21:18 ` Paul Moore
2018-10-18 7:20 ` Ondrej Mosnacek
2 siblings, 2 replies; 9+ messages in thread
From: William Roberts @ 2018-10-17 16:06 UTC (permalink / raw)
To: omosnace; +Cc: selinux, Stephen Smalley, selinux
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 16:06 ` William Roberts
@ 2018-10-17 21:18 ` Paul Moore
2018-10-17 21:22 ` Stephen Smalley
2018-10-18 7:20 ` Ondrej Mosnacek
1 sibling, 1 reply; 9+ messages in thread
From: Paul Moore @ 2018-10-17 21:18 UTC (permalink / raw)
To: bill.c.roberts; +Cc: omosnace, selinux, Stephen Smalley, selinux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 21:18 ` Paul Moore
@ 2018-10-17 21:22 ` Stephen Smalley
2018-10-17 22:05 ` William Roberts
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2018-10-17 21:22 UTC (permalink / raw)
To: Paul Moore, bill.c.roberts; +Cc: omosnace, selinux, selinux
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)
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 21:22 ` Stephen Smalley
@ 2018-10-17 22:05 ` William Roberts
0 siblings, 0 replies; 9+ messages in thread
From: William Roberts @ 2018-10-17 22:05 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Paul Moore, omosnace, selinux, selinux
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)
> >
> >
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libsepol: fix endianity in ibpkey range checks
2018-10-17 16:06 ` William Roberts
2018-10-17 21:18 ` Paul Moore
@ 2018-10-18 7:20 ` Ondrej Mosnacek
1 sibling, 0 replies; 9+ messages in thread
From: Ondrej Mosnacek @ 2018-10-18 7:20 UTC (permalink / raw)
To: bill.c.roberts; +Cc: selinux, Stephen Smalley, SElinux list
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)
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-18 7:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-17 14:46 [PATCH] libsepol: fix endianity in ibpkey range checks Ondrej Mosnacek
2018-10-17 15:04 ` William Roberts
2018-10-17 15:31 ` Stephen Smalley
2018-10-17 15:34 ` William Roberts
2018-10-17 16:06 ` William Roberts
2018-10-17 21:18 ` Paul Moore
2018-10-17 21:22 ` Stephen Smalley
2018-10-17 22:05 ` William Roberts
2018-10-18 7:20 ` Ondrej Mosnacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).