C++: Decompression Bombs

[am0o0]

This is part of All for one, one for all query submission, I'm going to submit an issue in github/securitylab for this pull request too.
this query will be upgraded more in this week whether in this pull request or in another pull request, currently I'm adding minizip, xz, bzip2, zstd, LZ4 related libraries.
I'm trying my best to add proper sanitizers too if there is any protection for each function as some functions are unsafe by default and can not be controlled during decompression.

[am0o0]

Anyone not watching the repo in general, note this is part of a family of submissions:

#13553
#13554
#13555
#13556
#13557
#13558
#13560

[github-actions]

QHelp previews:

cpp/ql/src/experimental/Security/CWE/CWE-409/DecompressionBombs.qhelp

User-controlled file decompression

Extracting Compressed files with any compression algorithm like gzip can cause denial of service attacks.

Attackers can compress a huge file consisting of repeated similiar bytes into a small compressed file.

Recommendation

When you want to decompress a user-provided compressed file you must be careful about the decompression ratio or read these files within a loop byte by byte to be able to manage the decompressed size in each cycle of the loop.

Example

Reading an uncompressed Gzip file within a loop and check for a threshold size in each cycle.

#include "zlib.h"

void SafeGzread(gzFile inFileZ) {
    const int MAX_READ = 1024 * 1024 * 4;
    const int BUFFER_SIZE = 8192;
    unsigned char unzipBuffer[BUFFER_SIZE];
    unsigned int unzippedBytes;
    unsigned int totalRead = 0;
    while (true) {
        unzippedBytes = gzread(inFileZ, unzipBuffer, BUFFER_SIZE);
        totalRead += unzippedBytes;
        if (unzippedBytes <= 0) {
            break;
        }

        if (totalRead > MAX_READ) {
            // Possible decompression bomb, stop processing.
            break;
        } else {
            // process buffer
        }
    }
}

The following example is unsafe, as we do not check the uncompressed size.

#include "zlib.h"

void UnsafeGzread(gzFile inFileZ) {
    const int BUFFER_SIZE = 8192;
    unsigned char unzipBuffer[BUFFER_SIZE];
    unsigned int unzippedBytes;
    while (true) {
        unzippedBytes = gzread(inFileZ, unzipBuffer, BUFFER_SIZE);
        if (unzippedBytes <= 0) {
            break;
        }

        // process buffer
    }
}

References

• Zlib documentation
• An explanation of the attack
• Common Weakness Enumeration: CWE-409.

[am0o0]

Hi, I've completed the work on this query and I don't have any further updates/commits here.

[jketema]

@am0o0 I've opened am0o0#1 to do some clean up on this. If you're happy with the changes there, please merge and the changes should propagate to here. Given that you've indicated that you have little time, and since the bounty program is ending, this seems to be the best way forward to get this PR merged.

Also, I failed to finalize the ZSTD tests because I thought I needed a special flow step that I couldn't write.

Could you point me to the part of the code where you need an additional flow step, then I can have a look.

[am0o0]

@jketema thank you alot for helping on this PR!

[jketema]

You're welcome. How about the following?

Also, I failed to finalize the ZSTD tests because I thought I needed a special flow step that I couldn't write.

Could you point me to the part of the code where you need an additional flow step, then I can have a look.

[am0o0]

@jketema I created a review of the tests. I mentioned you to receive a notification. IDK why the review doesn't exist in the main in here.

[jketema]

@jketema I created a review of the tests. I mentioned you to receive a notification. IDK why the review doesn't exist in the main in here.

I haven't received any notifications (as of yet).

[am0o0]

Ahh, the link of the comment(it wasn't a review):
https://github.com/github/codeql/pull/13560/files#diff-4812bb9c0e487330fa7d59e6c81cbc79f10a8838c8944edce71fa03d586c048fR61-R66

[jketema]

Ahh, the link of the comment(it wasn't a review): https://github.com/github/codeql/pull/13560/files#diff-4812bb9c0e487330fa7d59e6c81cbc79f10a8838c8944edce71fa03d586c048fR61-R66

Thanks. I've opened am0o0#2 to address this. This can probably be improved, but matches what you did for minizip.

[geoffw0]

Generally LGTM.

• great range of modelled sources and flow steps.
• I only checked a few of them are accurate (the rest do look reasonable).
• there are some minor issues with the documentation, but should be OK for experimental.
• test coverage is great, though lacking any "good" cases.
• quite a lot of results on MRVA (I guess because decompression is common and we model several libraries here); the results I examined looked plausible, but it's likely we'd want to take a closer look before promoting this query out of experimental.
• CI is failing.

[jketema]

@am0o0 PR to fix the CI problem @geoffw0 mentions above: am0o0#3

[jketema]

LGTM. Thanks for your contribution!