Page MenuHomeFreeBSD
Differential D56740

dhclient: Improve server and filename validation
ClosedPublic
Actions

Authored by des on Thu, Apr 30, 8:24 AM.
Tags
None
Referenced Files
F155105589: D56740.id176887.diff
Fri, May 1, 10:55 AM
F155105586: D56740.id176901.diff
Fri, May 1, 10:55 AM
F155105567: D56740.diff
Fri, May 1, 10:55 AM
Unknown Object (File)
Thu, Apr 30, 6:34 PM
Unknown Object (File)
Thu, Apr 30, 2:28 PM
Unknown Object (File)
Thu, Apr 30, 2:23 PM
Unknown Object (File)
Thu, Apr 30, 2:17 PM
Unknown Object (File)
Thu, Apr 30, 2:11 PM
View All 12 Files
Subscribers
andrew
brooks
imp

Details

Reviewers
markj
brooks
Group Reviewers
security
Commits
rGdc8762cfb6e2: dhclient: Improve server and filename validation
rGdfcb69cdb07e: dhclient: Improve server and filename validation
rGb362b6b6c8f2: dhclient: Improve server and filename validation
rG5bad905eb37f: dhclient: Improve server and filename validation
rGb1ece85741db: dhclient: Improve server and filename validation
rG252f603d1704: dhclient: Improve server and filename validation
rG2f9478ad42c4: dhclient: Improve server and filename validation
rG873a195ba635: dhclient: Improve server and filename validation
Summary
  • Don't iterate over each string three times; once is enough.
  • Reject control characters (anything below space) in addition to the double quote and backslash.
  • If an unsafe character is encountered, discard the string instead of rejecting the entire lease.
  • If backslashes are encountered in the file name option, convert them to forward slashes instead of rejecting the option.
  • Tweak the warning messages a bit. Looking through the rest of the code, it seems to me that notes generally end with a period while warnings generally don't.

Fixes: 8008e4b88daf ("dhclient: Check for unexpected characters in some
DHCP server options")
PR: 294886
MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

des created this revision.Thu, Apr 30, 8:24 AM
Herald added a subscriber: imp. · View Herald TranscriptThu, Apr 30, 8:24 AM
Harbormaster completed remote builds in B72664: Diff 176874.Thu, Apr 30, 8:24 AM
des requested review of this revision.Thu, Apr 30, 8:24 AM
des updated this revision to Diff 176886.Thu, Apr 30, 10:31 AM

no need for else after break

Harbormaster completed remote builds in B72665: Diff 176886.Thu, Apr 30, 10:31 AM
brooks accepted this revision.Thu, Apr 30, 10:34 AM
brooks added a subscriber: brooks.

The net effect look like the right one. I'm not convinced by the open-coding the checks and copy given the lease processing rate should single digits per hour in most environments and it will all be in L1 on even a terrible in-order core, but I don't care that much.

sbin/dhclient/dhclient.c
1267
This revision is now accepted and ready to land.Thu, Apr 30, 10:34 AM
des updated this revision to Diff 176887.Thu, Apr 30, 10:38 AM

fix copy-paste mistake

This revision now requires review to proceed.Thu, Apr 30, 10:38 AM
des marked an inline comment as done.Thu, Apr 30, 10:38 AM
Harbormaster completed remote builds in B72666: Diff 176887.Thu, Apr 30, 10:38 AM
andrew added a subscriber: andrew.Thu, Apr 30, 11:14 AM
andrew added inline comments.
sbin/dhclient/dhclient.c
1222–1223

Is this correct?

markj added inline comments.Thu, Apr 30, 12:14 PM
sbin/dhclient/dhclient.c
1222–1223

This is backwards, indeed.

des updated this revision to Diff 176899.Thu, Apr 30, 1:38 PM

fix inverted condition

Harbormaster completed remote builds in B72670: Diff 176899.Thu, Apr 30, 1:38 PM
des marked 2 inline comments as done.Thu, Apr 30, 1:38 PM
des added inline comments.
sbin/dhclient/dhclient.c
1222–1223

Thanks, I just hacked this up quickly when I saw the PR, and unfortunately I don't have a way of testing it.

des marked an inline comment as done.Thu, Apr 30, 1:38 PM
markj accepted this revision.Thu, Apr 30, 1:43 PM
This revision is now accepted and ready to land.Thu, Apr 30, 1:43 PM
markj added inline comments.Thu, Apr 30, 1:52 PM
sbin/dhclient/dhclient.c
1269

Probably we should avoid printing this message if the offending char is a backslash, as that's a normal situation.

des updated this revision to Diff 176901.Thu, Apr 30, 2:05 PM
des marked an inline comment as done.

convert backslashes in file names to forward slashes

This revision now requires review to proceed.Thu, Apr 30, 2:05 PM
Harbormaster completed remote builds in B72671: Diff 176901.Thu, Apr 30, 2:05 PM
des edited the summary of this revision. (Show Details)Thu, Apr 30, 2:05 PM
markj accepted this revision.Thu, Apr 30, 2:10 PM
This revision is now accepted and ready to land.Thu, Apr 30, 2:10 PM
Closed by commit rG873a195ba635: dhclient: Improve server and filename validation (authored by des). · Explain WhyThu, Apr 30, 4:46 PM
This revision was automatically updated to reflect the committed changes.
des added a commit: rG873a195ba635: dhclient: Improve server and filename validation.
des added a commit: rG2f9478ad42c4: dhclient: Improve server and filename validation.Thu, Apr 30, 9:07 PM
des added a commit: rG252f603d1704: dhclient: Improve server and filename validation.
des added a commit: rGb1ece85741db: dhclient: Improve server and filename validation.
markj added a commit: rG5bad905eb37f: dhclient: Improve server and filename validation.Fri, May 1, 3:08 PM
markj added a commit: rGb362b6b6c8f2: dhclient: Improve server and filename validation.
markj added a commit: rGdfcb69cdb07e: dhclient: Improve server and filename validation.
markj added a commit: rGdc8762cfb6e2: dhclient: Improve server and filename validation.

Revision Contents
Changeset List

PathSize
sbin/
dhclient/
75 lines

Diff 176925

View Options

sbin/dhclient/dhclient.c

Loading...