Bug 243747 - lib/libc/net/rcmd.c: add another length check to the iruserok_sa
Summary: lib/libc/net/rcmd.c: add another length check to the iruserok_sa
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-31 02:17 UTC by Andrew Reiter
Modified: 2020-04-03 13:36 UTC (History)
2 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Reiter 2020-01-31 02:17:53 UTC 
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) ` ?
Comment 1 Andrew Reiter 2020-01-31 02:24:54 UTC 
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.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2020-02-05 15:24:55 UTC 
Seems reasonable to me.
Comment 3 commit-hook freebsd_committer freebsd_triage 2020-02-05 16:10:39 UTC 
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
Comment 4 Andrew Reiter 2020-02-05 18:33:41 UTC 
Thank you for the quick response and the commit! Best, Andrew
Comment 5 Andrew Reiter 2020-02-07 02:59:06 UTC 
Woops, maybe I was ignoring c99 here. I should not submit pr's when sleepy.
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2020-02-07 14:44:13 UTC 
(In reply to Andrew Reiter from comment #5)
Could you elaborate?
Comment 7 Andrew Reiter 2020-02-08 18:13:52 UTC 
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.
Comment 8 Mark Johnston freebsd_committer freebsd_triage 2020-02-10 23:21:00 UTC 
(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.
Comment 9 commit-hook freebsd_committer freebsd_triage 2020-02-12 15:52:00 UTC 
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