Page MenuHomeFreeBSD
Differential D25622

ping(8): Improve validation of -g, -G and -h parameters.
ClosedPublic
Actions

Authored by markj on Jul 11 2020, 5:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 27, 1:52 AM
Unknown Object (File)
Wed, Jun 25, 7:41 PM
Unknown Object (File)
Tue, Jun 24, 10:23 PM
Unknown Object (File)
Tue, Jun 24, 5:48 PM
Unknown Object (File)
Tue, Jun 24, 4:57 PM
Unknown Object (File)
Mon, Jun 23, 12:39 PM
Unknown Object (File)
Sat, Jun 14, 5:44 AM
Unknown Object (File)
Mon, Jun 9, 7:28 PM
View All Files
Subscribers
imp
maxim
yuripv

Details

Reviewers
eugen_grosbein.net
Commits
rS367988: ping(8): Improve parameter validation
Summary
  • Check for potential overflow from strtol().
  • Make sure sweepmax isn't larger than the maximum packet size.
  • Check against sweepmax before incrementing the packet size, not after.

Inspired by PR 239978.

Diff Detail

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

Event Timeline

markj requested review of this revision.Jul 11 2020, 5:11 PM
markj created this revision.
Harbormaster completed remote builds in B32244: Diff 74313.Jul 11 2020, 5:11 PM
markj added a reviewer: eugen_grosbein.net.Jul 11 2020, 5:11 PM
yuripv added a subscriber: yuripv.Jul 11 2020, 6:23 PM
yuripv added inline comments.
sbin/ping/ping.c
342 ↗(On Diff #74313)

All of these would benefit of using strtonum(3) instead.

markj planned changes to this revision.Jul 11 2020, 6:41 PM
markj added inline comments.
sbin/ping/ping.c
342 ↗(On Diff #74313)

Oh nice, I didn't know about that function.

maxim added a subscriber: maxim.EditedNov 4 2020, 1:49 PM

For the record, the patch also fixes 239974 and 239975.

markj updated this revision to Diff 79514.Nov 13 2020, 6:51 PM

Convert to strtonum().

Harbormaster completed remote builds in B34782: Diff 79514.Nov 13 2020, 6:51 PM
maxim added inline comments.Nov 13 2020, 8:07 PM
sbin/ping/ping.c
250 ↗(On Diff #79514)

Do you really need to change the type here? All strtonum() use cases are limited by LONG_MAX or smaller values.

982 ↗(On Diff #79514)

You don't need a whitespace before ";" here. Overall, this change looks unrelated.

markj added inline comments.Nov 13 2020, 8:18 PM
sbin/ping/ping.c
250 ↗(On Diff #79514)

I don't see any reason why not though - leaving it defined as long will trigger compiler warnings, static analyzer warnings, etc..

982 ↗(On Diff #79514)

I didn't modify that line, but I can fix it.

This is a bug related to the validation of -g/-G/-h. It could be split out in a separate diff, I don't have strong feelings either way.

markj updated this revision to Diff 79522.Nov 13 2020, 8:21 PM

Fix a nearby style bug.

Harbormaster completed remote builds in B34787: Diff 79522.Nov 13 2020, 8:21 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 24 2020, 5:12 PM
Closed by commit rS367988: ping(8): Improve parameter validation (authored by markj). · Explain Why
This revision was automatically updated to reflect the committed changes.
markj added a commit: rS367988: ping(8): Improve parameter validation.
Herald added a subscriber: imp. · View Herald TranscriptNov 24 2020, 5:12 PM

Revision Contents
Changeset List

PathSize
head/
sbin/
ping/
110 lines

Diff 79936

View Options

head/sbin/ping/ping.c

Loading...