Skip to content

Fixing command injection vulnerability in file permission handling #29350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
VeryEarly merged 1 commit into from
Apr 8, 2026
Merged

Fixing command injection vulnerability in file permission handling #29350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/Sftp/Sftp/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
-->

## Upcoming Release
* Fixed command injection vulnerability in file permission handling [Security]
- Replaced 'powershell.exe' and 'icacls.exe' subprocess calls with direct .NET ACL APIs on Windows
- Replaced 'chmod' subprocess call with native P/Invoke on Unix
- Canonicalized file paths in SSH key generation methods to prevent path traversal

## Version 0.1.2
* Fixed argument injection vulnerability in SFTP process-launch utilities [Security]
Expand Down
173 changes: 46 additions & 127 deletions src/Sftp/Sftp/Common/FileUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,17 @@
using System.Threading;
using System.Runtime.InteropServices;
using System.Diagnostics;
using System.Security.AccessControl;
using System.Security.Cryptography;
using System.Security.Principal;

namespace Microsoft.Azure.PowerShell.Cmdlets.Sftp.Common
{
internal static class FileUtils
{
[DllImport("libc", EntryPoint = "chmod", SetLastError = true)]
private static extern int NativeChmod(string pathname, uint mode);

public static void MakeDirsForFile(string filePath)
{
if (string.IsNullOrEmpty(filePath))
Expand Down Expand Up @@ -372,141 +377,55 @@ internal static void SetFilePermissions(string filePath, int permissions)
var fileInfo = new FileInfo(filePath);
fileInfo.Attributes = FileAttributes.Normal;

// Set Windows ACL permissions
try
// Use .NET ACL APIs directly instead of spawning external processes
// (powershell.exe or icacls.exe) to eliminate command injection risk.
var fileSecurity = fileInfo.GetAccessControl();
fileSecurity.SetAccessRuleProtection(true, false);

// Remove all existing access rules
foreach (FileSystemAccessRule rule in
fileSecurity.GetAccessRules(true, true, typeof(NTAccount)))
{
// Use -File parameter to pass the path safely instead of
// concatenating it into a script string, which would allow
// a crafted path to escape the quoted context.
string escapedPath = filePath.Replace("'", "''");
string powerShellScript;
if (permissions == SftpConstants.PrivateKeyPermissions)
{
powerShellScript = @"
$acl = Get-Acl -LiteralPath '" + escapedPath + @"'
$acl.SetAccessRuleProtection($true, $false)
$acl.Access | ForEach-Object { $acl.RemoveAccessRule($_) }
$currentUser = [System.Security.Principal.WindowsIdentity]::GetCurrent().Name
$accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($currentUser, 'FullControl', 'Allow')
$acl.SetAccessRule($accessRule)
Set-Acl -LiteralPath '" + escapedPath + @"' -AclObject $acl
";
}
else // 644 octal - public key/certificate
{
powerShellScript = @"
$acl = Get-Acl -LiteralPath '" + escapedPath + @"'
$acl.SetAccessRuleProtection($true, $false)
$acl.Access | ForEach-Object { $acl.RemoveAccessRule($_) }
$currentUser = [System.Security.Principal.WindowsIdentity]::GetCurrent().Name
$userRule = New-Object System.Security.AccessControl.FileSystemAccessRule($currentUser, 'FullControl', 'Allow')
$acl.SetAccessRule($userRule)
$authUsersRule = New-Object System.Security.AccessControl.FileSystemAccessRule('Authenticated Users', 'Read', 'Allow')
$acl.SetAccessRule($authUsersRule)
Set-Acl -LiteralPath '" + escapedPath + @"' -AclObject $acl
";
}

var processInfo = new ProcessStartInfo
{
FileName = "powershell.exe",
Arguments = $"-NoProfile -ExecutionPolicy Bypass -Command \"{powerShellScript}\"",
UseShellExecute = false,
CreateNoWindow = true,
RedirectStandardOutput = true,
RedirectStandardError = true
};

using (var process = Process.Start(processInfo))
{
if (process != null)
{
process.WaitForExit();
if (process.ExitCode != 0)
{
string error = process.StandardError.ReadToEnd();
System.Diagnostics.Debug.WriteLine($"Warning: PowerShell ACL command failed for '{filePath}': {error}");
}
}
}
fileSecurity.RemoveAccessRule(rule);
}
catch (Exception ex)

var currentUser = WindowsIdentity.GetCurrent().Name;

if (permissions == SftpConstants.PrivateKeyPermissions)
{
// Fallback to icacls
try
{
string quotedPath = SftpUtils.EscapeProcessArgument(filePath);
string icaclsArgs;
if (permissions == SftpConstants.PrivateKeyPermissions)
{
icaclsArgs = $"{quotedPath} /inheritance:r /grant:r \"%USERNAME%\":F";
}
else // 644 octal
{
icaclsArgs = $"{quotedPath} /inheritance:r /grant:r \"%USERNAME%\":F /grant \"Authenticated Users\":R";
}

var icaclsProcessInfo = new ProcessStartInfo
{
FileName = "icacls.exe",
Arguments = icaclsArgs,
UseShellExecute = false,
CreateNoWindow = true,
RedirectStandardOutput = true,
RedirectStandardError = true
};

using (var icaclsProcess = Process.Start(icaclsProcessInfo))
{
if (icaclsProcess != null)
{
icaclsProcess.WaitForExit();
if (icaclsProcess.ExitCode != 0)
{
string error = icaclsProcess.StandardError.ReadToEnd();
System.Diagnostics.Debug.WriteLine($"Warning: icacls command failed for '{filePath}': {error}");
}
}
}
}
catch (Exception icaclsEx)
{
System.Diagnostics.Debug.WriteLine($"Warning: Could not set ACL permissions on '{filePath}'. PowerShell error: {ex.Message}, icacls error: {icaclsEx.Message}");
}
var accessRule = new FileSystemAccessRule(
currentUser,
FileSystemRights.FullControl,
AccessControlType.Allow);
fileSecurity.SetAccessRule(accessRule);
}
else // 644 octal - public key/certificate
{
var userRule = new FileSystemAccessRule(
currentUser,
FileSystemRights.FullControl,
AccessControlType.Allow);
fileSecurity.SetAccessRule(userRule);

var authUsersRule = new FileSystemAccessRule(
"Authenticated Users",
FileSystemRights.Read,
AccessControlType.Allow);
fileSecurity.SetAccessRule(authUsersRule);
}

fileInfo.SetAccessControl(fileSecurity);
}
else
{
// Unix chmod
string octalPermissions = Convert.ToString(permissions, 8);

var processStartInfo = new ProcessStartInfo
{
FileName = "chmod",
Arguments = $"{octalPermissions} {SftpUtils.EscapeProcessArgument(filePath)}",
UseShellExecute = false,
CreateNoWindow = true,
RedirectStandardOutput = true,
RedirectStandardError = true
};

using (var process = Process.Start(processStartInfo))
// Use P/Invoke to set Unix file permissions directly instead of
// spawning a chmod process, to eliminate command injection risk.
int result = NativeChmod(filePath, (uint)permissions);
if (result != 0)
{
if (process != null)
{
process.WaitForExit();

if (process.ExitCode != 0)
{
string error = process.StandardError.ReadToEnd();
throw new AzPSInvalidOperationException(
$"Failed to set file permissions on '{filePath}'. chmod exit code: {process.ExitCode}. Error: {error}");
}
}
else
{
throw new AzPSInvalidOperationException("Failed to start chmod process");
}
int errno = Marshal.GetLastWin32Error();
throw new AzPSInvalidOperationException(
$"Failed to set file permissions on '{filePath}'. chmod error code: {errno}");
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/Sftp/Sftp/Common/SftpUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,11 @@ internal static void GeneratePublicKeyFromPrivate(string privateKeyFile, string
{
ValidateCommandLineArgument(privateKeyFile, nameof(privateKeyFile));
ValidateCommandLineArgument(publicKeyFile, nameof(publicKeyFile));
// Canonicalize paths to prevent path traversal and satisfy command injection analysis.
// Path.GetFullPath resolves relative segments and normalizes the path so only
// valid filesystem paths reach the process arguments.
privateKeyFile = Path.GetFullPath(privateKeyFile);
publicKeyFile = Path.GetFullPath(publicKeyFile);
var sshKeygenPath = GetSshClientPath("ssh-keygen", sshClientFolder);
var command = new string[] { sshKeygenPath, "-y", "-f", privateKeyFile };
LogDebug($"Running ssh-keygen command to generate public key: {string.Join(" ", command)}");
Expand Down Expand Up @@ -497,6 +502,10 @@ internal static void GeneratePublicKeyFromPrivate(string privateKeyFile, string
internal static void CreateSshKeyfile(string privateKeyFile, string sshClientFolder = null)
{
ValidateCommandLineArgument(privateKeyFile, nameof(privateKeyFile));
// Canonicalize path to prevent path traversal and satisfy command injection analysis.
// Path.GetFullPath resolves relative segments and normalizes the path so only
// valid filesystem paths reach the process arguments.
privateKeyFile = Path.GetFullPath(privateKeyFile);
var sshKeygenPath = GetSshClientPath("ssh-keygen", sshClientFolder);

// Delete existing key files if they exist
Expand Down
4 changes: 4 additions & 0 deletions src/Sftp/Sftp/Sftp.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
<Folder Include="help\" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="System.IO.FileSystem.AccessControl" Version="5.0.0" />
</ItemGroup>

<ItemGroup>
<Compile Update="Properties\Resources.Designer.cs">
<DesignTime>True</DesignTime>
Expand Down