diff --git a/manifest/manifest.go b/manifest/manifest.go index 9f2fc43..5e5e1e9 100644 --- a/manifest/manifest.go +++ b/manifest/manifest.go @@ -27,6 +27,7 @@ var SubcommandCommands = map[string]bool{ "abaqus": true, "apachectl": true, "openssl": true, + "net": true, } //go:embed manifests/*.yaml manifests/denied/*.yaml manifests/powershell/*.yaml manifests/powershell/denied/*.yaml manifests/packs/*/*.yaml @@ -91,6 +92,13 @@ type Manifest struct { // (after the pid), and jcmd GC.run, VM.set_flag, and JVMTI.agent_load all // change the target JVM. Without this, such a tool is all-or-nothing. PositionalAllowlist *PositionalAllowlist `yaml:"positional_allowlist"` + // NoPositionalArgs rejects every positional argument. This is for tools + // that are read-only bare but write-capable with arguments - `net accounts` + // displays policy, while `net accounts /forcelogoff:30` sets it. Slash-style + // switches reach the validator as positionals, so flag validation alone + // cannot catch them. positional_allowlist cannot express this: an empty + // values list is rejected at load time as a likely manifest bug. + NoPositionalArgs bool `yaml:"no_positional_args"` // AllowsFlagBundling re-enables POSIX-style short-flag bundling for a // manifest that declares shell: powershell. Native Windows executables // (netstat, tracert, ping) run on a Windows host but use DOS-style bundled @@ -299,6 +307,7 @@ func parseManifest(data map[string]any, filePath string) (*Manifest, error) { RequiresOneOf: requiresOneOf, AllowsFlagBundling: defaultBool(data, "allows_flag_bundling"), PositionalAllowlist: positionalAllowlist, + NoPositionalArgs: defaultBool(data, "no_positional_args"), }, nil } diff --git a/manifest/manifest_test.go b/manifest/manifest_test.go index c67d21f..04b38d6 100644 --- a/manifest/manifest_test.go +++ b/manifest/manifest_test.go @@ -20,7 +20,7 @@ func mustLoadEmbedded(t *testing.T) map[string]*Manifest { func TestLoadEmbeddedCountAndNameMatch(t *testing.T) { registry := mustLoadEmbedded(t) - if got, want := len(registry), 363; got != want { + if got, want := len(registry), 364; got != want { t.Fatalf("len(registry) = %d, want %d", got, want) } diff --git a/manifest/manifests/powershell/denied/net.yaml b/manifest/manifests/powershell/denied/net.yaml index 0d15779..4455e19 100644 --- a/manifest/manifests/powershell/denied/net.yaml +++ b/manifest/manifests/powershell/denied/net.yaml @@ -1,4 +1,6 @@ name: net shell: powershell deny: true -reason: "Use Get-Service, Get-NetAdapter, etc. instead." +reason: + "Only 'net accounts' (account/lockout policy, no arguments) is allowed. For everything + else use Get-LocalUser, Get-LocalGroupMember, Get-SmbShare, Get-Service, etc. instead." diff --git a/manifest/manifests/powershell/format-table.yaml b/manifest/manifests/powershell/format-table.yaml index 2b4ec6f..08eb3da 100644 --- a/manifest/manifests/powershell/format-table.yaml +++ b/manifest/manifests/powershell/format-table.yaml @@ -6,5 +6,7 @@ flags: - flag: "-Property" takes_value: true - flag: "-AutoSize" + - flag: "-Wrap" + description: "Wrap long cell text (e.g. event Message columns) instead of truncating it." stdin: true stdout: true diff --git a/manifest/manifests/powershell/get-childitem.yaml b/manifest/manifests/powershell/get-childitem.yaml index e015ec7..b01c36a 100644 --- a/manifest/manifests/powershell/get-childitem.yaml +++ b/manifest/manifests/powershell/get-childitem.yaml @@ -8,6 +8,9 @@ flags: - flag: "-Recurse" - flag: "-Filter" takes_value: true + - flag: "-Include" + takes_value: true + description: "Comma-separated patterns, e.g. 'hs_err_pid*.log','*.hprof'. Unlike -Filter, matches several patterns in one pass; combine with -Recurse." - flag: "-LiteralPath" takes_value: true - flag: "-Name" diff --git a/manifest/manifests/powershell/net_accounts.yaml b/manifest/manifests/powershell/net_accounts.yaml new file mode 100644 index 0000000..eee9f4a --- /dev/null +++ b/manifest/manifests/powershell/net_accounts.yaml @@ -0,0 +1,15 @@ +name: net_accounts +description: + Show the local account policy - lockout threshold, lockout duration, observation + window, password age limits. No Get-* cmdlet exposes this policy. +category: security +shell: powershell +# `net accounts` is read-only ONLY when bare: the same switches that would +# display nothing (/forcelogoff:N, /lockoutthreshold:N, ...) SET the policy. +# Switches are slash-style, so they reach the validator as positionals and +# flag validation cannot catch them. Only the bare read-only form is allowed; +# bare `net` and all other subcommands stay denied. +no_positional_args: true +flags: [] +stdin: false +stdout: true diff --git a/manifest/manifests/powershell/select-string.yaml b/manifest/manifests/powershell/select-string.yaml index 12af415..dfa8447 100644 --- a/manifest/manifests/powershell/select-string.yaml +++ b/manifest/manifests/powershell/select-string.yaml @@ -9,5 +9,8 @@ flags: - flag: "-Path" takes_value: true - flag: "-SimpleMatch" + - flag: "-Context" + takes_value: true + description: "Lines of context around each match, e.g. -Context 2,5 (2 before, 5 after). The grep -B/-A equivalent." stdin: true stdout: true diff --git a/parser/powershell.go b/parser/powershell.go index 8bab20f..67dedcd 100644 --- a/parser/powershell.go +++ b/parser/powershell.go @@ -359,7 +359,7 @@ func diagnoseParseError(input string, parseErr error) error { return &ParseError{Message: "Subexpressions and method calls are not allowed. Use cmdlet parameters instead."} case '{': if i == 0 || input[i-1] != '@' { - return &ParseError{Message: "Script blocks are not supported. Use simplified Where-Object syntax: Where-Object PropertyName -eq Value"} + return &ParseError{Message: "This script block does not parse under the safe subset. Blocks support only: $_ with property access, comparison/arithmetic operators, and [math]::/[datetime]:: calls - e.g. Where-Object { $_.Id -eq 4625 } or @{N='GB';E={[math]::Round($_.WorkingSet64/1GB,2)}}. Cmdlet calls and assignments inside blocks are not allowed."} } case ';': if !insideHashtable(input, i) { diff --git a/parser/powershell_test.go b/parser/powershell_test.go index 26e1f96..b086943 100644 --- a/parser/powershell_test.go +++ b/parser/powershell_test.go @@ -478,3 +478,20 @@ func TestIPLiteralDoesNotShadowExistingTokens(t *testing.T) { } } } + +// The pre-safe-expression error said only "Script blocks are not supported", +// which primed agents to avoid constructs that now work (Where-Object blocks, +// calculated properties). When a block fails to parse, the error must describe +// the supported subset rather than deny that blocks exist. +func TestScriptBlockParseErrorDescribesSafeSubset(t *testing.T) { + _, err := ParsePowerShell("Get-Process | ForEach-Object { Stop-Process $_ }") + if err == nil { + t.Fatal("cmdlet call inside a script block should not parse") + } + msg := err.Error() + for _, want := range []string{"$_", "[math]::"} { + if !strings.Contains(msg, want) { + t.Errorf("script-block parse error should mention %q, got: %v", want, msg) + } + } +} diff --git a/server/hint_test.go b/server/hint_test.go new file mode 100644 index 0000000..8312dee --- /dev/null +++ b/server/hint_test.go @@ -0,0 +1,22 @@ +package server + +import ( + "strings" + "testing" +) + +// The connect hint is the first thing an agent learns about the PowerShell +// dialect, and agents obey it: a recorded 2026-08-05 incident session never +// attempted $_, calculated properties, or the JVM tools because the hint said +// "no $ or {} or ;" - a claim that stopped being true when the safe-expression +// grammar landed. The hint must advertise what the dialect supports. +func TestWinRMConnectHintAdvertisesSafeExpressions(t *testing.T) { + for _, want := range []string{"$_", "[math]::", "jps", "jstack", "FilterHashtable"} { + if !strings.Contains(winrmConnectHint, want) { + t.Errorf("winrm connect hint should mention %q\nhint: %s", want, winrmConnectHint) + } + } + if strings.Contains(winrmConnectHint, "no $ or {}") { + t.Errorf("winrm connect hint still carries the stale no-$-or-{} claim\nhint: %s", winrmConnectHint) + } +} diff --git a/server/server.go b/server/server.go index 8b518c5..1a3e275 100644 --- a/server/server.go +++ b/server/server.go @@ -369,10 +369,20 @@ func (c *Core) connectWinRM(ctx context.Context, in ConnectInput, start time.Tim "host": in.Host, "shell": "powershell", "message": message, - "hint": "Use PowerShell cmdlets (Get-Process, Get-Service, Get-WinEvent, etc.). Single quotes only, no $ or {} or ;. Use | to pipe between commands.", + "hint": winrmConnectHint, }, nil } +// winrmConnectHint describes the PowerShell dialect to the connecting agent. +// Keep it in sync with the parser's safe-expression grammar: agents trust this +// hint completely and never attempt constructs it says are unsupported. +const winrmConnectHint = "Use PowerShell cmdlets (Get-Process, Get-Service, Get-WinEvent, etc.), single-quoted strings, and | to pipe. " + + "Script blocks support $_ and [math]::/[datetime]:: calls: calculated properties like Select-Object @{N='GB';E={[math]::Round($_.WorkingSet64/1GB,2)}} " + + "and filters like Where-Object { $_.Id -eq 4625 } work. " + + "Get-WinEvent -FilterHashtable accepts time bounds: @{LogName='System'; StartTime='2026-08-05 03:00'}. " + + "JVM tools (jps, jstack, jcmd, jmap, jstat) and net accounts are available. " + + "Not supported: variables/assignment, subexpressions $(...), semicolons outside hashtables, redirection." + func (c *Core) Execute(ctx context.Context, in ExecuteInput) (output.CommandResult, error) { if strings.TrimSpace(in.Command) == "" { return output.CommandResult{}, errors.New("command is required") diff --git a/validator/validator.go b/validator/validator.go index 4e01753..b9f94af 100644 --- a/validator/validator.go +++ b/validator/validator.go @@ -462,6 +462,9 @@ func validateArgs(command string, args []string, m *manifest.Manifest) error { } } } else { + if m.NoPositionalArgs { + return &ValidationError{Message: fmt.Sprintf("'%s' does not accept arguments.", command)} + } if m.AllowsPathArgs { if err := checkRestrictedPath(arg, m); err != nil { return err diff --git a/validator/windows_diagnostics_test.go b/validator/windows_diagnostics_test.go new file mode 100644 index 0000000..4c9d8f9 --- /dev/null +++ b/validator/windows_diagnostics_test.go @@ -0,0 +1,79 @@ +package validator + +import ( + "strings" + "testing" + + "github.com/fawdyinc/shellguard/parser" +) + +// These commands come from a recorded 3DEXPERIENCE/SQL Server incident session +// (2026-08-05). Each was needed during diagnosis and rejected by the validator, +// costing turns or degrading the investigation. + +func validatePS(t *testing.T, command string) error { + t.Helper() + p, err := parser.ParsePowerShell(command) + if err != nil { + t.Fatalf("parse %q: %v", command, err) + } + return ValidatePipeline(p, testRegistry(t)) +} + +// Without -Wrap, long Message fields truncate mid-sentence in table view, +// forcing a fallback to the far more verbose Format-List. +func TestFormatTableWrap(t *testing.T) { + if err := validatePS(t, "Get-Process | Format-Table Name, CPU -AutoSize -Wrap"); err != nil { + t.Errorf("Format-Table -Wrap should be allowed: %v", err) + } +} + +// -Filter takes a single Win32 glob; -Include is the only way to search for +// several file patterns (crash dumps, heap dumps, javacores) in one pass. +func TestGetChildItemInclude(t *testing.T) { + if err := validatePS(t, `Get-ChildItem -Path 'C:\logs' -Recurse -Include 'hs_err_pid*.log','*.hprof'`); err != nil { + t.Errorf("Get-ChildItem -Include should be allowed: %v", err) + } +} + +// -Context is grep -B/-A: log lines around a match, not just the match. +func TestSelectStringContext(t *testing.T) { + if err := validatePS(t, `Select-String -Path 'C:\logs\app.log' -Pattern 'OutOfMemoryError' -Context 2,5`); err != nil { + t.Errorf("Select-String -Context should be allowed: %v", err) + } +} + +// Bare `net accounts` is the only reader for lockout/password policy; the +// Get-* cmdlets the net denial points to cannot show it. +func TestNetAccountsAllowed(t *testing.T) { + if err := validatePS(t, "net accounts"); err != nil { + t.Errorf("net accounts should be allowed: %v", err) + } +} + +// net accounts with any argument is a write - /forcelogoff:30 and friends set +// the policy they otherwise display. +func TestNetAccountsRejectsArguments(t *testing.T) { + if err := validateOne(t, "net", "accounts", "/forcelogoff:30"); err == nil { + t.Error("net accounts with arguments must be rejected") + } +} + +func TestNetOtherSubcommandsStayDenied(t *testing.T) { + for _, args := range [][]string{ + {"user"}, + {"user", "eve", "/add"}, + {"localgroup", "administrators"}, + {"share"}, + } { + if err := validateOne(t, "net", args...); err == nil { + t.Errorf("net %s must stay denied", strings.Join(args, " ")) + } + } +} + +func TestBareNetStaysDenied(t *testing.T) { + if err := validateOne(t, "net"); err == nil { + t.Error("bare net must stay denied") + } +}