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/7.0-staging] Zlib: Add some protections to the allocator used by zlib #89532

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 26, 2023

Backport of #84604 to release/7.0-staging

/cc @GrabYourPitchforks

Customer Impact

This 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.

See #84604 for more information.

Testing

We have a full suite of unit tests and performance tests. Additionally, this change has been baking in the 8.0 preview branches for several months. No regressions have yet been reported.

Risk

Low. This has been baking in the 8.0 branches for a while and no regressions have been reported. Additionally, at Tactics's request, an opt-out switch is provided.

Note: coreclr on Linux does not use the internal zlib implementation anyway. It's instead used in wasm and some mono scenarios.

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 Jul 26, 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

Backport of #84604 to release/7.0-staging

/cc @GrabYourPitchforks

Customer Impact

Testing

Risk

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: github-actions[bot]
Assignees: -
Labels:

area-System.IO.Compression

Milestone: -

@carlossanlop carlossanlop added this to the 7.0.x milestone Jul 26, 2023
@carlossanlop
Copy link
Member

Thanks for backporting. Approving as area owner. Please fill out the template, add the servicing-consider label, and send email to Tactics for approval.

@GrabYourPitchforks GrabYourPitchforks added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 26, 2023
@GrabYourPitchforks
Copy link
Member

Marked no-merge since Tactics wanted a compat switch before this PR went in.

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Aug 1, 2023

Most recent two commits add the environment variable for Win & Unix, respectively.

You can set either of these two values to disable the mitigations:

  • DOTNET_SYSTEM_IO_COMPRESSION_DISABLEZLIBMITIGATIONS=1
  • DOTNET_SYSTEM_IO_COMPRESSION_DISABLEZLIBMITIGATIONS=true (value must be lowercase)

Testing was accomplished by attaching a debugger and stepping through, ensuring that the correct paths were taken depending on the environment variable. For Unix testing, I created a custom build with the INTERNAL_ZLIB build variable defined, so that my custom Unix builds would use our zlib logic instead of the machine's global zlib implementation. Then I could observe the different code paths being taken in response to flipping the environment variable.

Ideally I'd be able to use pal.h across both files so that I have a common implementation of GetEnvironmentVariableA and InterlockedCompareExchange. But pal.h is being finnicky when pulled into external libs. So I figured the path of least resistance was to use getenv and the Unix ICE equivalent. (Note: argument ordering to the ICE routine differs between Unix and Windows!)

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

LGTM with the env vars.

- Mono defines HOST_WIN32, not CLR_CMAKE_HOST_WIN32
- Follow same pattern from src\native\eventpipe\CMakeLists.txt
@GrabYourPitchforks
Copy link
Member

I might want to cherry-pick 21113a1 back into main so that mono on Windows gets the same benefit CoreCLR does.

@carlossanlop
Copy link
Member

Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th.

@GrabYourPitchforks GrabYourPitchforks added Servicing-consider Issue for next servicing release review and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Aug 2, 2023
@jeffhandley jeffhandley added api-approved API was approved in API review, it can be implemented and removed Servicing-consider Issue for next servicing release review api-approved API was approved in API review, it can be implemented labels Aug 3, 2023
@jeffhandley jeffhandley added the Servicing-approved Approved for servicing release label Aug 3, 2023
@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.11 Aug 3, 2023
@carlossanlop carlossanlop merged commit 395c507 into release/7.0-staging Aug 3, 2023
174 of 180 checks passed
@carlossanlop carlossanlop deleted the backport/pr-84604-to-release/7.0-staging branch August 3, 2023 17:17
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 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.

3 participants