sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode
ClosedPublic
Actions

Authored by olce on Tue, Oct 7, 5:14 PM.

Details

Reviewers

rmacklem
dfr

Commits

rGf647564f3ff6: sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode
rG9492a1e27fb1: sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode
rG47e9c81d4f13: sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode

Summary

When the received authentication message had more than XU_NGROUPS, we
would write group IDs beyond the end of cr_groups[] in the 'struct
xucred' being filled (as 'ngroups_max' is always greater than
XU_NGROUPS).

For robustness, prevent various OOB accesses that would result from
changes of values of XU_NGROUPS and AUTH_SYS_MAX_GROUPS or a 'struct
xucred' with an invalid 'cr_ngroups' field, even if these cases are
unlikely.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 67618
Build 64501: arc lint + arc unit

Event Timeline

olce created this revision.Tue, Oct 7, 5:14 PM

Herald added a subscriber: imp. · View Herald TranscriptTue, Oct 7, 5:14 PM

olce requested review of this revision.Tue, Oct 7, 5:14 PM

Harbormaster completed remote builds in B67618: Diff 163711.Tue, Oct 7, 5:14 PM

olce added a child revision: D52961: sys/rpc: Define AUTH_SYS_MAX_{GROUPS,HOSTNAME}.Tue, Oct 7, 5:24 PM

olce added a reviewer: dfr.

olce mentioned this in D52263: krpc: UNIX auth: Prevent DoS, fix various OOB accesses.Tue, Oct 7, 5:26 PM

Looks ok to me. I'll leave min vs MIN up to you.

sys/rpc/authunix_prot.c
131	Technically, min() compares int and not uint32_t. It should be ok, but this is why I prefer to use the MIN() macro that doesn't care about types. I'll leave it up to you as to whether or not you change it.

This revision is now accepted and ready to land.Sun, Oct 12, 11:50 PM

In D52960#1212126, @rmacklem wrote:

Looks ok to me. I'll leave min vs MIN up to you.

Changing to MIN(). Yes, MIN() has the advantage that it works regardless of the type, but also the disadvantage that the expressions to compare are expanded twice, which can create useless work or cause side-effects to happen multiple times. I'm pretty sure it doesn't matter in this case, as there's no side-effect and the expressions simple enough for the compiler to be able to factor them out. We could actually have the best of both worlds by changing the definition of MIN(), making use of some GNU extensions, but I don't know if that would be acceptable in sys/param.h. Something for later.

sys/rpc/authunix_prot.c
131	`min()` takes `u_int`, not `int`. It's `imin()` that takes `int`. The day where an architecture with an `unsigned int` different than `uint32_t` has yet to come. :-) But I agree with you that we should preferably use a function with the exact type here, even if there's no practical consequence. Please see also the main comment.

olce mentioned this in D52964: sys/rpc: UNIX auth: Fix OOB reads on too short message.Mon, Oct 13, 1:16 PM

olce marked an inline comment as done.Mon, Oct 13, 3:11 PM

Closed by commit rG47e9c81d4f13: sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode (authored by olce). · Explain WhyTue, Oct 14, 12:23 PM

This revision was automatically updated to reflect the committed changes.

olce added a commit: rG47e9c81d4f13: sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode.

In D52960#1212329, @olce wrote:

In D52960#1212126, @rmacklem wrote:

Looks ok to me. I'll leave min vs MIN up to you.

Changing to MIN(). Yes, MIN() has the advantage that it works regardless of the type, but also the disadvantage that the expressions to compare are expanded twice, which can create useless work or cause side-effects to happen multiple times. I'm pretty sure it doesn't matter in this case, as there's no side-effect and the expressions simple enough for the compiler to be able to factor them out. We could actually have the best of both worlds by changing the definition of MIN(), making use of some GNU extensions, but I don't know if that would be acceptable in sys/param.h. Something for later.