Skip to content

fix(security): harden command validation and risk classification #859

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

Draft
yacosta738 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(security): harden command validation and risk classification #859

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
2 changes: 1 addition & 1 deletion .github/workflows/dependabot-auto-merge.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ jobs:
target: squash
approve: true
secrets:
token: ${{ secrets.GITHUB_TOKEN }}
token: ${{ secrets.GITHUB_TOKEN }}
2 changes: 1 addition & 1 deletion .github/workflows/semantic-pull-request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ on:
types: [opened, edited, synchronize]
jobs:
main:
uses: dallay/common-actions/.github/workflows/semantic-pull-request.yml@v2.0.0
uses: dallay/common-actions/.github/workflows/semantic-pull-request.yml@v2.0.0
85 changes: 62 additions & 23 deletions clients/agent-runtime/src/security/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ impl SecurityPolicy {
};

let base = base_raw.to_ascii_lowercase();
let args: Vec<String> = words.map(|w| w.to_ascii_lowercase()).collect();
let args: Vec<String> = words.map(|w| w.to_string()).collect();
let joined_segment = cmd_part.to_ascii_lowercase();

if is_high_risk_base_command(&base) || contains_high_risk_snippet(&joined_segment) {
Expand Down Expand Up @@ -522,16 +522,12 @@ impl SecurityPolicy {
}

let raw_args: Vec<&str> = words.collect();
let normalized_args = match normalize_args_for_path_checks(&raw_args) {
let args = match normalize_args_for_path_checks(&raw_args) {
Some(args) => args,
None => return false,
};
let args: Vec<String> = normalized_args
.iter()
.map(|arg| arg.to_ascii_lowercase())
.collect();

for arg in &normalized_args {
for arg in &args {
let effective_arg = Self::effective_path_arg(arg);
if !self.is_path_argument_safe(effective_arg) {
return false;
Expand Down Expand Up @@ -575,23 +571,35 @@ impl SecurityPolicy {
match base.as_str() {
"find" => {
// find -exec and find -ok allow arbitrary command execution
!args.iter().any(|arg| arg == "-exec" || arg == "-ok")
!args.iter().any(|arg| {
let low = arg.to_ascii_lowercase();
low == "-exec" || low.starts_with("-exec") || low == "-ok" || low.starts_with("-ok")
})
}
"git" => {
// git config, alias, and -c can be used to set dangerous options
// (e.g. git config core.editor "rm -rf /")
!args.iter().any(|arg| {
arg == "config"
|| arg.starts_with("config.")
|| arg == "alias"
|| arg.starts_with("alias.")
|| arg == "-c"
// git config, alias, and -c can be used to set dangerous options
let low = arg.to_ascii_lowercase();
low == "config"
|| low.starts_with("config.")
|| low == "alias"
|| low.starts_with("alias.")
// Case-sensitive block for -c to allow -C
|| arg.starts_with("-c")
|| arg.starts_with("--exec-path")
})
}
"npm" | "pnpm" | "yarn" => {
// npm config and set can be used to set dangerous options
// (e.g. npm config set editor "rm -rf /")
!args.iter().any(|arg| arg == "config" || arg == "set")
"npm" | "pnpm" | "yarn" | "cargo" => {
!args.iter().any(|arg| {
let low = arg.to_ascii_lowercase();
low == "config"
|| low == "set"
|| low == "--config"
|| low.starts_with("--config=")
// Case-sensitive block for -c
|| arg.starts_with("-c")
|| arg.starts_with("--exec-path")
})
}
_ => true,
}
Expand Down Expand Up @@ -778,8 +786,16 @@ fn contains_high_risk_snippet(segment: &str) -> bool {
}

fn is_medium_risk_command(base: &str, args: &[String]) -> bool {
// Only check the first few non-flag arguments for subcommands to avoid false positives on filenames
let verbs: Vec<String> = args
.iter()
.filter(|a| !a.starts_with('-'))
.take(2)
.map(|a| a.to_ascii_lowercase())
.collect();

match base {
"git" => args.first().is_some_and(|verb| {
"git" => verbs.iter().any(|verb| {
matches!(
verb.as_str(),
"commit"
Expand All @@ -794,9 +810,13 @@ fn is_medium_risk_command(base: &str, args: &[String]) -> bool {
| "checkout"
| "switch"
| "tag"
| "clone"
| "pull"
| "fetch"
| "init"
)
}),
"npm" | "pnpm" | "yarn" => args.first().is_some_and(|verb| {
"npm" | "pnpm" | "yarn" => verbs.iter().any(|verb| {
matches!(
verb.as_str(),
"install"
Expand All @@ -811,14 +831,33 @@ fn is_medium_risk_command(base: &str, args: &[String]) -> bool {
| "t"
| "it"
| "cit"
| "ci"
| "init"
| "link"
)
}),
"cargo" => args.first().is_some_and(|verb| {
"cargo" => verbs.iter().any(|verb| {
matches!(
verb.as_str(),
"add" | "remove" | "install" | "clean" | "publish" | "run" | "r" | "test" | "t"
"add"
| "remove"
| "install"
| "clean"
| "publish"
| "run"
| "r"
| "test"
| "t"
| "build"
| "b"
| "check"
| "c"
| "update"
| "init"
| "new"
)
}),
"find" => args.iter().any(|arg| arg.to_ascii_lowercase() == "-delete"),
"touch" | "mkdir" | "mv" | "cp" | "ln" => true,
_ => false,
}
Expand Down
Loading