diff --git a/Microsoft.Crank.sln b/Microsoft.Crank.sln index f7cdd3043..33e1d4114 100644 --- a/Microsoft.Crank.sln +++ b/Microsoft.Crank.sln @@ -55,6 +55,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Crank.JobObjectWr EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Crank.Jobs.K6", "src\Microsoft.Crank.Jobs.K6\Microsoft.Crank.Jobs.K6.csproj", "{D8DD4222-6929-46F3-A3F2-F38394AA1C72}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.Crank.UnitTests", "test\Microsoft.Crank.UnitTests\Microsoft.Crank.UnitTests.csproj", "{E735E8D7-2CF4-4C89-8D47-21E8B6AA26E6}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -129,6 +131,10 @@ Global {D8DD4222-6929-46F3-A3F2-F38394AA1C72}.Debug|Any CPU.Build.0 = Debug|Any CPU {D8DD4222-6929-46F3-A3F2-F38394AA1C72}.Release|Any CPU.ActiveCfg = Release|Any CPU {D8DD4222-6929-46F3-A3F2-F38394AA1C72}.Release|Any CPU.Build.0 = Release|Any CPU + {E735E8D7-2CF4-4C89-8D47-21E8B6AA26E6}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {E735E8D7-2CF4-4C89-8D47-21E8B6AA26E6}.Debug|Any CPU.Build.0 = Debug|Any CPU + {E735E8D7-2CF4-4C89-8D47-21E8B6AA26E6}.Release|Any CPU.ActiveCfg = Release|Any CPU + {E735E8D7-2CF4-4C89-8D47-21E8B6AA26E6}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -151,6 +157,7 @@ Global {A2B9140B-46E5-451D-9246-ECA019487F5C} = {995FCFF9-E5F6-4BDD-8E5B-FBDEA21145F9} {D02CC5A5-A6EA-42CF-9EA5-E3D1CE0FFBFE} = {995FCFF9-E5F6-4BDD-8E5B-FBDEA21145F9} {D8DD4222-6929-46F3-A3F2-F38394AA1C72} = {995FCFF9-E5F6-4BDD-8E5B-FBDEA21145F9} + {E735E8D7-2CF4-4C89-8D47-21E8B6AA26E6} = {07A30A34-2DDA-45EE-B767-28021086B235} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {C48AD7EE-82B1-4307-A869-3FC14AC9B21F} diff --git a/src/Microsoft.Crank.Agent/Controllers/JobsController.cs b/src/Microsoft.Crank.Agent/Controllers/JobsController.cs index 0afe2e13f..d11752ddf 100644 --- a/src/Microsoft.Crank.Agent/Controllers/JobsController.cs +++ b/src/Microsoft.Crank.Agent/Controllers/JobsController.cs @@ -665,12 +665,21 @@ public async Task Download(int id, string path) try { var job = _jobs.Find(id); - if (job == null) { return NotFound(); } + if (string.IsNullOrWhiteSpace(path)) + { + return BadRequest("Path parameter is required."); + } + if (PathValidator.ContainsDangerousCharacters(path)) + { + Log.Error($"Path contains dangerous characters: '{path}'"); + return BadRequest("Path contains invalid characters."); + } + // Downloads can't get out of this path var rootPath = Directory.GetParent(job.BasePath).FullName; @@ -685,9 +694,7 @@ public async Task Download(int id, string path) // Resolve dot notation in path var fullPath = Path.GetFullPath(path, job.BasePath); - Log.Info($"Download requested: '{path}'"); - if (!fullPath.StartsWith(rootPath, StringComparison.OrdinalIgnoreCase)) { Log.Error($"Client is not allowed to download '{fullPath}'"); @@ -801,7 +808,25 @@ public async Task Invoke(int id, string path) try { var job = _jobs.Find(id); - var response = await _httpClient.GetStringAsync(new Uri(new Uri(job.Url), path)); + + if (!Uri.TryCreate(job.Url, UriKind.Absolute, out var baseUri)) + { + Log.Error($"Invalid job URL: '{job.Url}'"); + return BadRequest("Invalid job configuration."); + } + var combinedUri = new Uri(baseUri, path); + + // Ensure the resulting URL is still under the same host/port as the base + var combinedUriComponents = combinedUri.GetComponents(UriComponents.SchemeAndServer | UriComponents.StrongAuthority, UriFormat.Unescaped); + var baseUriComponents = baseUri.GetComponents(UriComponents.SchemeAndServer | UriComponents.StrongAuthority, UriFormat.Unescaped); + if (!combinedUriComponents.Equals(baseUriComponents, StringComparison.InvariantCulture)) + { + Log.Error($"Trying to access different address. Base: {baseUri.Host}:{baseUri.Port}, Target: {combinedUri.Host}:{combinedUri.Port}"); + return BadRequest("Cannot invoke requests to different hosts."); + } + + Log.Info($"Invoking: {combinedUri}"); + var response = await _httpClient.GetStringAsync(combinedUri); return Content(response); } catch (Exception e) @@ -1028,5 +1053,19 @@ private IActionResult ObjectOrNotFoundResult(object obj) return new ObjectResult(obj); } + + private class PathValidator + { + private static readonly char[] _dangerousChars + = { ';', '|', '&', '$', '`', '\n', '\r', '<', '>', '"', '\'' }; + + /// + /// Returns true if detects shell metacharacters and control characters. False otherwise + /// + public static bool ContainsDangerousCharacters(string path) + { + return path.IndexOfAny(_dangerousChars) >= 0; + } + } } } diff --git a/test/Microsoft.Crank.UnitTests/JobsControllerTests.cs b/test/Microsoft.Crank.UnitTests/JobsControllerTests.cs new file mode 100644 index 000000000..ffb4c3e66 --- /dev/null +++ b/test/Microsoft.Crank.UnitTests/JobsControllerTests.cs @@ -0,0 +1,289 @@ +using System.Collections.Generic; +using System.IO; +using System.Runtime.InteropServices; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Crank.Agent.Controllers; +using Microsoft.Crank.Models; +using Repository; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.Crank.UnitTests +{ + public class JobsControllerTests + { + private readonly ITestOutputHelper _output; + + public JobsControllerTests(ITestOutputHelper output) + { + _output = output; + } + + [Theory] + [InlineData("$&\n\r.\\execute")] + [InlineData("../../../../../../etc/passwd")] // Unix-style traversal + [InlineData("../../../../../../../var/log/syslog")] // Absolute path traversal + [InlineData("foo/../../../etc/passwd")] // Mixed valid and traversal + [InlineData("/etc/passwd")] // Absolute Unix path + [InlineData("./../../../../../../etc/shadow")] // Dot slash mixed + [InlineData("test/../../../../../../etc/hosts")] // Valid then traversal + public async Task Download_PathTraversalAttempts_ReturnsBadRequest(string path) + { + // Use absolute path for testing + var tempDir = Path.GetTempPath(); + var jobDir = Path.Combine(tempDir, "crank_test_jobs", "1"); + Directory.CreateDirectory(jobDir); + + try + { + var jobRepo = new JobsRepository(); + jobRepo.Add(new() + { + Id = 1, + State = JobState.Running, + BasePath = jobDir + }); + + var jobsController = new JobsController(jobRepo); + + var result = await jobsController.Download(1, path); + + // Log diagnostic information + _output.WriteLine($"Testing path: {path}"); + _output.WriteLine($"BasePath: {jobDir}"); + _output.WriteLine($"Result type: {result.GetType().Name}"); + + Assert.IsType(result); + } + finally + { + // Cleanup + try + { + Directory.Delete(Path.Combine(tempDir, "crank_test_jobs"), true); + } + catch + { + // Ignore cleanup errors + } + } + } + + [Theory] + [InlineData("..\\..\\..\\Windows\\System32\\config\\SAM")] // Windows system files + [InlineData("..\\\\..\\\\..\\\\sensitive.txt")] // Double backslash + [InlineData("\\\\network\\share\\file.txt")] // UNC path + [InlineData("..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\file.txt")] // Excessive traversal + public async Task Download_PathTraversalAttempts_Windows_ReturnsBadRequest(string path) + { + // Skip this test on non-Windows platforms since backslash paths are only relevant on Windows + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + _output.WriteLine("Skipping Windows-specific path test on non-Windows platform"); + return; + } + + // Use absolute path for testing + var tempDir = Path.GetTempPath(); + var jobDir = Path.Combine(tempDir, "crank_test_jobs", "1"); + Directory.CreateDirectory(jobDir); + + try + { + var jobRepo = new JobsRepository(); + jobRepo.Add(new() + { + Id = 1, + State = JobState.Running, + BasePath = jobDir + }); + + var jobsController = new JobsController(jobRepo); + + var result = await jobsController.Download(1, path); + + // Log diagnostic information + _output.WriteLine($"Testing path: {path}"); + _output.WriteLine($"BasePath: {jobDir}"); + _output.WriteLine($"Result type: {result.GetType().Name}"); + + Assert.IsType(result); + } + finally + { + // Cleanup + try + { + Directory.Delete(Path.Combine(tempDir, "crank_test_jobs"), true); + } + catch + { + // Ignore cleanup errors + } + } + } + + [Fact] + public async Task Download_DiagnosticTest_ShowsPathResolution() + { + var tempDir = Path.GetTempPath(); + var jobDir = Path.Combine(tempDir, "crank_test_jobs", "1"); + Directory.CreateDirectory(jobDir); + + try + { + var path = "..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\file.txt"; + var rootPath = Directory.GetParent(jobDir).FullName; + var fullPath = Path.GetFullPath(path, jobDir); + + _output.WriteLine($"Input path: {path}"); + _output.WriteLine($"BasePath: {jobDir}"); + _output.WriteLine($"Root path: {rootPath}"); + _output.WriteLine($"Resolved full path: {fullPath}"); + _output.WriteLine($"FullPath starts with RootPath: {fullPath.StartsWith(rootPath, System.StringComparison.OrdinalIgnoreCase)}"); + + var jobRepo = new JobsRepository(); + jobRepo.Add(new() + { + Id = 1, + State = JobState.Running, + BasePath = jobDir + }); + + var jobsController = new JobsController(jobRepo); + var result = await jobsController.Download(1, path); + + _output.WriteLine($"Result type: {result.GetType().Name}"); + if (result is BadRequestObjectResult badRequest) + { + _output.WriteLine($"Error message: {badRequest.Value}"); + } + } + finally + { + try + { + Directory.Delete(Path.Combine(tempDir, "crank_test_jobs"), true); + } + catch + { + // Ignore cleanup errors + } + } + } + + [Theory] + [InlineData("http://localhost:5000/api", "http://evil.com/steal")] // Different domain + [InlineData("http://localhost:5000", "http://localhost:8080/api")] // Different port + [InlineData("http://localhost:5000", "https://localhost:5000/api")] // Different scheme + [InlineData("http://example.com", "http://attacker.com")] // Completely different host + [InlineData("http://api.example.com", "http://evil.example.com")] // Subdomain manipulation + [InlineData("http://localhost:5000", "//evil.com/steal")] // Protocol-relative URL + [InlineData("http://localhost:5000", "http://127.0.0.1:5000/api")] // IP vs hostname + public async Task Invoke_DifferentHostRequests_ReturnsBadRequest(string jobUrl, string path) + { + var jobRepo = new JobsRepository(); + jobRepo.Add(new() + { + Id = 1, + State = JobState.Running, + BasePath = Path.GetTempPath(), + Url = jobUrl + }); + + var jobsController = new JobsController(jobRepo); + var result = await jobsController.Invoke(1, path); + + _output.WriteLine($"Job URL: {jobUrl}"); + _output.WriteLine($"Request path: {path}"); + _output.WriteLine($"Result type: {result.GetType().Name}"); + + var badRequestResult = Assert.IsType(result); + } + + [Fact] + public async Task Invoke_InvalidJobUrl_ReturnsBadRequest() + { + var jobRepo = new JobsRepository(); + jobRepo.Add(new() + { + Id = 1, + State = JobState.Running, + BasePath = Path.GetTempPath(), + Url = "not-a-valid-url" // Invalid URL + }); + + var jobsController = new JobsController(jobRepo); + var result = await jobsController.Invoke(1, "/api/test"); + + _output.WriteLine($"Result type: {result.GetType().Name}"); + + var badRequestResult = Assert.IsType(result); + Assert.Equal("Invalid job configuration.", badRequestResult.Value); + } + + [Fact] + public async Task Invoke_NonExistentJob_ReturnsStatusCode500() + { + var jobRepo = new JobsRepository(); + var jobsController = new JobsController(jobRepo); + + var result = await jobsController.Invoke(999, "/api/test"); + + _output.WriteLine($"Result type: {result.GetType().Name}"); + + // When job is null, job.Url will throw NullReferenceException, caught by the catch block + Assert.IsType(result); + var objectResult = result as ObjectResult; + Assert.Equal(500, objectResult.StatusCode); + } + + [Theory] + [InlineData("http://localhost:5000", "http://localhost@evil.com/steal")] // Username in URL trick + [InlineData("http://localhost:5000", "http://evil.com#localhost:5000")] // Fragment manipulation + public async Task Invoke_AdvancedSSRFAttempts_ReturnsBadRequest(string jobUrl, string path) + { + var jobRepo = new JobsRepository(); + jobRepo.Add(new() + { + Id = 1, + State = JobState.Running, + BasePath = Path.GetTempPath(), + Url = jobUrl + }); + + var jobsController = new JobsController(jobRepo); + var result = await jobsController.Invoke(1, path); + + _output.WriteLine($"Job URL: {jobUrl}"); + _output.WriteLine($"Request path: {path}"); + _output.WriteLine($"Result type: {result.GetType().Name}"); + + Assert.IsType(result); + } + + private class JobsRepository : IJobRepository + { + private readonly Dictionary _jobs = new(); + + public Job Add(Job item) + { + _jobs.Add(item.Id, item); + return item; + } + + public Job Find(int id) + => _jobs.TryGetValue(id, out var job) ? job : null; + + public IEnumerable GetAll() + => _jobs.Values; + + public Job Remove(int id) + => _jobs.Remove(id, out var job) ? job : null; + + public void Update(Job item) + => _jobs[item.Id] = item; + } + } +} diff --git a/test/Microsoft.Crank.UnitTests/Microsoft.Crank.UnitTests.csproj b/test/Microsoft.Crank.UnitTests/Microsoft.Crank.UnitTests.csproj new file mode 100644 index 000000000..09157a256 --- /dev/null +++ b/test/Microsoft.Crank.UnitTests/Microsoft.Crank.UnitTests.csproj @@ -0,0 +1,24 @@ + + + + net8.0 + Microsoft.Crank.UnitTests + true + true + XUnit + false + + + + + + + + + + + + + + +