Commit 74de1e2e authored by tsdgeos's avatar tsdgeos Committed by Federico Mena Quintero

Make sure nSelectors is not out of range

nSelectors is used in a loop from 0 to nSelectors to access selectorMtf
which is
	UChar    selectorMtf[BZ_MAX_SELECTORS];
so if nSelectors is bigger than BZ_MAX_SELECTORS it'll do an invalid memory
access

Fixes out of bounds access discovered while fuzzying karchive
parent 5381b4f2
......@@ -287,7 +287,7 @@ Int32 BZ2_decompress ( DState* s )
GET_BITS(BZ_X_SELECTOR_1, nGroups, 3);
if (nGroups < 2 || nGroups > 6) RETURN(BZ_DATA_ERROR);
GET_BITS(BZ_X_SELECTOR_2, nSelectors, 15);
if (nSelectors < 1) RETURN(BZ_DATA_ERROR);
if (nSelectors < 1 || nSelectors > BZ_MAX_SELECTORS) RETURN(BZ_DATA_ERROR);
for (i = 0; i < nSelectors; i++) {
j = 0;
while (True) {
......
  • Hi, it seems this fix is causing a regression see bellow from https://bugs.launchpad.net/ubuntu/+source/bzip2/+bug/1834494:

    This test was made in Ubuntu disco with bzip2 1.0.6-9ubuntu0.19.04. Issue happens after that fix above was made in a security update in that distro.

    
    bunzip2 -tvv Jetson_Linux_R31.1.0_aarch64.tbz2 
      Jetson_Linux_R31.1.0_aarch64.tbz2: 
        [1: huff+mtf rt+rld]
        [2: huff+mtf rt+rld]
        [3: huff+mtf rt+rld]
        [4: huff+mtf rt+rld]
        [5: huff+mtf rt+rld]
        [6: huff+mtf rt+rld]
        [7: huff+mtf rt+rld]
        [8: huff+mtf rt+rld]
        [9: huff+mtf rt+rld]
        [10: huff+mtf rt+rld]
        [11: huff+mtf rt+rld]
        [12: huff+mtf rt+rld]
        [13: huff+mtf rt+rld]
        [14: huff+mtf rt+rld]
        [15: huff+mtf rt+rld]
        [16: huff+mtf rt+rld]
        [17: huff+mtf rt+rld]
        [18: huff+mtf rt+rld]
        [19: huff+mtf rt+rld]
        [20: huff+mtf rt+rld]
        [21: huff+mtf rt+rld]
        [22: huff+mtf rt+rld]
        [23: huff+mtf data integrity (CRC) error in data
    
    You can use the `bzip2recover' program to attempt to recover
    data from undamaged sections of corrupted files.

    Testing the previous version, without that fix, works fine.

    Edited by Leonidas S. Barbosa
  • As far as I can tell, this is the lbzip2 bug that was causing it to write more than bzip2's allowed selectors: https://github.com/kjn/lbzip2/issues/18

    Commit where it got fixed: https://github.com/kjn/lbzip2/commit/b6dc48a7b9bfe6b340ed1f6d72133608ad57144b

    The previous version of bzip2 may seem to be able to decompress files written by lbzip2 with more than the allowed number of selectors, but even then bzip2 would be writing past the end of the DState->selectorMtf[] array, into the DState->len[][].

    Why does the old code appear to work? At the time selectorMtf is initialized, I think the len arrays are still uninitialized and will get filled until later. Still, the code needs to catch malicious or corrupt data which may cause it to overflow the selectorMtf array.

    Edited by Federico Mena Quintero
  • mentioned in issue #24 (closed)

    Toggle commit list
  • I've moved this discussion to #24 (closed) - please continue it there.

Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment