Incorrect security remedy for adm_zip

Clean Code Report False-positive / False-negative...
,
1

I believe that the remedy for adm_zip incorrectly targets high compression ratios. It’s valid and common for compression ratios to be high for highly compressible files, well over the specified THRESHOLD_RATIO of 10 (compression levels only run from 1 - 9 for the zip protocol, but ratios are dynamic).

This blocked valid and innocuous zip files for us.

let compressionRatio = entrySize / zipEntry.header.compressedSize;
    if (compressionRatio > THRESHOLD_RATIO) {
        throw 'Reached max. compression ratio';
    }

The file size check provides adequate protection against disk. I’m not sure if this check was meant to protect against cpu. If so, then I think a combination of file size and compression ratio would be necessary but might have to be determined heuristically.

I have not reviewed suggestions for the other zip libraries, only adm_zip, but the same might apply.

I’m new to SonarQube, have used the lint rules for years but only recently the security rules. I’m not sure if this is the correct place to report such issues.

4

Hi Terry,

Welcome to the community and thanks for the report! This check is intended to prevent zip bombs. As with the MAX_SIZE and MAX_FILES, the threshold is picked rather arbitrarily as different situations require different limits. So it is absolutely acceptable to increase this limit if it is too low for your case.

I agree that a smarter check would be nice. In the not-too-distant future, we plan to revisit all hotspots, so I think that will be a good time as well to try to come up with a smarter solution here than what we currently have - if possible.

1 Like
6

Thanks for the response, it’s good to know that I didn’t misinterpret the results. Short of reimplementing the approach I think just a comment “Adjust these settings to meet your use case” would be helpful to future implementers.

1 Like