Update SharpCompress to 0.49.1 and re-arm its vulnerability alerting (SF-1864) - #2086
Closed
NickJosevski wants to merge 1 commit into
Closed
Update SharpCompress to 0.49.1 and re-arm its vulnerability alerting (SF-1864)#2086NickJosevski wants to merge 1 commit into
NickJosevski wants to merge 1 commit into
Conversation
…(SF-1864) Alternative to #2085, which takes the same change to 0.48.0. Same adaptations (WriterFactory.Open -> WriterFactory.OpenWriter, ExtractionOptions) and the same removal of the five <NoWarn>NU1902</NoWarn> elements, but landing on 0.49.1 instead. Why 0.49.1 rather than 0.48.0: 0.48.0 was never a considered floor. When the CVE surfaced on 11 May, 0.48.0 (published 6 May) was simply the newest release and the first version outside the advisory's <= 0.47.4 range, so Renovate raised it as the security bump and that number got baked into the ticket, the branch name and the original WIP commit. 0.48.1 through 0.50.1 were all published afterwards. 0.49.1 is where Octopus Server independently landed. Why not further: 0.50.0 changes Tar so it no longer attempts to decompress streams, and alters the detection API. Calamari's TarGzipPackageExtractor and TarBzipPackageExtractor are built on layered decompression, so that is a materially riskier step for no additional security benefit - nothing security-related shipped after 0.48.0. 1.0.0 is unlisted (NuGet reports its publish date as the 1900 sentinel) and is not a candidate. The one behaviour change in range that touches Calamari is 0.49.0 flipping WriterOptions.LeaveStreamOpen from true to false, so a writer now closes the stream it was given. Reviewed per call site: - GitHubPackageDownloader.DeNestContents - writer and stream are nested usings, so the stream is disposed twice; Stream.Dispose is idempotent, and nothing touches the stream after the writer is disposed. - UploadAwsS3Convention - returns targetArchivePath and never reads the stream after SaveTo, and the data is flushed to disk under either default. - ZipPackageProvider / NugetPackageProvider - use the path overload of SaveTo, so no stream ownership is involved. - TarGzBuilder (tests) - already set LeaveStreamOpen = false explicitly, so it is unaffected by the change in default. Verified: all affected projects build with 0 errors, NuGet's audit raises no advisory for 0.49.1, and 83 tests pass across PackageExtractorFixture and PackageExtractorUtilsFixture. Not covered by executed tests: GitHubPackageDownloader's own fixtures need GitHub credentials (they fail with 403 locally), and the Kubernetes EKS fixture that calls SaveTo is a live test. Both were assessed by reading the code rather than by running it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves SF-1864.
Important
Alternative to #2085 — same change, landing on 0.49.1 instead of 0.48.0. Pick one; close the other. Both are drafts.
Warning
Draft: not ready to merge. Green CI is necessary but not sufficient here — see "How to review".
Why 0.49.1 rather than 0.48.0
0.48.0 was never a considered floor. When the CVE surfaced on 11 May, 0.48.0 (published 6 May) was simply the newest release and the first version outside the advisory's
<= 0.47.4range. Renovate raised it as the[SECURITY]bump, and that number got baked into the ticket title, the branch name and the original WIP commit. Everything above it shipped later:0.49.1 is also where Octopus Server independently landed (
Octopus.Core.csproj).Why not 0.50.x
0.50.0 changes Tar so it no longer attempts to decompress streams, and alters the detection API.
TarGzipPackageExtractorandTarBzipPackageExtractorare built on layered decompression, so that is a materially riskier step — for no security benefit, since nothing security-related shipped after 0.48.0.The one behaviour change in range, reviewed per call site
0.49.0 flipped
WriterOptions.LeaveStreamOpenfromtruetofalse, so a writer now closes the stream it was handed. Every affected site:GitHubPackageDownloader.DeNestContentsusings → stream disposed twice.Stream.Disposeis idempotent, and nothing touches the stream after the writer. Safe.UploadAwsS3ConventiontargetArchivePath; never reads the stream afterSaveTo. Data is flushed to disk under either default. Safe.ZipPackageProvider/NugetPackageProviderSaveTo— no stream ownership involved.TarGzBuilder(tests)LeaveStreamOpen = falseexplicitly, so unaffected by the default change.Also in this PR: re-arming the alerting
All five references carried
<NoWarn>NU1902</NoWarn>, added to unblock builds when the CVE was published. Left in place after an upgrade they would silently hide the next SharpCompress advisory, so this PR removes all five. With them gone the projects build clean and NuGet's audit raises no advisory for 0.49.1.Were we actually vulnerable? Probably not
IArchive.WriteToDirectory(). Calamari iterates entries itself and calls the per-entry APIs — the latter named in the advisory as already guarded.ThrowIfPathTraversalAttemptedindependently resolves and bounds-checks every entry key, directory entries included, before anything is written.PackageExtractionOptionssupplies aWriteSymbolicLinkhandler that refuses to create links — not, as previously discussed, because Calamari supplies no handler. That distinction is load-bearing: implementing real symlink support there would reopen the arbitrary-file-write primitive.Automated tests
ExtractBlocksZipSlipViaDirectoryEntrycrafts a zip containing a traversing directory entry (System.IO.Compressioninjects it, since normal writers won't emit one) and asserts the extractor throws and nothing lands outside the root. It passes on 0.37.2 too — so it evidences that Calamari's own guard blocks this vector, and deliberately does not claim to verify the library's fix, which was never confirmed upstream.Verified locally: all affected projects build with 0 errors; 83 tests pass across
PackageExtractorFixtureandPackageExtractorUtilsFixture.Not covered by executed tests:
GitHubPackageDownloader's own fixtures need GitHub credentials (403 locally), and the Kubernetes EKS fixture that callsSaveTois a live test. Both were assessed by reading the code, not by running it.How to review this PR
Do not merge on green CI alone. #1913 passed Calamari's full suite, merged, then broke Server unpacking a consolidated package (
An item with the same key has already been addedinExtractCalamariPackage). This is the one area of the repo where the suite is a known-inadequate gate.Before this leaves draft:
build-local.sh+OCTOPUS__Dev__CalamariPackagePath), then run a health check / deployment that unpacks a consolidated package.LeaveStreamOpenchange actually lands and local tests skip it.ConsolidateCalamariPackagesno longer references SharpCompress at all, so the old signature coupling that blocked Server 2025.4 does not apply.Reducing risk
Out of scope, still open from the original discussion: extraction perf (~6s observed vs ~500ms expected, no benchmark exists), and whether this is backported to LTS branches.