Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
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));
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.

can we make this error but not fail the task...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this will trigger a warning, not fail the pipeline

}
filesToPublish.Add(new Tuple<string, string>(additionalCodeCoverageFilePath, GetCoverageDirectoryName(_buildId.ToString(), CodeCoverageConstants.RawFilesDirectory)));
}
commandContext.Output(StringUtil.Loc("PublishingCodeCoverageFiles"));
Expand Down
26 changes: 21 additions & 5 deletions src/Agent.Worker/CodeCoverage/CodeCoverageUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace Microsoft.VisualStudio.Services.Agent.Worker.CodeCoverage
{
public static class CodeCoverageUtilities
{
public static void CopyFilesFromFileListWithDirStructure(List<string> files, ref string destinatonFilePath)
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 +26,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, StringComparison.OrdinalIgnoreCase))
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.

ignore case is not needed for case-sensitive os IMO, we shuold figure out side effects of it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

{
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, StringComparison.OrdinalIgnoreCase))
Comment thread
sanjuyadav24 marked this conversation as resolved.
Outdated
{
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