fix(security): block dangerous stem+wildcard allow patterns in execApprovals.set (#255)
ValidateExecApprovalRules previously checked for dangerous fragments that end
with a trailing space (e.g. "rm ") but missed the case where the wildcard
character replaces the space — e.g. "rm*" passes the "rm " fragment check yet
matches "rm -rf /" via the ^rm.*$ regex, effectively bypassing the intended
block.
Fix: for each dangerous fragment that has trailing whitespace, also reject
patterns containing the trimmed stem followed directly by * or ?.
Before:
{ "pattern": "rm*", "action": "allow" } → accepted, allows "rm -rf /"
{ "pattern": "del*", "action": "allow" } → accepted, allows "del /s /q C:\\"
After:
{ "pattern": "rm*", "action": "allow" } → rejected ("Dangerous allow rule…")
{ "pattern": "del*", "action": "allow" } → rejected
Adds 7 InlineData regression tests covering: rm*, rm?, del*, del?,
remove-item*, shutdown*, net*.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
4207163091
commit
dc640eef32
@ -647,6 +647,16 @@ public class SystemCapability : NodeCapabilityBase
|
||||
{
|
||||
if (normalized.Contains(dangerous, StringComparison.Ordinal))
|
||||
return $"Dangerous allow rule is not permitted: {pattern}";
|
||||
|
||||
// Also block stem+wildcard (e.g. "rm*" bypasses "rm " because the
|
||||
// fragment has a trailing space that the wildcard replaces).
|
||||
var stem = dangerous.TrimEnd();
|
||||
if (stem.Length < dangerous.Length &&
|
||||
(normalized.Contains(stem + "*", StringComparison.Ordinal) ||
|
||||
normalized.Contains(stem + "?", StringComparison.Ordinal)))
|
||||
{
|
||||
return $"Dangerous allow rule is not permitted: {pattern}";
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -654,6 +654,52 @@ public class SystemCapabilityExecApprovalsTests
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that exec-approvals.set rejects Allow rules where a dangerous command stem
|
||||
/// is immediately followed by a wildcard (e.g. "rm*"), which would bypass the trailing-
|
||||
/// space fragment check used for patterns like "rm ".
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData("rm*")]
|
||||
[InlineData("rm?")]
|
||||
[InlineData("del*")]
|
||||
[InlineData("del?")]
|
||||
[InlineData("remove-item*")]
|
||||
[InlineData("shutdown*")]
|
||||
[InlineData("net*")]
|
||||
public async Task ExecApprovalsSet_RejectsDangerousStemPlusWildcardAllowRule(string dangerousPattern)
|
||||
{
|
||||
var tempDir = Path.Combine(Path.GetTempPath(), $"test-{Guid.NewGuid():N}");
|
||||
Directory.CreateDirectory(tempDir);
|
||||
try
|
||||
{
|
||||
var policy = new ExecApprovalPolicy(tempDir, _logger);
|
||||
var cap = CreateCapability(policy);
|
||||
|
||||
var json = JsonDocument.Parse($@"{{
|
||||
""baseHash"": ""{policy.GetPolicyHash()}"",
|
||||
""rules"": [
|
||||
{{""pattern"": ""{dangerousPattern}"", ""action"": ""allow""}}
|
||||
],
|
||||
""defaultAction"": ""deny""
|
||||
}}");
|
||||
|
||||
var request = new NodeInvokeRequest
|
||||
{
|
||||
Command = "system.execApprovals.set",
|
||||
Args = json.RootElement
|
||||
};
|
||||
|
||||
var result = await cap.ExecuteAsync(request);
|
||||
Assert.False(result.Ok);
|
||||
Assert.Contains("Dangerous allow rule is not permitted", result.Error!, StringComparison.OrdinalIgnoreCase);
|
||||
}
|
||||
finally
|
||||
{
|
||||
try { Directory.Delete(tempDir, true); } catch { }
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ExecApprovalsGet_ReturnsPolicy()
|
||||
{
|
||||
|
||||
Loading…
Reference in New Issue
Block a user