Page MenuHomeFreeBSD

Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes
ClosedPublic

Authored by truckman on May 21 2016, 11:08 PM.

Details

Summary

Limit the length of the name retrieved from the environment
variable $LOGNAME or $USER. Sanitize it by removing whitespace
to benefit log file parsers. Double any embedded '%' characters
since this string will be embedded in a *printf format string.

Detect and warn about overflow when building the format string.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

truckman updated this revision to Diff 16681.May 21 2016, 11:08 PM
truckman retitled this revision from to Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes.
truckman updated this object.
truckman edited the test plan for this revision. (Show Details)
truckman added a reviewer: jmallett.
markj accepted this revision.May 24 2016, 1:46 AM
markj edited edge metadata.

I think this looks right from a correctness perspective. It might be nice to have a comment explaining what sanitization we're doing. The amount of indentation in this function could be reduced if we return early in the logfile == NULL case.

usr.sbin/pw/pw_log.c
58 ↗(On Diff #16681)

The indentation looks wonky here, but that may just be phabricator.

67 ↗(On Diff #16681)

The cast seems superfluous since we're comparing with a constant whose value is clearly representable with an int. Does clang/coverity warn about this?

(ditto below)

69 ↗(On Diff #16681)

Why not have cp++ in the loop header?

This revision is now accepted and ready to land.May 24 2016, 1:46 AM

I like the early return idea, though it will make the diff a lot bigger.

usr.sbin/pw/pw_log.c
58 ↗(On Diff #16681)

I tried to match the existing style here with outdented "*" for pointers.

67 ↗(On Diff #16681)

Yes, clang complains, which I agree is bogus in this case:

cc -O2 -pipe -g -MD -MF.depend.pw_log.o -MTpw_log.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Qunused-arguments -c /usr/src/usr.sbin/pw/pw_log.c -o pw_log.o
/usr/src/usr.sbin/pw/pw_log.c:67:11: error: comparison of integers of different

signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
                              i < sizeof(sname) - 1; ) {
                              ~ ^ ~~~~~~~~~~~~~~~~~

1 error generated.

69 ↗(On Diff #16681)

Good idea.

truckman updated this revision to Diff 16767.May 24 2016, 2:54 AM
truckman edited edge metadata.

Return early when possible to reduce code nesting.

Add a comment to explain the sanitization of name.

Increment cp at the top of the for loop.

Adjust variable declarations to be more style compliant, including
moving initialization out of the declarations.

This revision now requires review to proceed.May 24 2016, 2:54 AM
truckman updated this revision to Diff 16773.May 24 2016, 4:07 AM
truckman edited edge metadata.

Re-order the tests in the name sanitization loop to eliminate the else clause

truckman updated this revision to Diff 16774.May 24 2016, 4:09 AM

Whitespace changes

markj accepted this revision.May 24 2016, 4:39 AM
markj edited edge metadata.

LGTM, thanks! My only other nit is that it might be nice to include the errant username in the overflow warning message.

This revision is now accepted and ready to land.May 24 2016, 4:39 AM
This revision was automatically updated to reflect the committed changes.