Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CVE-2022-37434 zlib vulnerability #824

Closed
s1gkill opened this issue Aug 16, 2022 · 20 comments
Closed

CVE-2022-37434 zlib vulnerability #824

s1gkill opened this issue Aug 16, 2022 · 20 comments

Comments

@s1gkill
Copy link

s1gkill commented Aug 16, 2022 •
edited
Loading

Hello,

searching through the web for this vulnerability about Node + zlib, I decided to post an issue here inspired by this.

As discussed in issue mentioned above, NodeJS relies on Chromium's zlib implementation and Chromium maintainers have been applying software patches proactively. Looking both the Chromium source and NodeJS source the fix(es) applied by the zlib maintainer has not been patched yet.

Just wanted to bring this into your attention and hope to get some clarification on how would these normally be resolved?

Many thanks in advance!
@DanielRuf
Copy link
Contributor

Hi @s1gkill,

thanks for letting us know.

As far as I understand from the description, Node is probably not affected.
Can you confirm this?

Some common applications bundle the affected zlib source code but may be unable to call inflateGetHeader (e.g., see the nodejs/node reference).

https://github.com/nodejs/node/blob/75b68c6e4db515f76df73af476eccf382bbcb00a/deps/zlib/inflate.c#L762-L764

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-37434
https://nvd.nist.gov/vuln/detail/CVE-2022-37434

Also I checked the linked "patch" reference. And there the developers write this:

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/zlib@eff308a#commitcomment-80706024

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?)

madler/zlib@eff308a#commitcomment-80753451

So me it looks like the CPE entry is not correct and the CVE entry itself is also not valid. I will ask MITRE and the developers for clarification.

DanielRuf referenced this issue in madler/zlib Aug 16, 2022
Fix a bug when getting a gzip header extra field with inflate().
eff308a 
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.
@RafaelGSS
Copy link
Member

It was also referred by: nodejs/nodejs-dependency-vuln-assessments#50. I'll take a look on that to create an assessment

@s1gkill
Copy link
Author

s1gkill commented Aug 16, 2022

As far as I understand from the description, Node is probably not affected. Can you confirm this?

Some common applications bundle the affected zlib source code but may be unable to call inflateGetHeader (e.g., see the nodejs/node reference).

https://github.com/nodejs/node/blob/75b68c6e4db515f76df73af476eccf382bbcb00a/deps/zlib/inflate.c#L762-L764

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-37434 https://nvd.nist.gov/vuln/detail/CVE-2022-37434

Also I checked the linked "patch" reference. And there the developers write this:

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/zlib@eff308a#commitcomment-80706024

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?)

madler/zlib@eff308a#commitcomment-80753451

Thanks for a quick reply!

Hmm... I interpreted the comments as if they're talking about a bug that was introduced by the actual CVE vulnerability fix (so the 2 commits in zlib develop branch which has not yet been released). I also saw the commits being patched to other projects in here: curl/curl#9271, hence thought they must be important.

The original issue for myself is that this vulnerability is considered Critical by a security scanner.

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 16, 2022 •
edited
Loading

Things are getting a bit clearer now in the discussion.

@RafaelGSS
Copy link
Member

We'll need to wait for the fork update as well. We don't use madler/zlib directly. Instead, we use the https://chromium.googlesource.com/chromium/src/third_party/zlib version.

@DanielRuf
Copy link
Contributor

We'll need to wait for the fork update as well. We don't use madler/zlib directly. Instead, we use the https://chromium.googlesource.com/chromium/src/third_party/zlib version.

Understood. Linking the relevant lines that will need the changes: https://github.com/nodejs/node/blob/main/deps/zlib/inflate.c#L762-L764

@RafaelGSS RafaelGSS mentioned this issue Aug 16, 2022
Closed
@AxxlFoley
Copy link

@RafaelGSS @DanielRuf There was an update to https://chromium.googlesource.com/chromium/src/third_party/zlib today. Is this fixing the issue ?

@RafaelGSS
Copy link
Member

RafaelGSS commented Aug 26, 2022 •
edited
Loading

@RafaelGSS @DanielRuf There was an update to https://chromium.googlesource.com/chromium/src/third_party/zlib today. Is this fixing the issue ?

It should. We're working to update the zlib on Node.js any time soon. Feel free to help on nodejs/node#44254.

@RafaelGSS RafaelGSS mentioned this issue Aug 26, 2022
Closed
@madler
Copy link

madler commented Aug 26, 2022

If node.js doesn't use inflateGetHeader(), then this is a fool's errand. It probably does not, since there is nothing in the node.js zlib interface that would require its use. All you need to do is search the node.js source code for that name.

@RafaelGSS
Copy link
Member

Yes, it probably won't affect Node.js itself, but anyway it's good to have it updated.

@madler
Copy link

madler commented Aug 26, 2022

Sure, so long as no one is panicking and thinking "OMG, I need to get this updated right now!"

@RafaelGSS
Copy link
Member

Absolutely. I'll just confirm that it doesn't affect Node.js and then close this issue.

@mhdawson
Copy link
Member

@RafaelGSS thanks if you can confirm one way or the other we can then tag the issue in https://github.com/nodejs/nodejs-dependency-vuln-assessments/issues as to whether it affects node.js or not.

@mhdawson
Copy link
Member

mhdawson commented Aug 26, 2022 •
edited
Loading

@madler thanks for the suggestion, I should have thought of checking that earlier :)

@RafaelGSS
Copy link
Member

Sorry for the delay. Node.js doesn't use the inflateGetHeader() method, therefore it's not affected. To follow the zlib update, see: nodejs/node#44412

@Neustradamus
Copy link

@madler has done the new build, the 1.2.13 has been released with the CVE-2022-37434 fix.

@mhdawson
Copy link
Member

@RafaelGSS did you talk to @facutuesca about helping with doing a zlib update? Seems like a good time to make sure we can now that a patch is available.

@RafaelGSS
Copy link
Member

Yes, I did. He will be available to do that after Oct 24.

@facutuesca
Copy link

I saw that the PR (nodejs/node#44412) for updating zlib was fixed and now passes CI. Is there anything else needed to merge?

@RafaelGSS
Copy link
Member

We're waiting for the Benchmark CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@Neustradamus @madler @DanielRuf @facutuesca @mhdawson @AxxlFoley @RafaelGSS @s1gkill