Fix/xinetd probe memory safety#2371
Open
edznux-dd wants to merge 1 commit into
Open
Conversation
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}
Mab879
requested changes
Jun 17, 2026
Mab879
left a comment
Mab879
left a comment
Member
There was a problem hiding this comment.
Please review the finding from Sonar and GitHub security findings.
Fuzzing the xinetd OVAL probe config parser surfaced several crashes and memory-safety issues on malformed input. This fixes them and hardens the parser: - Fix the heap overflow, NULL-derefs, out-of-bounds reads, unbounded recursion and invalid free/leak reachable from crafted config files. - Build the (name, protocol) translation key and include paths with bounded snprintf via shared helpers, so registration and lookup can never disagree or overflow the fixed buffers. - Reduce parser nesting and cognitive complexity flagged by static analysis (extract include / includedir handling and strans registration into helpers; for->while; split declarations). Adds a regression test that feeds malformed configs through the parser under sanitizers, plus teardown coverage.
edznux-dd
force-pushed
the
fix/xinetd-probe-memory-safety
branch
from
38a411a to
68e1aff
Compare
|
| continue; | ||
| } | ||
|
|
||
| xiconf_add_cfile (xiconf, pathbuf, xifile->depth + 1); |
| if (inctype == XICONF_INCTYPE_FILE) { | ||
| strncpy (pathbuf, inclarg, sizeof(pathbuf)-1); | ||
| dD("includefile: %s", pathbuf); | ||
| xiconf_add_cfile (xiconf, pathbuf, xifile->depth + 1); |
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
3 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!