Skip to content

Commit

Permalink
Fix a bug when getting a gzip header extra field with inflate().
Browse files Browse the repository at this point in the history
If the extra field was larger than the space the user provided with
inflateGetHeader(), and if multiple calls of inflate() delivered
the extra header data, then there could be a buffer overflow of the
provided space. This commit assures that provided space is not
exceeded.
  • Loading branch information
Mark Adler committed Jul 30, 2022
1 parent b8bd098 commit eff308a
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions inflate.c
Original file line number Diff line number Diff line change
Expand Up @@ -763,9 +763,10 @@ int flush;
copy = state->length;
if (copy > have) copy = have;
if (copy) {
len = state->head->extra_len - state->length;
if (state->head != Z_NULL &&
state->head->extra != Z_NULL) {
len = state->head->extra_len - state->length;
state->head->extra != Z_NULL &&
len < state->head->extra_max) {
zmemcpy(state->head->extra + len, next,
len + copy > state->head->extra_max ?
state->head->extra_max - len : copy);
Expand Down

18 comments on commit eff308a

@bagder
Copy link

@bagder bagder commented on eff308a Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit causes a segfault that we can reproduce with curl: curl/curl#9271

@piru
Copy link

@piru piru commented on eff308a Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If state->head is NULL this new code will dereference zero page and crash.

@madler
Copy link
Owner

@madler madler commented on eff308a Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piru Thank you for the report. Fixed in 1eb7682 .

@madler
Copy link
Owner

@madler madler commented on eff308a Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bagder Can you try that again with the zlib develop branch up through 1eb7682 ? Thanks.

@ljavorsk
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @madler was this CVE introduced in the zlib-1.2.12 version, or this CVE affects the older 1.2.11 version as well?

@madler
Copy link
Owner

@madler madler commented on eff308a Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither. This bug is not present in any released version of zlib. It was introduced two commits ago on the develop branch of zlib. (Is it standard practice to issue CVEs for unreleased code?)

@ljavorsk
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I get it now. I was talking about CVE-2022-37434.

Looking at it now, this commit fixes the CVE-2022-37434 and the commit 1eb7682 fixes the segfault introduced by this commit. Is this assumption correct?

@madler
Copy link
Owner

@madler madler commented on eff308a Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

@DanielRuf
Copy link

@DanielRuf DanielRuf commented on eff308a Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it standard practice to issue CVEs for unreleased code?

That is definitely not standard practice.
To me this CVE entry is merely a false-positive (from a technical point of view, since no affected version was released) if my assumption turns out to be right.

image

https://nvd.nist.gov/vuln/detail/CVE-2022-37434#range-8202095

@piru
Copy link

@piru piru commented on eff308a Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRuf

Looking at https://github.com/madler/zlib/blame/master/inflate.c the bug is at least 11 years old. It appears the affected code was committed in 9811b53 as zlib 1.2.2.1

@DanielRuf
Copy link

@DanielRuf DanielRuf commented on eff308a Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piru thanks for checking this.

Hm, I'm not sure why but the changelog says 2004, but the commit was in 2011.
Maybe the project was uploaded to GitHub afterwards and the commit timestamps are not the right ones.

https://zlib.net/fossils/ says also 2004. So if this is the first affected version (which would have to be verified), then it means that we at least have a definitive list of affected versions and a relevant CPE (1.2.2.1 - <= 1.2.12).
See also https://github.com/madler/zlib/commit/9811b53dd9e8f67015c7199fff12b5bfc6965330.patch

So if the changelog is right, then all versions since 2004 (1.2.2.1) are affected?

zlib/ChangeLog

Line 801 in 21767c6

- Add inflateGetheader() to retrieve gzip headers

Can someone verify this?
Because this contradicts the statement from eff308a#commitcomment-80753451:

Neither. This bug is not present in any released version of zlib. It was introduced two commits ago on the develop branch of zlib. (Is it standard practice to issue CVEs for unreleased code?)

Which was in reply to eff308a#commitcomment-80706024:

Hi, @madler was this CVE introduced in the zlib-1.2.12 version, or this CVE affects the older 1.2.11 version as well?

Or maybe there is some misunderstanding, which needs some clarification.

@nmoinvaz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were talking about a bug caused by this commit that was fixed in 1eb7682.

@piru
Copy link

@piru piru commented on eff308a Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's of course possible that the code originates from even earlier. It as at least 11 years old.

There appears to be confusion between the actual bug causing the vulnerability in the first place and the buggy (initial) fix for the vulnerability.

The initial fix was never released and it was buggy, but that doesn't make the CVE invalid in any way.

@nmoinvaz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to create a GH issue for all CVE-2022-37434 discussion.

@mhdawson
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piru based on your clarification my understanding is that this does affect existing releases. Is there a release planned with the fix? From https://zlib.net/ the most recent releases looks like it came out March 27 which I assume was before this issue.

@borrrden
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the CVE there are no released versions that contain the fix. Is there a way to get a notification when there is one?

@yoowonsuk
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borrrden
nope. but i mention you

@DanielRuf
Copy link

@DanielRuf DanielRuf commented on eff308a Oct 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.