Update SharpCompress to 0.49.1 and re-arm its vulnerability alerting (SF-1864) - #2087
Update SharpCompress to 0.49.1 and re-arm its vulnerability alerting (SF-1864)#2087NickJosevski wants to merge 1 commit into
Conversation
Verification against the shipped linux-x64 artifactRan the packaged Calamari — not Setup: published Every supported format extracts correctly
Driven via Controls, so the above isn't vacuousNegative control — a deliberately corrupt archive was correctly rejected: That exception type confirms the shipped 0.49.1 assembly is doing the work, not some fallback path. Positive control for the highest-risk API. The three decorators that call
So Correction to earlier reasoning in this threadI previously argued the packaged-artifact check mattered because Calamari ships trimmed, and trimming plus a library upgrade could silently remove a reflection-reachable path. That is wrong. Reading There is therefore no trim risk for this change. The check retained value for a different reason (it runs the real shipped assemblies on real Linux), but the original justification does not hold. Still not covered
Given the trimming concern evaporated, a full local Server run is now optional for this change rather than important. The genuinely uncovered path is the GitHub feed one. |
|
I've run this locally and tested the GitHub path. Works as expected!
Can you add some more context to this? |
| protected override Stream GetCompressionStream(Stream stream) | ||
| { | ||
| return new BZip2Stream(stream, CompressionMode.Decompress, false); | ||
| return BZip2Stream.Create(stream, CompressionMode.Decompress, false, false); |
There was a problem hiding this comment.
Nit: Use named arguments to avoid ambiguity:
| return BZip2Stream.Create(stream, CompressionMode.Decompress, false, false); | |
| return BZip2Stream.Create(stream, CompressionMode.Decompress, decompressConcatenated: false, leaveOpen: false); |
…(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>
4f909e5 to
f7f8af1
Compare
|
Thanks — that closes the gap I was most concerned about. One small correction on the ARM point, so the coverage claim stays accurate: macOS on Apple Silicon exercises an osx RID, whereas the ARM gap I meant is Applied your On "large or pathological archives"Fair question — that line was doing too much work. Two distinct things, neither addressed here: 1. Performance, which is a pre-existing open question. In the original thread Orion noted extracting from the ConsolidatedPackage took ~6s where ~500ms would be expected, and that there's no benchmark anywhere in the repo to measure against. That was never resolved. My fixtures use tiny archives — a couple of files, a few hundred bytes — so this PR neither improves nor regresses that, and provides no evidence either way. A 13-minor-version jump could plausibly change extraction throughput in either direction and we would not notice. 2. Adversarial inputs, where we have partial coverage. The archive-limit decorators ( What's untested in both categories: deeply nested paths, very long entry names, high entry counts, truncated or partially-corrupt archives beyond the single "not an archive at all" case I used, and unusual encodings beyond the existing UTF-8 fixtures. My take: not a blocker for this PR. The change is a version bump whose behavioural risk is concentrated in API surface, and that's covered — CI is green across every platform, all six formats extract correctly from the shipped linux-x64 binary, and the limit protections still work. Performance and fuzz-style robustness are properties of the extraction code that were unmeasured before this change and remain unmeasured after. They're worth a ticket though, and the perf one has a concrete starting point already: a benchmark for ConsolidatedPackage extraction would let us confirm or dismiss Orion's ~6s observation, and would then also tell us whether a future SharpCompress bump moves it. Happy to raise that separately if you agree it's worth tracking rather than leaving in a PR comment. |
|
I'll tidy up what claude posted later |
The bundled dotnet-script 1.6.0 is a framework-dependent application targeting
Microsoft.NETCore.App 8.0.0. A framework-dependent app does not roll forward
across a major version by default, so once the build agents were provisioned
with only .NET 10 it failed to launch:
App: .../Binaries/dotnet-script/dotnet-script.dll
Framework: 'Microsoft.NETCore.App', version '8.0.0' (x64)
The following frameworks were found:
10.0.10 at [.../shared/Microsoft.NETCore.App]
That is the cause of the DotnetScriptProxyFixture failures on the Linux
netcore-testing agents in PR #2091.
Calamari itself is published self-contained and carries no such requirement -
this affects only the separate dotnet-script process. Setting
DOTNET_ROLL_FORWARD=Major on that invocation lets it run on whatever newer
runtime is present. An explicit value already set in the environment is
respected rather than overwritten.
This is a customer-facing fix as much as a CI one: without it, a target with
.NET 10 but no .NET 8 runtime cannot run C# script steps at all. It should also
reduce the need to provision a specific runtime alongside Calamari on workers.
EnvironmentVars is applied additively by SilentProcessRunner
(process.StartInfo.EnvironmentVariables[key] = value), so passing a dictionary
where one was previously null does not discard the inherited environment.
Not done here: upgrading the bundled dotnet-script. 2.0.1 was evaluated and
rejected for now - the GitHub release zip that Calamari vendors is still
net8.0-targeted (only the NuGet package ships a net10.0 build), so it would not
have fixed this, and it changes the NuGet version dotnet-script bundles
(6.10.1.5 -> 6.14.3.1) which invalidates the premise of
UsingIsolatedAssemblyLoadContext. Worth doing as its own piece of work.
Verified: solution builds 171 warnings / 0 errors; all 10 DotnetScriptFixture
tests pass. The 6 DotnetScriptProxyFixture failures seen locally are
environmental - they need the proxy-initializer-fixture-good-proxy host, which
is NXDOMAIN off CI, and they pass on baseline PR #2087's Debian agent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bundled dotnet-script 1.6.0 is a framework-dependent application targeting
Microsoft.NETCore.App 8.0.0. A framework-dependent app does not roll forward
across a major version by default, so once the build agents were provisioned
with only .NET 10 it failed to launch:
App: .../Binaries/dotnet-script/dotnet-script.dll
Framework: 'Microsoft.NETCore.App', version '8.0.0' (x64)
The following frameworks were found:
10.0.10 at [.../shared/Microsoft.NETCore.App]
That is the cause of the DotnetScriptProxyFixture failures on the Linux
netcore-testing agents in PR #2091.
Calamari itself is published self-contained and carries no such requirement -
this affects only the separate dotnet-script process. Setting
DOTNET_ROLL_FORWARD=Major on that invocation lets it run on whatever newer
runtime is present. An explicit value already set in the environment is
respected rather than overwritten.
This is a customer-facing fix as much as a CI one: without it, a target with
.NET 10 but no .NET 8 runtime cannot run C# script steps at all. It should also
reduce the need to provision a specific runtime alongside Calamari on workers.
EnvironmentVars is applied additively by SilentProcessRunner
(process.StartInfo.EnvironmentVariables[key] = value), so passing a dictionary
where one was previously null does not discard the inherited environment.
Not done here: upgrading the bundled dotnet-script. 2.0.1 was evaluated and
rejected for now - the GitHub release zip that Calamari vendors is still
net8.0-targeted (only the NuGet package ships a net10.0 build), so it would not
have fixed this, and it changes the NuGet version dotnet-script bundles
(6.10.1.5 -> 6.14.3.1) which invalidates the premise of
UsingIsolatedAssemblyLoadContext. Worth doing as its own piece of work.
Verified: solution builds 171 warnings / 0 errors; all 10 DotnetScriptFixture
tests pass. The 6 DotnetScriptProxyFixture failures seen locally are
environmental - they need the proxy-initializer-fixture-good-proxy host, which
is NXDOMAIN off CI, and they pass on baseline PR #2087's Debian agent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Resolves SF-1864.
CVE advisory's says
<= 0.47.4range impacted.0.49.1 is where Octopus Server is at
0.49.0 flipped
WriterOptions.LeaveStreamOpenfromtruetofalse, so a writer now closes the stream it was handed.Remove
<NoWarn>NU1902</NoWarn>, so we get the warnings again and decide then if we want it back, or we fix right away.Was not vulnerable
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.Automated tests
ExtractBlocksZipSlipViaDirectoryEntrycrafts a zip containing a traversing directory entry 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.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.Verification done so far:
ArchiveFactory.OpenArchiveworks on 0.49.1. See the verification comment.Before this leaves draft:
GitHubPackageDownloader.DeNestContentsis the one shipping site the 0.49.0LeaveStreamOpenflip lands on, and it is the only genuinely uncovered path. Best done via a deployment sourced from a GitHub repository feed.