| Message ID | 5582E8CF.3030509@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/18/2015 11:50 AM, Carlos O'Donell wrote: > The ruserok API does hosts checks first while it walks the > user's ~/.rhosts file. This results in lots of DNS queries > that could have been skipped if we short-circuit test the > user portion first to see if would have had a failed match. > > This supports configurations where rlogin is used on internal > secure networks with large numbers of users and machines. > > The Red Hat QE team did extensive testing on various rlogin > combinations to validate this change, and in fact we found > a defect in the first version which is fixed in this version. > Unfortunately without installed tree + container testing we > can't add an easy test case for this. We need to setup one > or two systems in order to verify, and that's what we did. > We'll get to this eventually though. > > I have also updated the linux kernel man page to describe > the configuration syntax in more detail: > http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/?id=427cee53f06a4be5bfd808191ecc5624d3f0240b > (with some follow up commits) > > Tested on x86-64, i686, ppc64, ppc64le, aarch64, s390, and > s390x with no regressions. > > OK? > > 2015-06-18 Carlos O'Donell <carlos@redhat.com> [BZ #18557] > * inet/rcmd.c (__validuser2_sa): Check user first to short-circuit > additional host check. Added BZ since this is user visible behaviour. Cheers, Carlos.
On 18 Jun 2015 11:50, Carlos O'Donell wrote: > --- a/inet/rcmd.c > +++ b/inet/rcmd.c > > + /* First check the user part. This is an optimization, since > + one should always check the host first in order to detect > + negative host checks (which we check for later). */ > + ucheck = __icheckuser (user, ruser); the phrasing in this comment sounds confused, or maybe misleading ? > + /* Next check host part */ style is incorrect (period/trailing space) otherwise patch looks fine to me -mike
On 07/08/2015 02:30 AM, Mike Frysinger wrote: > On 18 Jun 2015 11:50, Carlos O'Donell wrote: >> --- a/inet/rcmd.c >> +++ b/inet/rcmd.c >> >> + /* First check the user part. This is an optimization, since >> + one should always check the host first in order to detect >> + negative host checks (which we check for later). */ >> + ucheck = __icheckuser (user, ruser); > > the phrasing in this comment sounds confused, or maybe misleading ? Rewritten as: /* First check the user part. In a naive implementation we would check the host part first, then the user. However, if we check the user first and reject the entry we will have saved doing any host lookups to normalize the comparison and that likely saves several DNS queries. Therefore we check the user first. */ >> + /* Next check host part */ > > style is incorrect (period/trailing space) Fixed. Given your review and no objections, and the testing we've done on x86_64, i686, aarch64, s390, s390x, ppc64, and ppc64le I'm considering this OK. Checked in. Cheers, Carlos.
diff --git a/inet/rcmd.c b/inet/rcmd.c index 98b3735..91623b0 100644 --- a/inet/rcmd.c +++ b/inet/rcmd.c @@ -809,29 +809,38 @@ __validuser2_sa(hostf, ra, ralen, luser, ruser, rhost) *p = '\0'; /* <nul> terminate username (+host?) */ /* buf -> host(?) ; user -> username(?) */ + if (*buf == '\0') + break; + if (*user == '\0') + user = luser; + + /* First check the user part. This is an optimization, since + one should always check the host first in order to detect + negative host checks (which we check for later). */ + ucheck = __icheckuser (user, ruser); + + /* Either we found the user, or we didn't and this is a + negative host check. We must do the negative host lookup + in order to preserve the semantics of stopping on this line + before processing others. */ + if (ucheck != 0 || *buf == '-') { + + /* Next check host part */ + hcheck = __checkhost_sa (ra, ralen, buf, rhost); + + /* Negative '-host user(?)' match? */ + if (hcheck < 0) + break; - /* First check host part */ - hcheck = __checkhost_sa (ra, ralen, buf, rhost); - - if (hcheck < 0) - break; - - if (hcheck) { - /* Then check user part */ - if (! (*user)) - user = luser; - - ucheck = __icheckuser (user, ruser); - - /* Positive 'host user' match? */ - if (ucheck > 0) { + /* Positive 'host user' match? */ + if (hcheck > 0 && ucheck > 0) { retval = 0; break; } - /* Negative 'host -user' match? */ - if (ucheck < 0) - break; + /* Negative 'host -user' match? */ + if (hcheck > 0 && ucheck < 0) + break; /* Neither, go on looking for match */ }