Skip to content

Zlib: Add some protections to the allocator used by zlib - #84604

Merged
GrabYourPitchforks merged 6 commits into
dotnet:mainfrom
GrabYourPitchforks:zlib/main
Apr 21, 2023
Merged

Zlib: Add some protections to the allocator used by zlib#84604
GrabYourPitchforks merged 6 commits into
dotnet:mainfrom
GrabYourPitchforks:zlib/main

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Apr 11, 2023

Copy link
Copy Markdown
Member

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.

The .c files included in this PR describe the defenses in more detail.

Windows benchmark results:

MethodToolchainlevelfileMeanErrorStdDevRatioRatioSD
CompresscompareOptimalC:\pe(...)t.pdf [93]3,729.7 μs715.06 μs39.19 μs1.030.03
CompressmainOptimalC:\pe(...)t.pdf [93]3,606.6 μs1,178.33 μs64.59 μs1.000.00
DecompresscompareOptimalC:\pe(...)t.pdf [93]404.5 μs41.28 μs2.26 μs0.950.03
DecompressmainOptimalC:\pe(...)t.pdf [93]427.2 μs264.41 μs14.49 μs1.000.00
CompresscompareOptimalC:\pe(...)9.txt [88]5,210.0 μs1,589.48 μs87.12 μs0.960.11
CompressmainOptimalC:\pe(...)9.txt [88]5,504.3 μs12,272.10 μs672.68 μs1.000.00
DecompresscompareOptimalC:\pe(...)9.txt [88]521.7 μs280.13 μs15.35 μs1.010.03
DecompressmainOptimalC:\pe(...)9.txt [88]517.0 μs201.16 μs11.03 μs1.000.00
CompresscompareOptimalC:\pe(...)a\sum [80]777.8 μs466.43 μs25.57 μs1.010.02
CompressmainOptimalC:\pe(...)a\sum [80]772.3 μs413.65 μs22.67 μs1.000.00
DecompresscompareOptimalC:\pe(...)a\sum [80]157.2 μs98.73 μs5.41 μs1.000.03
DecompressmainOptimalC:\pe(...)a\sum [80]157.4 μs66.97 μs3.67 μs1.000.00
CompresscompareFastestC:\pe(...)t.pdf [93]2,658.7 μs1,499.52 μs82.19 μs1.030.04
CompressmainFastestC:\pe(...)t.pdf [93]2,579.9 μs2,456.89 μs134.67 μs1.000.00
DecompresscompareFastestC:\pe(...)t.pdf [93]382.7 μs117.18 μs6.42 μs0.960.04
DecompressmainFastestC:\pe(...)t.pdf [93]398.6 μs173.96 μs9.54 μs1.000.00
CompresscompareFastestC:\pe(...)9.txt [88]1,697.4 μs420.08 μs23.03 μs1.000.01
CompressmainFastestC:\pe(...)9.txt [88]1,701.4 μs215.07 μs11.79 μs1.000.00
DecompresscompareFastestC:\pe(...)9.txt [88]591.7 μs346.91 μs19.02 μs1.040.04
DecompressmainFastestC:\pe(...)9.txt [88]568.1 μs233.40 μs12.79 μs1.000.00
CompresscompareFastestC:\pe(...)a\sum [80]347.1 μs166.74 μs9.14 μs0.980.08
CompressmainFastestC:\pe(...)a\sum [80]355.5 μs457.27 μs25.06 μs1.000.00
DecompresscompareFastestC:\pe(...)a\sum [80]151.5 μs133.82 μs7.34 μs0.970.05
DecompressmainFastestC:\pe(...)a\sum [80]155.9 μs121.91 μs6.68 μs1.000.00

Though the three PRs #84602, #84603, and #84604 are all related to zlib; they're each fully standalone and can be reviewed as isolated units.

@ghost

Copy link
Copy Markdown

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

The .c files included in this PR describe the defenses in more detail.

Windows benchmark results:

MethodToolchainlevelfileMeanErrorStdDevRatioRatioSD
CompresscompareOptimalC:\pe(...)t.pdf [93]3,729.7 μs715.06 μs39.19 μs1.030.03
CompressmainOptimalC:\pe(...)t.pdf [93]3,606.6 μs1,178.33 μs64.59 μs1.000.00
DecompresscompareOptimalC:\pe(...)t.pdf [93]404.5 μs41.28 μs2.26 μs0.950.03
DecompressmainOptimalC:\pe(...)t.pdf [93]427.2 μs264.41 μs14.49 μs1.000.00
CompresscompareOptimalC:\pe(...)9.txt [88]5,210.0 μs1,589.48 μs87.12 μs0.960.11
CompressmainOptimalC:\pe(...)9.txt [88]5,504.3 μs12,272.10 μs672.68 μs1.000.00
DecompresscompareOptimalC:\pe(...)9.txt [88]521.7 μs280.13 μs15.35 μs1.010.03
DecompressmainOptimalC:\pe(...)9.txt [88]517.0 μs201.16 μs11.03 μs1.000.00
CompresscompareOptimalC:\pe(...)a\sum [80]777.8 μs466.43 μs25.57 μs1.010.02
CompressmainOptimalC:\pe(...)a\sum [80]772.3 μs413.65 μs22.67 μs1.000.00
DecompresscompareOptimalC:\pe(...)a\sum [80]157.2 μs98.73 μs5.41 μs1.000.03
DecompressmainOptimalC:\pe(...)a\sum [80]157.4 μs66.97 μs3.67 μs1.000.00
CompresscompareFastestC:\pe(...)t.pdf [93]2,658.7 μs1,499.52 μs82.19 μs1.030.04
CompressmainFastestC:\pe(...)t.pdf [93]2,579.9 μs2,456.89 μs134.67 μs1.000.00
DecompresscompareFastestC:\pe(...)t.pdf [93]382.7 μs117.18 μs6.42 μs0.960.04
DecompressmainFastestC:\pe(...)t.pdf [93]398.6 μs173.96 μs9.54 μs1.000.00
CompresscompareFastestC:\pe(...)9.txt [88]1,697.4 μs420.08 μs23.03 μs1.000.01
CompressmainFastestC:\pe(...)9.txt [88]1,701.4 μs215.07 μs11.79 μs1.000.00
DecompresscompareFastestC:\pe(...)9.txt [88]591.7 μs346.91 μs19.02 μs1.040.04
DecompressmainFastestC:\pe(...)9.txt [88]568.1 μs233.40 μs12.79 μs1.000.00
CompresscompareFastestC:\pe(...)a\sum [80]347.1 μs166.74 μs9.14 μs0.980.08
CompressmainFastestC:\pe(...)a\sum [80]355.5 μs457.27 μs25.06 μs1.000.00
DecompresscompareFastestC:\pe(...)a\sum [80]151.5 μs133.82 μs7.34 μs0.970.05
DecompressmainFastestC:\pe(...)a\sum [80]155.9 μs121.91 μs6.68 μs1.000.00
Author:GrabYourPitchforks
Assignees:-
Labels:

area-System.IO.Compression

Milestone:-

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think we can have tests that verify this new code?

Comment threadsrc/native/external/zlib-intel.cmake Outdated
Comment threadsrc/native/external/zlib/dotnet_allocator_win.c Outdated
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

Do you think we can have tests that verify this new code?

@carlossanlop I'm open to suggestions as to how to test this! The most straightforward way that occurs to me is to have a callback mechanism, but now we're leaking abstractions through the DLL's exports and inserting complexity. :(

@danmoseley

Copy link
Copy Markdown
Contributor

do you plan to offer these to zlib upstream?

@GrabYourPitchforks

GrabYourPitchforks commented Apr 12, 2023

Copy link
Copy Markdown
MemberAuthor

do you plan to offer these to zlib upstream?

No. Per our other conversation, I don't think it's necessary.

@GrabYourPitchforks

GrabYourPitchforks commented Apr 13, 2023

Copy link
Copy Markdown
MemberAuthor

I also reran the aspnet benchmarks after ripping out the LFH code and didn't observe any difference before + after the change.

Comment threadsrc/native/external/zlib/dotnet_allocator_unix.c Outdated
Comment threadsrc/native/external/zlib/dotnet_allocator_unix.c
Comment threadsrc/native/external/zlib/dotnet_allocator_unix.c
- Move allocator files under Compression.Native dir
- Update pal_zlib includes, use calloc instead of malloc
- Remove custom typedefs from zlib unix allocator
@GrabYourPitchforks

GrabYourPitchforks commented Apr 14, 2023

Copy link
Copy Markdown
MemberAuthor

High level notes from the latest iteration:

  • Moved the custom allocators out of zlib and zlib-intel and into the Compression.Native folder, where the PAL already exists.
  • Removed custom typedefs, as requested.
  • Added a flag so that we know when we're using zlib vs zlib-intel, which allows both the allocator and the PAL to pull the correct header. Previously it was always pulling the normal (non-intel) zlib header, which seems to have worked, but probably was not intended behavior.

I smoke tested the Unix version on my Ubuntu box by making a change to cmakelists.txt and forcing us to use the internal version, even though the distro already had a shared zlib library.

New test run since the other one went stale: https://dev.azure.com/dnceng-public/public/_build/results?buildId=246796

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the change. And thanks for all the comments, they were quite helpful.

@GrabYourPitchforks
GrabYourPitchforks merged commit a41fd14 into dotnet:mainApr 21, 2023
@GrabYourPitchforks
GrabYourPitchforks deleted the zlib/main branch April 21, 2023 20:56
@ghostghost locked as resolved and limited conversation to collaborators May 22, 2023
@GrabYourPitchforks

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-staging

@github-actionsgithub-actionsBot unlocked this conversation Jul 26, 2023
@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5674150369

@github-actionsgithub-actionsBot locked as resolved and limited conversation to collaborators Jul 26, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@GrabYourPitchforks@danmoseley@carlossanlop@jkotas