Page MenuHomeFreeBSD
Differential D6201

Add nid_namelen bounds check to nfssvc system call
ClosedPublic
Actions

Authored by emaste on May 4 2016, 2:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 5:52 PM
Unknown Object (File)
Mon, Nov 18, 8:30 PM
Unknown Object (File)
Oct 4 2024, 9:42 PM
Unknown Object (File)
Oct 3 2024, 3:54 PM
Unknown Object (File)
Oct 1 2024, 3:13 AM
Unknown Object (File)
Sep 24 2024, 11:15 PM
Unknown Object (File)
Sep 22 2024, 12:04 AM
Unknown Object (File)
Sep 21 2024, 11:52 PM
View All 49 Files
Subscribers
cturt_hardenedbsd.org
lattera-gmail.com

Details

Reviewers
rmacklem
Commits
rS299199: Add nid_namelen bounds check to nfssvc system call
Summary

This is only allowed by root and only used by the nfs daemon, which should not provide an incorrect value. However, it's still good practice to avoid integer overflow on data provided by userland.

PR:         206626
Reported by:       CTurt <cturt@hardenedbsd.org>
MFC After:  1 month

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste updated this revision to Diff 15889.May 4 2016, 2:30 PM
emaste retitled this revision from to Add nid_namelen bounds check to nfssvc system call.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added a reviewer: rmacklem.
emaste added a subscriber: cturt_hardenedbsd.org.
lattera-gmail.com added a subscriber: lattera-gmail.com.May 4 2016, 4:01 PM
emaste added inline comments.May 4 2016, 5:52 PM
sys/fs/nfs/nfs_commonsubs.c
3177 ↗(On Diff #15889)

I think MAXHOSTNAMELEN is a reasonable upper bound, rather than bare 128 in the submitted patch.

rmacklem edited edge metadata.May 4 2016, 10:25 PM

nid_namelen is set to the length of one of three strings:

  • the fully qualified domain name
  • a group name returned by getgrent()
  • a user name returned by getpwent()

I'd say 0 isn't a valid value and I have no idea what the upper bound
should be, but I'd say at least 1024.
There is a definition of MAXNAME in nfsuserd.c which is set to 1024.

rmacklem added a comment.May 4 2016, 10:32 PM

For this specific case, MAXHOSTNAMELEN (which I think is 1024)
would be correct, since I think that is the limit for fully qualified
domain name?

emaste added a comment.May 5 2016, 2:23 AM
In D6201#132163, @rmacklem wrote:

For this specific case, MAXHOSTNAMELEN (which I think is 1024)
would be correct, since I think that is the limit for fully qualified
domain name?

In sys/param.h:

#define MAXHOSTNAMELEN      256

but I think this is still a valid bound for the length of a domain name, and user and group names have a smaller limit.

rmacklem accepted this revision.May 5 2016, 12:57 PM
rmacklem edited edge metadata.

Given that the patch only checks for the domain name case, I think this is fine.
(I might suggest "if (nid->nid_namelen <= 0 ...", but since I didn't feel the
check was needed in the first place, I don't think making it "<=" is a big deal.

This revision is now accepted and ready to land.May 5 2016, 12:57 PM
Closed by commit rS299199: Add nid_namelen bounds check to nfssvc system call (authored by emaste). · Explain WhyMay 6 2016, 9:19 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
sys/
fs/
nfs/
4 lines

Diff 16001

View Options

head/sys/fs/nfs/nfs_commonsubs.c

Loading...