Reject path-traversal entries in binlog embedded files by jankratochvilcz · Pull Request #14208 · dotnet/msbuild

jankratochvilcz · 2026-06-29T10:57:48Z

No description provided.

Guard against zip-slip when reading binlog embedded project imports: reject any archive entry whose path contains a '..' traversal segment. Legitimate binlogs store rooted, colon-stripped paths and never contain '..', so this fails fast on maliciously crafted logs without false positives. Adds unit tests for the IsPathTraversal helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR hardens MSBuild binary log replay by rejecting embedded archive entries that contain directory-traversal (..) path segments, preventing “zip-slip” style attacks when consuming embedded files from a potentially malicious .binlog.

Changes:

Add a path traversal guard when reading embedded archive entries in BuildEventArgsReader.
Add a new localized resource string for the rejection error message.
Add unit tests for the traversal detection helper.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Build/Resources/xlf/Strings.zh-Hant.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.zh-Hans.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.tr.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.ru.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.pt-BR.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.pl.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.ko.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.ja.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.it.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.fr.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.es.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.de.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/xlf/Strings.cs.xlf	Adds localized resource entry for the new path-traversal rejection message.
src/Build/Resources/Strings.resx	Adds the new `Binlog_PathTraversalInEmbeddedFile` resource string.
src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs	Rejects embedded archive entries containing traversal segments; adds `IsPathTraversal` helper.
src/Build.UnitTests/BinaryLogger_PathTraversal_Tests.cs	Adds unit coverage for traversal detection logic.

github-actions · 2026-06-29T11:23:48Z

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

MSBuild Target Authoring Conventions — LGTM

This PR modifies BuildEventArgsReader.cs and Strings.resx (plus XLF translation files) only. No .targets, .props, or MSBuild target authoring is touched, so this dimension does not apply.

Generated by Expert Code Review (on open) for #14208 · 1.9K AIC · ⊞ 30.3K · ◷

github-actions · 2026-06-29T11:23:49Z

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Design-level notes

Missing issue reference / PR description — For a security fix addressing a zip-slip attack vector, the PR has no description, no linked issue, and no reference to a CVE or GHSA. This makes it impossible to assess whether backporting to servicing branches is needed and prevents coordinating a security advisory if one is warranted. Please link or open a tracking issue.

EmbeddedContentRead branch is unguarded (internal-only gap) — ReadEmbeddedContent has a second code path (else if (EmbeddedContentRead != null) at ~line 457) that passes the raw ZIP stream without calling ThrowIfPathTraversal. This is currently safe because EmbeddedContentRead is internal and both its known subscribers (BinaryLogger blob copy, ProjectImportsCollector flush) never iterate ZIP entries by path. However, there is no code comment documenting this invariant. A future internal subscriber that does per-entry extraction would silently inherit the vulnerability. Recommend adding a brief comment at the else if branch, e.g.:

// EmbeddedContentRead receives raw ZIP bytes; subscribers must not
// extract entries by FullName without validating against IsPathTraversal.

Generated by Expert Code Review (on open) for #14208 · 1.9K AIC · ⊞ 30.3K · ◷

github-actions

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Error Message Quality

Error Message Quality — ISSUE

SEVERITY: MODERATE
FILE: src/Build/Resources/Strings.resx
LINES: Binlog_PathTraversalInEmbeddedFile entry
SCENARIO: A user or tooling encounters this exception — e.g. a build system parses
          MSBuild diagnostic output expecting MSBxxxx codes to look up documentation,
          file issues, or configure error suppression. This message has no code,
          while the adjacent binlog messages (MSB4279, MSB4280, MSB4281) all do.
          Additionally, the call site uses FormatResourceStringStripCodeAndKeyword,
          which is the right helper for resources *with* a code — using it on a
          codeless string is a harmless mismatch but signals the code was intended
          to follow the code convention.
FINDING: The resource string is missing an MSBxxxx error code. This breaks
         consistency with peer binlog errors, prevents code-based documentation
         lookup, and makes the error untargetable by tooling (e.g. /WarnAsError:MSBxxxx,
         suppression lists, telemetry grouping by code).
RECOMMENDATION: Assign the next available MSB4xxx code (check the bottom of
                Strings.resx for the next-available comment), prefix the value string
                with it, e.g.:
                  <value>MSB4282: Embedded file path "{0}" contains a directory
                  traversal segment and was rejected as potentially malicious.</value>
                No change needed at the call site — FormatResourceStringStripCodeAndKeyword
                will then correctly strip and return the code alongside the message.

The message text itself is clear and actionable (states what happened and why). The only gap is the missing error code per the repository convention.

Generated by Expert Code Review (on open) for #14208 · 1.9K AIC · ⊞ 30.3K

Replace string.Split (which allocates a string[] plus a string per segment on every archive entry) with a ReadOnlySpan<char> scan that detects ".." segments without allocations on the binlog replay path. Also drops the redundant null-forgiving operator now that the scan no longer dereferences the string directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The user-facing exception for a rejected path-traversal entry was the only binlog-reader error string without an MSBxxxx code. Prefix it with MSB4282 (next available code) and mark the localized {StrBegin}. The call site already uses FormatResourceStringStripCodeAndKeyword, which strips the code automatically, so no call-site change is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

"Rooted paths with the drive colon stripped" is Windows-centric; on Linux/macOS CalculateArchivePath performs no colon removal. Broaden both the inline comment and the XML doc to describe the normalization in a platform-neutral way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

For an Is* predicate, action-verb result segments ("Rejects"/"Allows") are ambiguous about whether they describe the return value or a thrown exception. Use the strict MethodUnderTest_Scenario_ExpectedResult form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The existing tests validate IsPathTraversal in isolation; none exercised the actual guard in the embedded-content read loop. Add a test that crafts a binlog stream whose embedded archive contains a "../../escape.txt" entry, replays it through BuildEventArgsReader with ArchiveFileEncountered subscribed, and asserts an InvalidDataException is thrown before the entry is surfaced. This catches accidental removal of the security check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings June 29, 2026 10:57

jankratochvilcz temporarily deployed to copilot-pat-pool June 29, 2026 10:57 — with GitHub Actions Inactive

jankratochvilcz temporarily deployed to copilot-pat-pool June 29, 2026 10:58 — with GitHub Actions Inactive

Copilot started reviewing on behalf of jankratochvilcz June 29, 2026 10:58 View session

jankratochvilcz requested a review from rainersigwald June 29, 2026 10:58

jankratochvilcz temporarily deployed to copilot-pat-pool June 29, 2026 10:59 — with GitHub Actions Inactive

Copilot AI reviewed Jun 29, 2026

View reviewed changes

Comment thread src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs

jankratochvilcz temporarily deployed to copilot-pat-pool June 29, 2026 11:19 — with GitHub Actions Inactive

jankratochvilcz temporarily deployed to copilot-pat-pool June 29, 2026 11:23 — with GitHub Actions Inactive

github-actions Bot reviewed Jun 29, 2026

View reviewed changes

jankratochvilcz temporarily deployed to copilot-pat-pool June 29, 2026 11:23 — with GitHub Actions Inactive

rainersigwald approved these changes Jun 29, 2026

View reviewed changes

jankratochvilcz and others added 5 commits June 30, 2026 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reject path-traversal entries in binlog embedded files#14208

Reject path-traversal entries in binlog embedded files#14208
jankratochvilcz wants to merge 6 commits into
dotnet:mainfrom
jankratochvilcz:binlog-zipslip-guard

jankratochvilcz commented Jun 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jankratochvilcz commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Design-level notes

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Error Message Quality

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jankratochvilcz commented Jun 29, 2026 •

edited

Loading