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.

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

[release/6.0-staging] Zlib: Update zlib to v1.2.13, intel-zlib to v1.2.13_jtk; add memory protections #93634

Merged

Conversation

GrabYourPitchforks
Copy link
Member

This is a backport of #84602 and #84604 to the release/6.0-staging branch. These PRs were originally backported to 7.0 via #89517 and #89532. Since the directory structure changed quite a bit here, I'm submitting this PR manually instead of relying on the bot to backport.

Customer Impact

We're getting reports of compliance tooling flagging our distribution of zlib. (And we've gotten flagged internally by Component Governance on this.) While there are no current CVEs affecting how we use zlib, upgrading to the latest version will silence these alerts and restore the compliance status.

This also adds defense-in-depth protections to the allocator used by zlib to help mitigate the risk posed by potential future CVEs against this library. Basic defenses against use-of-uninitialized-memory bugs, local buffer overruns, and double-free bugs are provided. The overall goal is that should a future CVE be found that fits one of those categories, the CVE's nominal severity can drop from Critical -> Important or from Important -> Moderate because of the difficulty of successful exploit.

Testing

We have a full suite of unit tests and performance tests. Additionally, this change has been baking in 8.0 for quite some time and was backported to 7.0 as part of the Aug 2023 servicing update. No regressions have yet been reported.

See #89532 (comment) for more information on how a Frankenbuild was created for local smoke-testing.

Risk

Medium-low for the zlib update. Servicing a dependency always carries the risk of introducing new bugs. However, skimming through https://github.com/madler/zlib/commits/develop, I don't see any recent changes which indicate that 1.2.13 introduces serious bugs that needed addressing. I also cherry-picked madler/zlib@e554695 out of an abundance of caution. (This same change went in to the 7.0 backport.)

The introduction of the memory allocator is low-risk. It takes advantage of OS-specific capabilities when available. On Windows, we avoid the use of a new heap when running as a 32-bit process, since memory might be at a premium. Additionally, at Tactics's request, an opt-out switch is provided.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@ghost
Copy link

ghost commented Oct 17, 2023

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a backport of #84602 and #84604 to the release/6.0-staging branch. These PRs were originally backported to 7.0 via #89517 and #89532. Since the directory structure changed quite a bit here, I'm submitting this PR manually instead of relying on the bot to backport.

Customer Impact

We're getting reports of compliance tooling flagging our distribution of zlib. (And we've gotten flagged internally by Component Governance on this.) While there are no current CVEs affecting how we use zlib, upgrading to the latest version will silence these alerts and restore the compliance status.

This also adds defense-in-depth protections to the allocator used by zlib to help mitigate the risk posed by potential future CVEs against this library. Basic defenses against use-of-uninitialized-memory bugs, local buffer overruns, and double-free bugs are provided. The overall goal is that should a future CVE be found that fits one of those categories, the CVE's nominal severity can drop from Critical -> Important or from Important -> Moderate because of the difficulty of successful exploit.

Testing

We have a full suite of unit tests and performance tests. Additionally, this change has been baking in 8.0 for quite some time and was backported to 7.0 as part of the Aug 2023 servicing update. No regressions have yet been reported.

See #89532 (comment) for more information on how a Frankenbuild was created for local smoke-testing.

Risk

Medium-low for the zlib update. Servicing a dependency always carries the risk of introducing new bugs. However, skimming through https://github.com/madler/zlib/commits/develop, I don't see any recent changes which indicate that 1.2.13 introduces serious bugs that needed addressing. I also cherry-picked madler/zlib@e554695 out of an abundance of caution. (This same change went in to the 7.0 backport.)

The introduction of the memory allocator is low-risk. It takes advantage of OS-specific capabilities when available. On Windows, we avoid the use of a new heap when running as a 32-bit process, since memory might be at a premium. Additionally, at Tactics's request, an opt-out switch is provided.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: GrabYourPitchforks
Assignees: -
Labels:

Servicing-consider, area-System.IO.Compression

Milestone: -

@GrabYourPitchforks GrabYourPitchforks changed the title [6.0] Zlib: Update zlib to v1.2.13, intel-zlib to v1.2.13_jtk [release/6.0-staging] Zlib: Update zlib to v1.2.13, intel-zlib to v1.2.13_jtk; add memory protections Oct 17, 2023
@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Oct 17, 2023

The PR is divided into three primary commits to make it easier to review.

  • 7181f61: The actual upgrade of zlib and zlib-intel, including updating license files. These files were copied from the 7.0 directory. Note the removal of the PDF document, as discussed in the 7.0 and 8.0 work items.
  • 9cbc6e7: Adding the missing cgmanifest.json entries so that Component Governance can properly detect our usage of the zlib sources.
  • c63d759: Adding the custom memory allocator as a defense-in-depth protection.

It's all tiny cleanup beyond those three commits.

Note also that I did not hook the custom allocator up to the mono copy of zlib. From tracing the code backward, it appears that copy of zlib is used only for mono's loading of trusted PPDB files, which means that this implementation of zlib is not exposed to hostile input. The normal implementation accessed through the System.IO.Compression API surface is protected by the new allocator, even on mono.

@GrabYourPitchforks GrabYourPitchforks added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 18, 2023
@carlossanlop carlossanlop merged commit 3ae53c0 into dotnet:release/6.0-staging Oct 18, 2023
149 of 151 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants