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

Fix potential OOB access during huffman decompression #396

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Caball009
Copy link
Contributor

If 'readsize' is equal to the size of the input buffer, there's potentially OOB access because the while loop increases the bit offset and only checks the nodes from the huffman tree and not whether the number of bits exceeds 'readsize'. This won't lead to problems if the size of input buffer is greater than 'readsize' but still a design flaw.

If 'readsize' is equal to the size of the input buffer, there's potentially OOB access because the while loop increases the bit offset and only checks the nodes from the huffman tree and not whether the number of bits exceeds 'readsize'. This won't lead to problems if the size of input buffer is greater than 'readsize' but still a design flaw.
…ompression

If the OOB check is only included in the while evaluation, it's possible the 0x07 / EOF byte is never returned from this function.
@@ -61,6 +61,13 @@ static void Huff_offsetReceive( node_t *node, int *ch, byte *fin, int *offset )
node = node->left;

}

if ( bloc >= readsize ) {
//Com_PrintError("OOB buffer access\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's worth printing. Why comment it? 😄

@raphael12333
Copy link

raphael12333 commented Sep 25, 2024

Hi, why this is not merged?
Is it because the cod4x modified MSG_ReadBitsCompress and the check decompressMsg.cursize == decompressMsg.maxsize are enough to prevent control access?

Can't server still get crashed if not fixing Huff_offsetReceive? (ioquake/ioq3@d2b1d12)

@proxict
@IceNinjaman

@raphael12333
Copy link

ah my bad, i thought this project was still active

@proxict
Copy link
Contributor

proxict commented Sep 26, 2024

Hi, it's not merged because it's not tested yet.
There are currently other things being worked on in cod4x in the background.

@raphael12333
Copy link

raphael12333 commented Sep 26, 2024

Hi, it's not merged because it's not tested yet. There are currently other things being worked on in cod4x in the background.

Ah ok, some exploits don't seem easy to do
i prefer to test securities before adding them too, but when i see it's too hard for me, i prefer to add anyway, rather than hoping someday i would manage to test, if it's not too late

@proxict
Copy link
Contributor

proxict commented Sep 26, 2024

If this bug was easily exploitable, it wouldn't have been publicly disclosed before fixing it.

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

Successfully merging this pull request may close these issues.

4 participants