LibCompress: Fix reaching end-of-stream without collecting the required number of bits

[xspager]

This Fix #22130

[BuggieBot]

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why. Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

[timschumi]

This doesn't make much sense, since these two usages are precisely the usages which we meant to fix with the original patch. I am also very sure that I tested unzip manually, and none of the other tests from our collection have failed.

[xspager]

@timschumi, I went back and looked if we have any zip file that I could try on the system image and I found one and this one I can decompress without a problem. I noticed that the zip files are not of the same type, if I run file on the zip file that ends up at ~/Documents/zip/archive.zip it prints: Zip archive data, at least v1.0 to extract, compression method=store For the files I'm trying to decompress I get: Zip archive data, at least v2.0 to extract, compression method=deflate

You can try decompressing this .rock file https://github.com/rocks-moonscript-org/moonrocks-mirror/raw/master/kong-lapis-1.16.0.1-1.src.rock

[timschumi]

I noticed that the zip files are not of the same type, if I run file on the zip file that ends up at ~/Documents/zip/archive.zip it prints: Zip archive data, at least v1.0 to extract, compression method=store For the files I'm trying to decompress I get: Zip archive data, at least v2.0 to extract, compression method=deflate

It doesn't seem like this has any particular effect, other than that a ZIP using the compression method store would never invoke DEFLATE in the first place. I have a bunch of files here that are Zip archive data, at least v2.0 to extract, compression method=deflate, and none of them fail to decompress.

You can try decompressing this .rock file rocks-moonscript-org/moonrocks-mirror@master/kong-lapis-1.16.0.1-1.src.rock (raw)

That one indeed fails to decompress, but the file name makes me believe that it could be a custom format that is based on ZIP. I'll try and look into it, but I still strongly doubt that enabling beyond-EOF zero filling is correct here.

[xspager]

@timschumi, I decided to learn about DEFLATE and that will take a while.

Let me share with you an observation about that .rock file I linked. Only two files inside the Zip file require a going beyond-EOF: lapis/spec/lua_request_spec.lua, lapis/bin/lapis

I had a look around and can't find anything unusual about the first file metadata inside the Zip file . Also I used warnln to print how many bits it was necessary to add and it was just one for both files that faild to decompress with Serenity's unzip without my PR.

[timschumi]

Haven't forgotten about this, just need to see when I have enough time on hand to start reducing the relevant test cases.

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

[LucasChollet]

I gave more details in https://github.com/SerenityOS/serenity/issues/22130#issuecomment-1890335274, but while the issue is valid, this patch isn't the proper solution.

This doesn't make much sense, since these two usages are precisely the usages which we meant to fix with the original patch.

I can confirm that. With this patch, this test provokes an infinite loop.

TEST_CASE(ossfuzz_58046) {
    Array<u8, 35> const compressed {
        0x04, 0x20, 0x04, 0x49, 0xff, 0xbf, 0x9e, 0x10, 0x04, 0x20, 0x04, 0x49, 0xff, 0xbf, 0x49, 0x05, 0xc0, 0x9e, 0x10, 0x04, 0x20, 0x04, 0x49, 0xff, 0xbf, 0x9e, 0x10, 0x04, 0x20, 0xbf, 0x9e, 0xbc, 0x04, 0x9e, 0x10
    };
    auto const decompressed = TRY_OR_FAIL(Compress::DeflateDecompressor::decompress_all(compressed));
}