Bug 243747
| Summary: | lib/libc/net/rcmd.c: add another length check to the iruserok_sa | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Andrew Reiter <areiter> |
| Component: | misc | Assignee: | Mark Johnston <markj> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | arr, markj |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Also, I did not mention that I was uncertain if this behavior was to conform to some specification which should allow this ticket to be closed. Seems reasonable to me. A commit references this bug: Author: markj Date: Wed Feb 5 16:09:44 UTC 2020 New revision: 357575 URL: https://svnweb.freebsd.org/changeset/base/357575 Log: Improve validation of the sockaddr length in iruserok_sa(). Negative numbers are not valid sockaddr lengths. PR: 243747 Submitted by: Andrew Reiter <areiter@veracode.com> MFC after: 1 week Changes: head/lib/libc/net/rcmd.c Thank you for the quick response and the commit! Best, Andrew Woops, maybe I was ignoring c99 here. I should not submit pr's when sleepy. (In reply to Andrew Reiter from comment #5) Could you elaborate? I think bc sizeof() returns size_t only the == 0 case will matter.. but that being said, having it clear/explicit (ie <= 0) is kind of nice... and at this point I am being wasteful of people's time on a pedantic bit :-). /me goes back to hole. Thanks again. (In reply to Andrew Reiter from comment #7) Ah, yes, I think you're right. I agree that it's probably better this way anyway. A commit references this bug: Author: markj Date: Wed Feb 12 15:51:29 UTC 2020 New revision: 357825 URL: https://svnweb.freebsd.org/changeset/base/357825 Log: MFC r357575: Improve validation of the sockaddr length in iruserok_sa(). PR: 243747 Changes: _U stable/12/ stable/12/lib/libc/net/rcmd.c |
This is not really a bug, I have not seen it misused in code utilizing the API call, and I wonder the actual number of users of the call, but I file this if it might be worth adding for robustness sake. Apologies to open a ticket for such a small and likely inconsequential-if-not-changed bit of nitpickery, but so it goes: This is simply adding an additional length check to iruserok_sa() function in lib/libc/net/rcmd.c. There is a check for the length on line 441: 427 iruserok_sa(const void *ra, int rlen, int superuser, const char *ruser, 428 const char *luser) 429 { 430 char *cp; 431 struct stat sbuf; 432 struct passwd *pwd; 433 FILE *hostf; 434 uid_t uid; 435 int first; 436 char pbuf[MAXPATHLEN]; 437 const struct sockaddr *raddr; 438 struct sockaddr_storage ss; 439 440 /* avoid alignment issue */ 441 if (rlen > sizeof(ss)) 442 return(-1); 443 memcpy(&ss, ra, rlen); Is worth changing to `(rlen > sizeof(ss) || rlen <= 0) ` ?