[SECURITY] Implementation of readUvarint vulnerable to CVE-2020-16845

[0xdecaf]

Implementation of readUvarint at https://github.com/ulikunitz/xz/blob/master/bits.go#L56 is very similar to the vulnerable code in the Golang encoding/binary library and seems to suffer from the same vulnerability described in https://github.com/golang/go/issues/40618.

See the fix at https://go-review.googlesource.com/c/go/+/247120/2/src/encoding/binary/varint.go

Note: I couldn't find any information on how to disclose this issue to the maintainers. I would also suggest setting up a Security Policy for the project within GitHub

[ulikunitz]

I confirm the issue and I will implement a patch and will have a new release before the end of the week.

An attacker could construct a byte sequence so that readUvarint would not stop to consume bytes.

[ulikunitz]

Release v0.5.8 fixes the issue.

The actual commit with the change is 69c6093.

[ulikunitz]

I published security advisory GHSA-25xm-hr59-7c27

[0xdecaf]

Fantastic, thank you!

[p-rog]

If this issue is similar to that what was detected in Golang Standard Library (CVE-2020-16845), then for this case there should be assigned separate CVE ID. Just to make it clear that it's not exactly the issue in Go, but other security issue in this repository, which is similar to that what was detected in Go stdlib.

Could you please request a new CVE for this vulnerability? You can raise a CVE request in Github/Snyk or Red Hat (I can assist you with the last one, just let me know if you need help with that).

[ulikunitz]

You are right. Because the xz module is not the golang stdlib the advisory should have its own CVE. I have now requested a CVE through the Github Security Advisory interface. I was not aware of that option.

[p-rog]

Thank you for the prompt response. When the new CVE will be assigned, kindly please share it here and please also update the GHSA-25xm-hr59-7c27 advisory.

[ulikunitz]

I got the following information:

GitHub has issued CVE-2021-29482 for this Security Advisory after reviewing it for compliance with CVE rules. Since you've already published this Security Advisory, we'll publish this CVE to the CVE List.

[p-rog]

Thank you @ulikunitz !

[dsnet]

Hello! GitHub's dependabot flagged our repo has a having a "high severity" security risk. I don't believe this is a "high severity" issue or really an issue at all.

The only way this can be an issue is if the attacker provides an infinite number of bytes to keep the read loop going forever. I would only classify this as "high severity" if a finite input could cause infinite looping (which is not the case here).

Note that there are already many ways that an input could cause the decompressor to spend CPU time and make no progress. For example, the attacker could trivially provide a sequence of valid XZ streams that all decompress to nothing. That is functionally equivalent to the security hole being plugged here.

Fundamentally, any application that reads untrusted XZ input needs to enforce limits on the amount of input read and amount of output that may be produced.

While it's easy for us to upgrade our dependency the latest version, I suspect this might be causing unnecessary churn for users for what I would argue is not a security risk at all.

\cc @danderson

[ulikunitz]

Many thanks for the input.

It was clearly an issue that had a fix. It was not a statement that there are no other ways to provide input that may consume CPU time or have other negative effects.

I have been new to the vulnerability management in Github and the consequences. The rating was based of the rating of the exact same issue for the Golang library. I will review the rating definition and whether the rating was appropriate for the issue in the context of the xz library. At this time I don't know whether I can change the rating retrospectively, if my review results in a lower rating.

I will also review the example you provided. Should it highlight an issue that is not inherent in the protocol and there is actually a fix, I will create another issue and fix this new issue as well. I'm not sure whether I will create another security vulnerability report for it. Personally I believe that this vulnerability management forces unpaid developers to do compliance work for corporate enterprises. It is still manageable, but I have spent already too much time on the vulnerability management for this relatively simple issue and fix.

[dsnet]

I will also review the example you provided. Should it highlight an issue that is not inherent in the protocol and there is actually a fix,

The point I was trying to make is that this class of "attack" is inherent to the XZ format (and pretty much every compression format actually).

The example I provided is equivalent to:

(while true; do echo -n | xz; done) | xz -d

You can replace xz with gzip, bzip2, or zstd and the behavior is the same (i.e., decompressor will never output anything).

Essentially, every modern compression format has a means through which an infinite amount of input can result in lots of CPU churn and zero output. Thus, my argument is that this shouldn't be considered a security risk. Otherwise, the logical extension is that the use of decompression at all for any format itself is a security risk, which seems like an absurd conclusion.

Personally I believe that this vulnerability management forces unpaid developers to do compliance work for corporate enterprises. It is still manageable, but I have spent already too much time on the vulnerability management for this relatively simple issue and fix.

Thank you for taking the time to try to work through vulnerability management, even though I'm arguing that it was unnecessary for this case.

As a fellow maintainer of OSS, I've also received security reports reported on projects I maintain. I usually push back and question the assertion that something is truly a security risk. I usually ask myself what the worst possible outcome that can happen with the reported flaw: Does it lead to a panic? Does it lead to arbitrary code execution? Can a finite input cause the program to loop forever or allocate unreasonable amounts of memory? Etc.

In this case, the only way someone could use this property to mount an attack is if the attacker commits resources proportional to the amount that the server itself will waste. It would be much more concerning if the attacker can send a tiny XZ payload that causes the server to crash, burn lots of CPU, or allocate lots of memory.

[p-rog]

@dsnet - your arguments are good, but what is the definition of the vulnerability? Security Vulnerability is unexpected behavior/result to some specific action. In this case wrong usage of ReadUvarint function, like in Go case CVE-2020-16845.

The point I was trying to make is that this class of "attack" is inherent to the XZ format (and pretty much every compression format actually).

The example I provided is equivalent to: (while true; do echo -n | xz; done) | xz -d

You can replace xz with gzip, bzip2, or zstd and the behavior is the same (i.e., decompressor will never output anything).

And this is an example when this issue is not a vulnerability, because this is an expected result, right? When the function/module/application works like it supposed to work, then such issues shouldn't be classified as CVE worthy vulnerabilities.

I agree that there are many flaws where exploitation is very unlikely, but as long as it's in "vulnerability" definition scope, it should be classified and get assigned CVE.

@ulikunitz - your decision to keep the CVSS similar to what is in the Go CVE (CVE-2020-16845) was correct in my opinion. It assumes the worst possible attack scenario and consequences. Maybe there could be a discussion if the Availability metric should really be High. In regards to the Severity Ratings, by default NVD assign impact based on the total CVSS score (see https://www.first.org/cvss/specification-document section 5). But always it should be in the owner of the product/application scope to decide how their products/applications are impacted. For example Red Hat classified this flaw with Moderate impact (https://access.redhat.com/security/cve/cve-2021-29482).

[ulikunitz]

I have reviewed the rating and I get the same CVSS 3.1 base score 7.5 as the Go CVE, which makes it a high risk vulnerability.

It is clearly a violation of the xz specification section 1.2, which limits the number of supported integer bits. A sufficient large number of input bytes makes this an availability issue with high impact that could be exploited over the network. Infinite input is not required. The CVSS base score for such a vulnerability is 7.5, making it a high risk vulnerability. You might disagree with the CVSS scoring methodology, but it has to be applied here to allow a comparison of vulnerabilities.

The concatenation of empty files is supported by the specification and is therefore supported by the library. If that leads to availability issues it has to be handled by the code using the library, e.g. by limiting the number of input bytes supported. So I don't regard the example as an issue for the library that requires fixing.

[ulikunitz]

Just looked in detail to the Red Hat medium rating. They state only that they use CVSS ratings as guidance, but they are not stating what criteria they are actually using. I don't support this approach because it makes vulnerability rating arbitrary.

In my professional life we are always reviewing all vulnerabilities in security reports and target to fix all of them and use the rating only for prioritization. Low risk issues might be fixed as part of the normal release cycle. If we exclude a reported vulnerability from fixing it requires a justification that is approved by management.

[p-rog]

Just looked in detail to the Red Hat medium rating. They state only that they use CVSS ratings as guidance, but they are not stating what criteria they are actually using. I don't support this approach because it makes vulnerability rating arbitrary.

In regards to the CVSS, Red Hat uses CVSS 3.1 standard. Only the severity rating is not based the CVSS total score, but is precisely defined and explained here: https://access.redhat.com/security/updates/classification

[ulikunitz]

My statement has been wrong, I retract it here. It doesn't add anyway to the discuission.