diff --git a/CHANGELOG.md b/CHANGELOG.md index e694a6b834..bcffaba032 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Fixed a path traversal vulnerability where workspace consumer identifiers (`clientName`/`pluginName`) and workspace configuration keys were used as filesystem path components without containment validation, allowing a crafted name (e.g. `junk/../Victim`) to overwrite another consumer's cached OpenAPI description. [#7919](https://github.com/microsoft/kiota/issues/7919) + ## [1.33.0] - 2026-07-06 ### Added diff --git a/src/Kiota.Builder/WorkspaceManagement/DescriptionStorageService.cs b/src/Kiota.Builder/WorkspaceManagement/DescriptionStorageService.cs index 1a8d415481..04ec0cf6a0 100644 --- a/src/Kiota.Builder/WorkspaceManagement/DescriptionStorageService.cs +++ b/src/Kiota.Builder/WorkspaceManagement/DescriptionStorageService.cs @@ -1,5 +1,6 @@ using System; using System.IO; +using System.Linq; using System.Threading; using System.Threading.Tasks; using AsyncKeyedLock; @@ -21,7 +22,39 @@ public DescriptionStorageService(string targetDirectory) o.PoolSize = 20; o.PoolInitialFill = 1; }); - private string GetDescriptionFilePath(string clientName, string extension) => Path.Combine(TargetDirectory, DescriptionsSubDirectoryRelativePath, clientName, $"openapi.{extension}"); + private string GetDescriptionFilePath(string clientName, string extension) + { + ValidateConsumerName(clientName); + ValidateExtension(extension); + var documentsDirectory = Path.Join(TargetDirectory, DescriptionsSubDirectoryRelativePath); + var descriptionFilePath = Path.GetFullPath(Path.Combine(documentsDirectory, clientName, $"openapi.{extension}")); + var documentsFullPath = Path.GetFullPath(documentsDirectory); + var documentsFullPathWithSeparator = Path.EndsInDirectorySeparator(documentsFullPath) ? documentsFullPath : documentsFullPath + Path.DirectorySeparatorChar; + var comparison = OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + if (!descriptionFilePath.StartsWith(documentsFullPathWithSeparator, comparison)) + throw new InvalidOperationException($"The consumer name '{clientName}' resolves to a path outside of the documents directory."); + return descriptionFilePath; + } + internal static void ValidateConsumerName(string clientName) + { + if (string.IsNullOrWhiteSpace(clientName)) + throw new InvalidOperationException("The consumer name must not be empty or whitespace."); + if (Path.IsPathRooted(clientName) || + clientName.Contains('/', StringComparison.Ordinal) || + clientName.Contains('\\', StringComparison.Ordinal) || + clientName.Split('/', '\\').Contains("..", StringComparer.Ordinal) || + clientName is "." or "..") + throw new InvalidOperationException($"The consumer name '{clientName}' is not a valid single path segment and cannot navigate the file system."); + } + private static void ValidateExtension(string extension) + { + if (string.IsNullOrWhiteSpace(extension)) + throw new InvalidOperationException("The description file extension must not be empty or whitespace."); + if (Path.IsPathRooted(extension) || + extension.Contains('/', StringComparison.Ordinal) || + extension.Contains('\\', StringComparison.Ordinal)) + throw new InvalidOperationException($"The description file extension '{extension}' must not contain path separators or be rooted."); + } public async Task UpdateDescriptionAsync(string clientName, Stream description, string extension = "yml", CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(clientName); @@ -62,7 +95,7 @@ public void RemoveDescription(string clientName, string extension = "yml") } public void Clean() { - var kiotaDirectoryPath = Path.Combine(TargetDirectory, DescriptionsSubDirectoryRelativePath); + var kiotaDirectoryPath = Path.Join(TargetDirectory, DescriptionsSubDirectoryRelativePath); if (Path.Exists(kiotaDirectoryPath)) Directory.Delete(kiotaDirectoryPath, true); } diff --git a/src/Kiota.Builder/WorkspaceManagement/WorkspaceConfigurationStorageService.cs b/src/Kiota.Builder/WorkspaceManagement/WorkspaceConfigurationStorageService.cs index 8cc9849309..e2718763e3 100644 --- a/src/Kiota.Builder/WorkspaceManagement/WorkspaceConfigurationStorageService.cs +++ b/src/Kiota.Builder/WorkspaceManagement/WorkspaceConfigurationStorageService.cs @@ -111,20 +111,22 @@ private void ValidateConsumerOutputPaths(WorkspaceConfiguration? configuration) foreach (var client in configuration.Clients) try { + DescriptionStorageService.ValidateConsumerName(client.Key); BaseApiConsumerConfiguration.ValidateOutputPath(client.Value.OutputPath, workspaceDirectory); } catch (InvalidOperationException ex) { - throw new InvalidOperationException($"The client {client.Key} has an invalid output path: {ex.Message}", ex); + throw new InvalidOperationException($"The client {client.Key} has an invalid configuration: {ex.Message}", ex); } foreach (var plugin in configuration.Plugins) try { + DescriptionStorageService.ValidateConsumerName(plugin.Key); BaseApiConsumerConfiguration.ValidateOutputPath(plugin.Value.OutputPath, workspaceDirectory); } catch (InvalidOperationException ex) { - throw new InvalidOperationException($"The plugin {plugin.Key} has an invalid output path: {ex.Message}", ex); + throw new InvalidOperationException($"The plugin {plugin.Key} has an invalid configuration: {ex.Message}", ex); } } public async Task BackupConfigAsync(CancellationToken cancellationToken = default) diff --git a/src/Kiota.Builder/WorkspaceManagement/WorkspaceManagementService.cs b/src/Kiota.Builder/WorkspaceManagement/WorkspaceManagementService.cs index 74ec756bf5..1638b6510f 100644 --- a/src/Kiota.Builder/WorkspaceManagement/WorkspaceManagementService.cs +++ b/src/Kiota.Builder/WorkspaceManagement/WorkspaceManagementService.cs @@ -42,6 +42,7 @@ public WorkspaceManagementService(ILogger logger, HttpClient httpClient, bool us private readonly DescriptionStorageService descriptionStorageService; public async Task IsConsumerPresentAsync(string clientName, CancellationToken cancellationToken = default) { + DescriptionStorageService.ValidateConsumerName(clientName); if (!UseKiotaConfig) return false; var (wsConfig, _) = await workspaceConfigurationStorageService.GetWorkspaceConfigurationAsync(cancellationToken).ConfigureAwait(false); return wsConfig is not null && (wsConfig.Clients.ContainsKey(clientName) || wsConfig.Plugins.ContainsKey(clientName)); @@ -68,6 +69,7 @@ private BaseApiConsumerConfiguration UpdateConsumerConfiguration(GenerationConfi public async Task UpdateStateFromConfigurationAsync(GenerationConfiguration generationConfiguration, string descriptionHash, Dictionary> templatesWithOperations, Stream descriptionStream, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(generationConfiguration); + DescriptionStorageService.ValidateConsumerName(generationConfiguration.ClientClassName); if (UseKiotaConfig) { var (wsConfig, manifest) = await LoadConfigurationAndManifestAsync(cancellationToken).ConfigureAwait(false); @@ -154,6 +156,7 @@ public async Task ShouldGenerateAsync(GenerationConfiguration inputConfig, } public async Task GetDescriptionCopyAsync(string clientName, string inputPath, bool cleanOutput, CancellationToken cancellationToken = default) { + DescriptionStorageService.ValidateConsumerName(clientName); if (!UseKiotaConfig || cleanOutput) return null; return await descriptionStorageService.GetDescriptionAsync(clientName, new Uri(inputPath).GetFileExtension(), cancellationToken).ConfigureAwait(false); @@ -178,6 +181,7 @@ public Task RemovePluginAsync(string clientName, bool cleanOutput = false, Cance } private async Task RemoveConsumerInternalAsync(string consumerName, Func> consumerRetrieval, bool cleanOutput, string consumerDisplayName, CancellationToken cancellationToken) where T : BaseApiConsumerConfiguration { + DescriptionStorageService.ValidateConsumerName(consumerName); if (!UseKiotaConfig) throw new InvalidOperationException($"Cannot remove a {consumerDisplayName} in lock mode"); var (wsConfig, manifest) = await workspaceConfigurationStorageService.GetWorkspaceConfigurationAsync(cancellationToken).ConfigureAwait(false); diff --git a/tests/Kiota.Builder.Tests/WorkspaceManagement/DescriptionStorageServiceTests.cs b/tests/Kiota.Builder.Tests/WorkspaceManagement/DescriptionStorageServiceTests.cs index b211cb6131..29bb2c57e4 100644 --- a/tests/Kiota.Builder.Tests/WorkspaceManagement/DescriptionStorageServiceTests.cs +++ b/tests/Kiota.Builder.Tests/WorkspaceManagement/DescriptionStorageServiceTests.cs @@ -46,4 +46,86 @@ public async Task DefensiveAsync() await Assert.ThrowsAsync(() => service.UpdateDescriptionAsync("foo", null, cancellationToken: TestContext.Current.CancellationToken)); await Assert.ThrowsAsync(() => service.GetDescriptionAsync(null, cancellationToken: TestContext.Current.CancellationToken)); } + + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("..")] + [InlineData("a/b")] + [InlineData("a\\b")] + [InlineData("./Victim")] + [InlineData(".")] + [InlineData(" ")] + public async Task UpdateDescriptionRejectsTraversalNamesAsync(string clientName) + { + var service = new DescriptionStorageService(tempPath); + using var stream = new MemoryStream(); + stream.WriteByte(0x1); + await Assert.ThrowsAsync(() => service.UpdateDescriptionAsync(clientName, stream, cancellationToken: TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task UpdateDescriptionTraversalDoesNotEscapeConsumerNamespaceAsync() + { + var service = new DescriptionStorageService(tempPath); + using var victimStream = new MemoryStream(); + victimStream.WriteByte(0x2); + await service.UpdateDescriptionAsync("Victim", victimStream, cancellationToken: TestContext.Current.CancellationToken); + + using var maliciousStream = new MemoryStream(); + maliciousStream.WriteByte(0x9); + await Assert.ThrowsAsync(() => service.UpdateDescriptionAsync("junk/../Victim", maliciousStream, cancellationToken: TestContext.Current.CancellationToken)); + + // The victim's cached description must remain untouched (single byte 0x2 written above). + var victimFilePath = Path.Combine(tempPath, DescriptionStorageService.DescriptionsSubDirectoryRelativePath, "Victim", "openapi.yml"); + Assert.True(File.Exists(victimFilePath)); + var contents = await File.ReadAllBytesAsync(victimFilePath, TestContext.Current.CancellationToken); + Assert.Equal([0x2], contents); + } + + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task GetDescriptionRejectsTraversalNamesAsync(string clientName) + { + var service = new DescriptionStorageService(tempPath); + await Assert.ThrowsAsync(() => service.GetDescriptionAsync(clientName, cancellationToken: TestContext.Current.CancellationToken)); + } + + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public void RemoveDescriptionRejectsTraversalNames(string clientName) + { + var service = new DescriptionStorageService(tempPath); + Assert.Throws(() => service.RemoveDescription(clientName)); + } + + [Theory] + [InlineData("../evil")] + [InlineData("a/b")] + [InlineData("a\\b")] + [InlineData("")] + [InlineData(" ")] + public async Task UpdateDescriptionRejectsInvalidExtensionsAsync(string extension) + { + var service = new DescriptionStorageService(tempPath); + using var stream = new MemoryStream(); + stream.WriteByte(0x1); + await Assert.ThrowsAsync(() => service.UpdateDescriptionAsync("clientName", stream, extension, cancellationToken: TestContext.Current.CancellationToken)); + } + + [Theory] + [InlineData("../evil")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task GetDescriptionRejectsInvalidExtensionsAsync(string extension) + { + var service = new DescriptionStorageService(tempPath); + await Assert.ThrowsAsync(() => service.GetDescriptionAsync("clientName", extension, cancellationToken: TestContext.Current.CancellationToken)); + } } diff --git a/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceConfigurationStorageServiceTests.cs b/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceConfigurationStorageServiceTests.cs index c66705c8b7..87f083e80a 100644 --- a/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceConfigurationStorageServiceTests.cs +++ b/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceConfigurationStorageServiceTests.cs @@ -113,6 +113,50 @@ await WriteWorkspaceConfigurationAsync($$""" await Assert.ThrowsAsync(() => service.GetWorkspaceConfigurationAsync(cancellationToken: TestContext.Current.CancellationToken)); } + [InlineData("../Victim")] + [InlineData("junk/../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + [Theory] + public async Task RejectsClientNameWithTraversalKeyAsync(string clientName) + { + var service = new WorkspaceConfigurationStorageService(tempPath); + await WriteWorkspaceConfigurationAsync($$""" + { + "version": "1.0.0", + "clients": { + "{{clientName.Replace("\\", "\\\\", StringComparison.Ordinal)}}": { + "outputPath": "./client" + } + }, + "plugins": {} + } + """); + + await Assert.ThrowsAsync(() => service.GetWorkspaceConfigurationAsync(cancellationToken: TestContext.Current.CancellationToken)); + } + [InlineData("../Victim")] + [InlineData("junk/../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + [Theory] + public async Task RejectsPluginNameWithTraversalKeyAsync(string pluginName) + { + var service = new WorkspaceConfigurationStorageService(tempPath); + await WriteWorkspaceConfigurationAsync($$""" + { + "version": "1.0.0", + "clients": {}, + "plugins": { + "{{pluginName.Replace("\\", "\\\\", StringComparison.Ordinal)}}": { + "outputPath": "./plugin" + } + } + } + """); + + await Assert.ThrowsAsync(() => service.GetWorkspaceConfigurationAsync(cancellationToken: TestContext.Current.CancellationToken)); + } private async Task WriteWorkspaceConfigurationAsync(string content) { var configurationDirectory = Path.Combine(tempPath, WorkspaceConfigurationStorageService.KiotaDirectorySegment); diff --git a/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceManagementServiceTests.cs b/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceManagementServiceTests.cs index eb6a8c64a0..3924903264 100644 --- a/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceManagementServiceTests.cs +++ b/tests/Kiota.Builder.Tests/WorkspaceManagement/WorkspaceManagementServiceTests.cs @@ -264,6 +264,82 @@ await File.WriteAllTextAsync(descriptionPath, @$"openapi: 3.0.1 Assert.NotNull(descriptionCopy); } + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task RemoveClientRejectsTraversalNamesAsync(string clientName) + { + var mockLogger = Mock.Of(); + Directory.CreateDirectory(tempPath); + var service = new WorkspaceManagementService(mockLogger, httpClient, true, tempPath); + await Assert.ThrowsAsync(() => service.RemoveClientAsync(clientName, cancellationToken: TestContext.Current.CancellationToken)); + } + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task RemovePluginRejectsTraversalNamesAsync(string clientName) + { + var mockLogger = Mock.Of(); + Directory.CreateDirectory(tempPath); + var service = new WorkspaceManagementService(mockLogger, httpClient, true, tempPath); + await Assert.ThrowsAsync(() => service.RemovePluginAsync(clientName, cancellationToken: TestContext.Current.CancellationToken)); + } + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task IsConsumerPresentRejectsTraversalNamesAsync(string clientName) + { + var mockLogger = Mock.Of(); + Directory.CreateDirectory(tempPath); + var service = new WorkspaceManagementService(mockLogger, httpClient, true, tempPath); + await Assert.ThrowsAsync(() => service.IsConsumerPresentAsync(clientName, cancellationToken: TestContext.Current.CancellationToken)); + } + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task GetDescriptionCopyRejectsTraversalNamesAsync(string clientName) + { + var mockLogger = Mock.Of(); + Directory.CreateDirectory(tempPath); + var service = new WorkspaceManagementService(mockLogger, httpClient, true, tempPath); + await Assert.ThrowsAsync(() => service.GetDescriptionCopyAsync(clientName, Path.Combine(tempPath, "openapi.yml"), false, cancellationToken: TestContext.Current.CancellationToken)); + } + [Theory] + [InlineData("junk/../Victim")] + [InlineData("../Victim")] + [InlineData("a/b")] + [InlineData("a\\b")] + public async Task UpdateStateRejectsTraversalClientNamesAsync(string clientName) + { + var mockLogger = Mock.Of(); + Directory.CreateDirectory(tempPath); + var service = new WorkspaceManagementService(mockLogger, httpClient, true, tempPath); + var configuration = new GenerationConfiguration + { + ClientClassName = clientName, + OutputPath = Path.Combine(tempPath, "client"), + OpenAPIFilePath = Path.Combine(tempPath, "openapi.yaml"), + ApiRootUrl = "https://graph.microsoft.com", + }; + using var stream = new MemoryStream(); + stream.WriteByte(0x1); + await Assert.ThrowsAsync(() => service.UpdateStateFromConfigurationAsync( + configuration, + "foo", + new Dictionary> { + { "/foo", new HashSet { "GET" } } + }, + stream, cancellationToken: TestContext.Current.CancellationToken)); + } + public void Dispose() { if (Directory.Exists(tempPath))