Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .azure-pipelines/build-job.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,24 @@ jobs:
compiled:
enabled: false
justificationForDisabling: ${{ parameters.justificationForDisablingSdl}}
# Guardian / BinSkim ship as x64 binaries today and cannot run on
# Linux ARM64 hosts ("cannot execute binary file: Exec format error").
# When SDL is being disabled for a build (e.g. Alpine/Linux ARM64),
# also turn off these compiled scanners and the PostAnalysis gate so
# the pipeline does not fail with Guardian exit code 126.
${{ if and(eq(parameters.arch, 'arm64'), ne(parameters.os, 'win'), ne(parameters.os, 'osx')) }}:
binskim:
enabled: false
policheck:
enabled: false
credscan:
enabled: false
psscriptanalyzer:
enabled: false
armory:
enabled: false
tsa:
enabled: false
variables:
PACKAGE_TYPE: ${{ parameters.packageType }}
${{ if eq(parameters.os, 'win') }}:
Expand All @@ -110,6 +128,9 @@ jobs:
DisableCFSDetector: true
DisableDockerDetector: true
nugetMultiFeedWarnLevel: none
# Clear GDN_FILEPATH on ARM64 Linux to prevent Guardian from using the pre-installed x64 binary
${{ if and(eq(parameters.arch, 'arm64'), ne(parameters.os, 'win'), ne(parameters.os, 'osx')) }}:
GDN_FILEPATH: ''
CheckoutBranch: ${{ parameters.branch }}
${{ if ne(parameters.targetFramework, 'all') }}:
targetFramework: ${{ parameters.targetFramework }}
Expand Down
8 changes: 6 additions & 2 deletions .azure-pipelines/pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ extends:
--token "$(ACCESS_TOKEN)" \
--templateParameters "{ \"branch\": \"${branch}\", \"target_framework\": \"$(targetFramework)\"}"
displayName: Test Proxy Agent
continueOnError: true

# Windows (x64)
- ${{ if parameters.win_x64 }}:
Expand Down Expand Up @@ -260,8 +261,9 @@ extends:
displayName: Linux (ARM64)
pool:
name: 1ES-ABTT-Shared-ARM-64-Pool
vmImage: abtt-azurelinux3_arm64
image: abtt-azurelinux3_arm64
os: linux
hostArchitecture: arm64
timeoutInMinutes: 75
os: linux
arch: arm64
Expand Down Expand Up @@ -298,12 +300,14 @@ extends:
- ${{ if parameters.alpine_arm64 }}:
- template: /.azure-pipelines/build-jobs.yml@self
parameters:
disableSdl: true
jobName: build_alpine_arm64
displayName: Alpine (ARM64)
pool:
name: 1ES-ABTT-Shared-ARM-64-Pool
vmImage: abtt-azurelinux3_arm64
image: abtt-azurelinux3_arm64
os: linux
hostArchitecture: arm64
# container: # arm64v8/alpine (N/A)
os: linux-musl
arch: arm64
Expand Down
7 changes: 6 additions & 1 deletion src/Agent.Worker/CodeCoverage/CodeCoverageCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ private async Task PublishCodeCoverageAsync(
if (_additionalCodeCoverageFiles != null && _additionalCodeCoverageFiles.Count != 0)
{
additionalCodeCoverageFilePath = GetCoverageDirectory(_buildId.ToString(), CodeCoverageConstants.RawFilesDirectory);
CodeCoverageUtilities.CopyFilesFromFileListWithDirStructure(_additionalCodeCoverageFiles, ref additionalCodeCoverageFilePath);
var skippedFiles = new List<string>();
CodeCoverageUtilities.CopyFilesFromFileListWithDirStructure(_additionalCodeCoverageFiles, ref additionalCodeCoverageFilePath, skippedFiles);
foreach (var skipped in skippedFiles)
{
executionContext.Warning(StringUtil.Loc("CodeCoverageFileSkippedPathTraversal", skipped));
Comment thread
tarunramsinghani marked this conversation as resolved.
}
filesToPublish.Add(new Tuple<string, string>(additionalCodeCoverageFilePath, GetCoverageDirectoryName(_buildId.ToString(), CodeCoverageConstants.RawFilesDirectory)));
}
commandContext.Output(StringUtil.Loc("PublishingCodeCoverageFiles"));
Expand Down
32 changes: 27 additions & 5 deletions src/Agent.Worker/CodeCoverage/CodeCoverageUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,20 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using System.Xml;
using System.Xml.Linq;

namespace Microsoft.VisualStudio.Services.Agent.Worker.CodeCoverage
{
public static class CodeCoverageUtilities
{
public static void CopyFilesFromFileListWithDirStructure(List<string> files, ref string destinatonFilePath)
private static readonly StringComparison PathComparison =
RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
? StringComparison.OrdinalIgnoreCase
: StringComparison.Ordinal;

public static void CopyFilesFromFileListWithDirStructure(List<string> files, ref string destinatonFilePath, List<string> skippedFiles = null)
{
Comment thread
sanjuyadav24 marked this conversation as resolved.
string commonPath = null;
if (files != null)
Expand All @@ -26,22 +32,38 @@ public static void CopyFilesFromFileListWithDirStructure(List<string> files, ref
commonPath = SharedSubstring(files[0], files[files.Count - 1]);
}

var canonicalDest = Path.GetFullPath(destinatonFilePath + Path.DirectorySeparatorChar);

foreach (var file in files)
{
string newFile = null;

if (!string.IsNullOrEmpty(commonPath))
// FIX 1: Use Substring instead of Replace to safely remove only the prefix
if (!string.IsNullOrEmpty(commonPath) && file.StartsWith(commonPath, PathComparison))
{
newFile = file.Replace(commonPath, "");
newFile = file.Substring(commonPath.Length);
}
else
{
newFile = Path.GetFileName(file);
}

// FIX 2: Strip leading directory separators to prevent Path.Combine
// from treating newFile as an absolute path and ignoring destinatonFilePath
newFile = newFile.TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);

newFile = Path.Combine(destinatonFilePath, newFile);
Directory.CreateDirectory(Path.GetDirectoryName(newFile));
File.Copy(file, newFile, true);
var canonicalNewFile = Path.GetFullPath(newFile);

// FIX 3: Canonicalization boundary check - skip files that resolve outside destination
if (!canonicalNewFile.StartsWith(canonicalDest, PathComparison))
{
skippedFiles?.Add(file);
continue;
}
Comment thread
sanjuyadav24 marked this conversation as resolved.

Directory.CreateDirectory(Path.GetDirectoryName(canonicalNewFile));
File.Copy(file, canonicalNewFile, true);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Misc/layoutbin/en-US/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"ClientSecret": "Client secret",
"ClockSkewStopRetry": "Stopped retrying OAuth token request exception after {0} seconds.",
"CodeCoverageDataIsNull": "No coverage data found. Check the build errors/warnings for more details.",
"CodeCoverageFileSkippedPathTraversal": "Skipping code coverage file due to path traversal attempt: {0}",
"CodeCoveragePublishIsValidOnlyForBuild": "Publishing code coverage works only for 'build'.",
"CollectionName": "Collection Name",
"CommandDuplicateDetected": "Command {0} already installed for area {1}",
Expand Down
122 changes: 122 additions & 0 deletions src/Test/L0/Worker/CodeCoverage/CodeCoverageUtilitiesTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,128 @@ public void ThrowsIfParameterIsWhiteSpace()
Assert.Throws<ArgumentException>(() => CodeCoverageUtilities.TrimNonEmptyParam(" ", "inputName"));
}

[Fact]
[Trait("Level", "L0")]
[Trait("Category", "PublishCodeCoverage")]
public void CopyFilesSkipsPathTraversalFiles()
{
string destinationFilePath = Path.Combine(Path.GetTempPath(), "cc_dest_traversal1");
// Nest srcDir 3 levels deep so ../../ traversal stays within temp directory
var srcDir = Path.Combine(Path.GetTempPath(), "cc_traversal_src", "level1", "level2");
var srcRoot = Path.Combine(Path.GetTempPath(), "cc_traversal_src");
try
{
Directory.CreateDirectory(destinationFilePath);
var legitimateFile = Path.Combine(srcDir, "legit.xml");
Directory.CreateDirectory(srcDir);
File.WriteAllText(legitimateFile, "Test");

// Craft a path that traverses above srcDir.
// srcDir/../../evil.xml resolves to cc_traversal_src/evil.xml (within temp)
var maliciousFile = Path.Combine(srcDir, "..", "..", "evil.xml");
var resolvedMalicious = Path.GetFullPath(maliciousFile);
File.WriteAllText(resolvedMalicious, "Malicious");

var files = new List<string> { legitimateFile, maliciousFile };
var skippedFiles = new List<string>();

CodeCoverageUtilities.CopyFilesFromFileListWithDirStructure(files, ref destinationFilePath, skippedFiles);

// Malicious file should be skipped, not copied
Assert.True(skippedFiles.Count > 0, "Expected at least one file to be skipped due to path traversal");
Assert.Contains(maliciousFile, skippedFiles);
}
finally
{
if (Directory.Exists(destinationFilePath)) Directory.Delete(destinationFilePath, true);
if (Directory.Exists(srcRoot)) Directory.Delete(srcRoot, true);
}
}

[Fact]
[Trait("Level", "L0")]
[Trait("Category", "PublishCodeCoverage")]
public void CopyFilesHandlesAbsolutePathInjection()
{
// Verifies that the Substring+TrimStart fix prevents the Replace trick attack.
// Attack: craft paths so SharedSubstring = "srcDir\pwn", then old Replace
// on "srcDir\pwn\<separator><path>" removes prefix, leaving "\<path>"
// which Path.Combine treats as absolute (ignoring destination).
// Fix: Substring removes only prefix, TrimStart strips leading separators.
string destinationFilePath = Path.Combine(Path.GetTempPath(), "cc_dest_abs");
var srcDir = Path.Combine(Path.GetTempPath(), "cc_abs_src");
try
{
Directory.CreateDirectory(destinationFilePath);

// Create two files under srcDir that share prefix "srcDir\pwn"
// File 1: srcDir\pwn\sub\evil.txt
// File 2: srcDir\pwnX
var subDir = Path.Combine(srcDir, "pwn", "sub");
Directory.CreateDirectory(subDir);
var payloadFile = Path.Combine(subDir, "evil.txt");
File.WriteAllText(payloadFile, "Malicious");

var dummyFile = Path.Combine(srcDir, "pwnX");
File.WriteAllText(dummyFile, "Dummy");

var files = new List<string> { payloadFile, dummyFile };
var skippedFiles = new List<string>();

CodeCoverageUtilities.CopyFilesFromFileListWithDirStructure(files, ref destinationFilePath, skippedFiles);

// With old Replace("srcDir\pwn",""), payloadFile becomes "\sub\evil.txt"
// Path.Combine(dest, "\sub\evil.txt") → "\sub\evil.txt" (absolute, escapes!)
// With fix: Substring → "\sub\evil.txt", TrimStart → "sub\evil.txt" (relative, safe)

// Verify: file must NOT exist at root \sub\evil.txt
var rootEscape = Path.Combine(Path.GetPathRoot(destinationFilePath), "sub", "evil.txt");
Assert.False(File.Exists(rootEscape),
"File must not escape to drive root via leading separator injection");

// Verify: file should land safely inside destination
Assert.True(File.Exists(Path.Combine(destinationFilePath, "sub", "evil.txt")),
"File should be safely nested inside destination directory");

Assert.True(skippedFiles.Count == 0, "Legitimate file should not be skipped");
}
finally
{
if (Directory.Exists(destinationFilePath)) Directory.Delete(destinationFilePath, true);
if (Directory.Exists(srcDir)) Directory.Delete(srcDir, true);
}
}

[Fact]
[Trait("Level", "L0")]
[Trait("Category", "PublishCodeCoverage")]
public void CopyFilesSucceedsWithLegitimateFiles()
{
string destinationFilePath = Path.Combine(Path.GetTempPath(), "cc_dest_legit");
var srcDir = Path.Combine(Path.GetTempPath(), "cc_legit_src");
try
{
Directory.CreateDirectory(destinationFilePath);
var file1 = Path.Combine(srcDir, "sub1", "a.xml");
var file2 = Path.Combine(srcDir, "sub2", "b.xml");
Directory.CreateDirectory(Path.GetDirectoryName(file1));
Directory.CreateDirectory(Path.GetDirectoryName(file2));
File.WriteAllText(file1, "Content1");
File.WriteAllText(file2, "Content2");

var files = new List<string> { file1, file2 };
CodeCoverageUtilities.CopyFilesFromFileListWithDirStructure(files, ref destinationFilePath);

Assert.True(File.Exists(Path.Combine(destinationFilePath, "1", "a.xml")));
Assert.True(File.Exists(Path.Combine(destinationFilePath, "2", "b.xml")));
}
finally
{
if (Directory.Exists(destinationFilePath)) Directory.Delete(destinationFilePath, true);
if (Directory.Exists(srcDir)) Directory.Delete(srcDir, true);
}
}

private void SetupMocks()
{
_ec = new Mock<IExecutionContext>();
Expand Down
Loading