Fix/xinetd probe memory safety#2371
Open
edznux-dd wants to merge 2 commits into
Open
Conversation
Bugs found by fuzzing xiconf_parse()/xiconf_parse_section() with crafted
xinetd configuration content. A regression test exercising each case is
added in a follow-up commit (tests/probes/xinetd).
- A line with no trailing newline set the line length to the whole file
length instead of the bytes remaining from the current offset, so the
fixed line buffer was overflowed by memcpy() (heap-buffer-overflow). Two
sites: the top-level scanner and xiconf_parse_section().
- An unterminated service section ran its for(;;) reader past the end of
the in-memory file; bound the loop to inoff < inlen and guard the
section entry against inoff already being at/after the end.
- *strchr(buffer, ' ') = '\0' dereferenced NULL when the copied line
contained an embedded NUL (arbitrary file content), at two sites
(keyword and service name). Check the result before writing.
- For a keyword with no value, op was set past the keyword's NUL
terminator, reading out of bounds; only step past it when a value
follows.
- A (name, protocol) translation key was built with strcpy()/strcat()
into a fixed buffer with no NULL or length check; a NULL protocol
dereferenced and an over-long name+protocol overflowed it. Guard both,
matching the checks already in xiconf_getservice().
- xiconf_service_free() recursed over the ->next chain; a long crafted
chain overflowed the stack. Free iteratively.
- On an unrecognized type value, scur->type (strdup'd, later free'd) was
reassigned to a string literal -> invalid free, and the old value
leaked. Free the old value and strdup("").
- op_assign_str() leaked the previous value when an attribute was
repeated within a section; free before reassigning.
We are aware of the in-progress hardening in OpenSCAP#2349, which touches this
file; its scope (include-path handling) is narrower and does not cover
the bugs above. Two findings overlap and are reconciled here.
Adds the crafted inputs that triggered the memory-safety bugs fixed in the previous commit as test fixtures, and a test case that feeds each one to test_probe_xinetd and fails if the parser is killed by a signal or trips a sanitizer (the pre-existing, harmless UBSan function-pointer-type report on the rbtree free callback is ignored). test_probe_xinetd now calls xiconf_free() on the parsed config before returning, both to release it (the test leaked it previously) and so the regression also exercises the teardown path, where two of the bugs lived.
edznux-dd
requested review from
Mab879,
jan-cerny and
matusmarhefka
as code owners
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * NULL/length guards on lookup.) */ | ||
| if (scur->name != NULL && scur->protocol != NULL && | ||
| strlen(scur->name) + strlen(scur->protocol) <= XICFG_STRANS_MAXKEYLEN) { | ||
| strcpy(st_key, scur->name); |
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
2 participants
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.
Hi! this is another (and I believe last! 😅 ) followup of #2361
This time I tried to focus on the probe part. This was my focus after investigating a few more crashes (segfault) that we discovered internally.
Some of these parsers are quite complex, but i've tried to keep the change simple and minimal.
I've decided to invert the condition for
as otherwise, the
Skipping (name, protocol) translation for servicebranch would have needed agoto(because of the strcpy)I've added regression test + reproducer inputs for the fuzz harnesses as well.
A few fuzz harnesses have been added as a new commit of the PR #2365
I believe this covers a big part of the "probes" of openscap and should help with the reliability during the parsing of arbitrary data.
The fuzzers were compiled with multiple sanitizer, so it discovered a few uninitialized variable and other UB.
Note:
I am aware of #2349 but this supersedes that PR by fixing other bugs:
l_size = inlenon no-newline line -> heap-overflow memcpy (scanner, section)for(;;)unbounded read past inmem*strchr(buf,' ')NULL-deref on embedded-NUL content (two sites)xiconf_parse_sectionentry guard (inoff >= inlen reads past buffer)xiconf_service_free-> stack overflow (we made it iterative)scur->type = ""literal later free()'d -> invalid free + leakop_assign_strleak on repeated attributeHappy to collaborate if you believe the PR 2349 should be merged first.
Thank you!