Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libkmod: Check child range in memory mapped index #167

Closed
wants to merge 1 commit into from

Conversation

stoeckmann
Copy link
Contributor

If value of first is negative, then a broken index can trigger a stack based buffer overflow, because child_count could become larger than INDEX_CHILDMAX.

Proof of Concept:

  1. Create broken index files
MYDIR=$(mktemp -d)
KDIR=$MYDIR/lib/modules/$(uname -r)
mkdir -p $KDIR
cat > $MYDIR/bin.xz.b64 << EOF
/Td6WFoAAATm1rRGBMAinAghARwAAAAAAAAAAOX2A/rgBBsAGl0AWAHahc6kB/nBvrJW9H1Nj78L
jsj0KxaUn0kAAAA4Anj08IzoigABPpwIAAAA/lBnLLHEZ/sCAAAAAARZWg==
EOF
base64 -d $MYDIR/bin.xz.b64 | xz -cd > $KDIR/modules.alias.bin
for i in builtin.alias builtin dep symbols
do
        ln -s modules.alias.bin $KDIR/modules.$i.bin
done
  1. Run modprobe
modprobe -cd $MYDIR

On glibc based systems, you will most likely encounter a message like

*** stack smashing detected ***: terminated

Alternatively, compile with address sanitizer.

If value of "first" is negative, then a broken index can trigger a stack
based buffer overflow, because child_count could become larger than
INDEX_CHILDMAX.

Signed-off-by: Tobias Stoeckmann <[email protected]>
@@ -689,7 +689,7 @@ static struct index_mm_node *index_mm_read_node(struct index_mm *idx, uint32_t o
first = read_char_mm(&p);
last = read_char_mm(&p);

if (first > last)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, but then now we silently use a corrupted index - modprobe even returns success.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is also true for first > last or out of memory conditions with failing memory allocations here.

If it's a subtle hint to add error checks of index_mm_read_node callers: Yes, we should do that. ;) Eventually we have to adjust the API though, because index_dump itself returns void.

lucasdemarchi pushed a commit that referenced this pull request Oct 9, 2024
If value of "first" is negative, then a broken index can trigger a stack
based buffer overflow, because child_count could become larger than
INDEX_CHILDMAX.

Signed-off-by: Tobias Stoeckmann <[email protected]>
Link: #167
Signed-off-by: Lucas De Marchi <[email protected]>
@lucasdemarchi
Copy link
Contributor

Applied, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants