conf: fix one-byte OOB read/write underflow in chomp() - #95
Merged
Conversation
chomp() trims trailing newlines from a line read from a .service config file. For a string consisting of only a newline (str == n, exactly what fgets() produces for a blank line), the pointer p starts at str+0. The loop body zeroes str[0], decrements p to str-1, and then unconditionally dereferences *p to test the loop condition -- a one-byte read before the start of the buffer. If that stray byte also happens to be 0x0A the loop keeps walking backward, turning the OOB read into an OOB write as well. Confirmed with the project's own asan CI flags (-fsanitize=address,undefined -fno-sanitize-recover=undefined): a .service file with a blank line between two directives makes UBSan report 'load of address ... with insufficient space for an object of type char' at this exact line, reachable via the normal parse() -> read_line() -> chomp() path. Bound the pointer before dereferencing it, mirroring the tab indentation used by the rest of the function (the line was previously indented with spaces instead of a tab). Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
Owner
|
Ouch, stale copy of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
chomp()insrc/conf.ctrims trailing newlines from a line readfrom a
.serviceconfig file:For a string consisting of only a newline (
str == "\n", exactlywhat
fgets()produces for a blank line in a.servicefile),pstarts at
str + 0. The loop body zeroesstr[0], decrementspto
str - 1, and then unconditionally dereferences*pto test theloop condition — a one-byte read before the start of the buffer. If
that stray byte also happens to be
0x0Athe loop keeps walkingbackward, turning the OOB read into an OOB write as well.
This is reachable via the normal
parse() -> read_line() -> chomp()path whenever a
.servicefile contains a blank line betweendirectives, and
conf.cisn't covered by the project's unit testsor the
asanCI job'sTESTS=list.Repro
Built with the exact flags the
asanCI job uses(
-fsanitize=address,undefined -fno-sanitize-recover=undefined -g -O1),and a
.servicefile with a blank line:Running
mdnsd -n -l debugagainst it reports:Fix
Bound the pointer before dereferencing it, mirroring the tab
indentation used by the rest of the function (the affected line was
indented with spaces instead of a tab):
Rebuilding with the same UBSan/ASan flags and rerunning the same
repro produces zero sanitizer errors; the blank line is now handled
as a safe no-op and the following
txt version=1line still parsesand shows up correctly in the resulting TXT record.