Skip to content

SECURITY.md

Latest commit

 

History

History
387 lines (340 loc) · 25.6 KB

SECURITY.md

File metadata and controls

387 lines (340 loc) · 25.6 KB
Raw

Security notes

This app lets authorized users execute arbitrary scripts, optionally as a different Windows account, optionally elevated. That's a lot of power concentrated in one place. Read this before deploying it.

The two required secrets

Neither ships with a default - the app refuses to start without them so you can't accidentally run with a guessable key.

Environment variable Used for Generate with
SCRIPTRUNNER_MASTER_KEY AES-256-GCM encryption of stored account passwords at rest [Convert]::ToBase64String([System.Security.Cryptography.RandomNumberGenerator]::GetBytes(32)) (PowerShell 7) or openssl rand -base64 32
SCRIPTRUNNER_JWT_SIGNING_KEY Signs login session tokens same as above

Both must decode to at least 32 bytes. Store them the way you'd store any production secret (a secrets manager, a protected CI variable, DPAPI-protected config, etc.) - not committed to source control. If you rotate SCRIPTRUNNER_MASTER_KEY, every stored credential becomes undecryptable and must be re-entered; if you rotate SCRIPTRUNNER_JWT_SIGNING_KEY, every logged-in session is invalidated.

Don't generate either key with Get-Random (an earlier version of this doc, the README, and IMasterKeyProvider.cs's own comment suggested exactly that, and have since been corrected) - Get-Random is a fast, statistically-random PRNG, not a cryptographically secure one, and both of these values are keys, not data. On Windows PowerShell 5.1 (rather than PowerShell 7), RandomNumberGenerator.GetBytes isn't available - use [System.Security.Cryptography.RandomNumberGenerator]::Create().GetBytes($bytes) instead (the same fix applied to Install-Service.ps1's own key generation, which runs under 5.1 as an MSI deferred custom action).

Both keys now have their length enforced in code. AesCredentialProtector's constructor throws unless SCRIPTRUNNER_MASTER_KEY decodes to exactly 32 bytes (AES-256), and JwtTokenService's constructor throws unless SCRIPTRUNNER_JWT_SIGNING_KEY decodes to at least 32 bytes (HMAC-SHA256 doesn't require an exact key length the way AES-256 does, so this is a minimum, not an exact match) or isn't valid base64 at all. Both are resolved eagerly at startup (see Program.cs's "fail fast" scope block) so a missing/short/invalid key is caught immediately rather than on first login or first credential use.

ICredentialProtector/IMasterKeyProvider (in ScriptRunner.Core/Services) are small interfaces specifically so you can swap EnvironmentMasterKeyProvider for one backed by Azure Key Vault, AWS KMS, or similar, without touching anything else.

The initial admin account

An installed Windows Service (the MSI, or Install-Service.ps1) no longer has a default admin credential at all in the normal path: first launch shows a web-based first-run setup wizard (SetupController/SetupWizardPage) where you pick the data directory and set a real admin username/password yourself - see the root README's "First run for an installed service". Until that wizard completes, SetupGateMiddleware blocks the rest of the API (everything except /api/setup/*), so there's no window where an unconfigured instance with a guessable admin credential is reachable.

Local development (dotnet run) and scripted/unattended deployments still use the older, simpler path: if SCRIPTRUNNER_ADMIN_PASSWORD isn't set before the first run (and the wizard is bypassed - see below), DbSeeder creates the admin account with a fixed, hardcoded default password (DefaultAdminPassword in ScriptRunner.Data/Seed/DbSeeder.cs, currently "ScriptRunner"). This is a deliberate simplicity trade-off for local/dev use, but it means every such instance starts with the exact same, publicly-known admin credential until someone logs in and changes it - a classic "known default credentials" exposure if the instance is reachable before that happens. (Setting SCRIPTRUNNER_ADMIN_USERNAME/SCRIPTRUNNER_ADMIN_PASSWORD before an installed service's first startup also auto-completes setup at the default data directory using those values, skipping the wizard entirely - useful for scripted deployments, but it inherits this same "don't skip SCRIPTRUNNER_ADMIN_PASSWORD" caveat.)

Mitigate this by doing one of:

  • For an installed service, just use the wizard - there's no default password to worry about in that path.
  • For local dev or a scripted deployment, set SCRIPTRUNNER_ADMIN_USERNAME/ SCRIPTRUNNER_ADMIN_PASSWORD before first run so the default is never used at all.
  • If you do rely on the default, log in and change the password before exposing the instance beyond localhost/a trusted admin - a warning is logged and written to first-run-admin-password.txt next to the database as a reminder.
  • Firewall the port to trusted networks/VPN until the password is rotated.

The first-run setup wizard's API is intentionally unauthenticated

SetupController (GET/POST /api/setup/*) has no [Authorize] on it, on purpose - there's no admin account (and often no database at all) to authenticate against until setup finishes. That includes the directory-browsing endpoints (GET /api/setup/browse, POST /api/setup/create-directory), which let an unauthenticated caller enumerate directory names and drive letters on the ScriptRunner host, and create new empty folders anywhere the service account can write. Each of these endpoints refuses once IDataDirectoryProvider.IsSetupComplete is true (a 409 Conflict), so this window closes permanently the moment setup finishes - but until then, anyone who can reach the port can browse the server's directory structure and create folders on it. SetupGateMiddleware blocks every other API endpoint during this window, so the practical exposure is limited to directory names/structure and empty-folder creation, not data access - but treat "reachable before setup completes" the same way you would the default-password window above: firewall the port to trusted networks until an admin has walked through the wizard, especially on a network you don't fully trust.

Network-referenced scripts and their credentials

A script with SourceMode: NetworkReference (see the root README's "Network-referenced scripts") has ScriptRunner read a UNC path live at the start of every run, optionally using a stored credential to authenticate to that share. A few things worth knowing:

  • The network path is only as trustworthy as whoever can write to that share. ScriptRunner executes whatever is on the share at run time, with no diffing, review, or pinning to a known-good version - if the share is writable by more people than the script's own Write-permission list in ScriptRunner, that's a wider blast radius than a Stored script (where changing the content requires ScriptRunner Write access specifically). Restrict write access to the share itself accordingly, the same way you would the ScriptRunner credential store.
  • The credential used to read the network path is decrypted the same way run-as credentials are (ICredentialProtector/AES-256-GCM - see "The two required secrets" above) and is never logged or returned to the browser.
  • Impersonation is one-shot and scoped to just the file read, via LogonUser with LOGON32_LOGON_NEW_CREDENTIALS + WindowsIdentity.RunImpersonated (see NetworkScriptContentReader) - deliberately not LOGON32_LOGON_INTERACTIVE (what run-as/elevation uses) and deliberately not a persistent net-use-style session. This means the network credential's account does not need local logon rights on the ScriptRunner host at all (unlike a run-as credential, which does), only read access to the share - a real advantage for locking down that account's blast radius. It also means two network-referenced scripts on different shares can use two different credentials in the same process without Windows's usual one-credential-per-server restriction getting in the way.
  • This impersonation path has not been exercised on a real Windows machine as part of building this feature, for the same reason noted throughout this document (see "This has not been run on a real Windows machine" below) - budget real testing time, in particular: a share requiring a domain credential vs. a local one, an access-denied case (confirm it's classified AccessDenied and not silently treated as NotFound), and a path that becomes unreachable mid-run (network blip) to confirm it fails the run cleanly rather than hanging.

How "run as a different account" and "elevation" actually work

This is the part of the app most worth scrutinizing. Short version: it's the same family of Win32 techniques Sysinternals PsExec's -h switch uses, run from a Windows Service.

  1. No credential chosen: the script just runs as whatever account the ScriptRunner process itself runs as (System.Diagnostics.Process, straightforward). If RequiresElevation is also set with no run-as account, that's a no-op - there's no meaningful "elevate myself" operation for a non-interactive service process, and the UI/logs say so.

  2. A credential is chosen, not elevated: WindowsProcessLauncher calls LogonUser with the stored (decrypted) username/password to get a token for that account, then CreateProcessAsUser to launch the script under that token - with the process's stdout/stderr wired to pipes we own, so output still streams into the UI. (.NET's own ProcessStartInfo.UserName/Password can't do this - it explicitly disallows redirecting output when launching as a different user.)

  3. A credential is chosen, elevated: if the target account is a UAC "split-token" administrator, Windows keeps a second, fully-elevated token linked to the normal (filtered) one you get back from LogonUser. GetTokenInformation(..., TokenLinkedToken) retrieves that linked token, and that token is what gets passed to CreateProcessAsUser. This is exactly how psexec -h avoids an interactive UAC prompt. If the account isn't a split-token admin (or UAC is off), there's no linked token to fetch; ScriptRunner logs that and continues with the standard token rather than failing the run.

All of this lives in ScriptRunner.Core/Services/Execution/Windows/NativeMethods.cs and WindowsProcessLauncher.cs.

This has not been run on a real Windows machine

This project was built inside a Linux sandbox with no Windows host available, and the sandbox couldn't install the .NET SDK either (no network route to nuget.org/dot.net), so none of this code has been compiled or executed for real. The P/Invoke signatures match the documented Win32 contracts and the overall approach mirrors long-established tools (PsExec, various RMM agents), but "matches the docs" and "actually works on your Windows Server build with your Group Policy" are different claims. Budget real testing time before trusting this with anything sensitive.

Testing the elevation path

Before relying on this in production, on an actual Windows test box:

  1. dotnet build the whole solution and fix anything the compiler flags - the code here was written without a compiler in the loop.
  2. Create a local admin test account (a UAC split-token admin, not a domain account, is the simplest first test) and store it as a credential.
  3. Create a trivial diagnostic script, e.g. a PowerShell script containing just whoami /all and echo $env:USERPROFILE, and run it:
    • As the service account (no credential) - confirm output streams live.
    • As the test admin account, not elevated - confirm whoami shows that account, and check whoami /groups shows the filtered token (Administrators group listed as "Deny" or absent from enabled groups).
    • As the test admin account, elevated - confirm whoami /groups now shows Administrators enabled. This is the case most worth verifying carefully; if GetTokenInformation(TokenLinkedToken) isn't returning what's expected, this is where it'll show up as "elevation silently did nothing."
  4. Test with a domain account if your real usage involves domain credentials - domain logon semantics, profile loading, and CreateEnvironmentBlock behavior can differ from local-account testing.
  5. Test what happens when the account/password is wrong (should surface a clear LogonUser failed error, not a hang) and when a script times out (should show up as a killed process, not an orphan).
  6. Load-test a handful of concurrent runs. WindowsProcessLauncher serializes the pipe-creation + CreateProcessAsUser step with a semaphore (to avoid a handle-inheritance race between concurrent launches - see the comment above ProcessCreationLock), and the process-exit wait currently occupies a thread-pool thread for the life of each run; if you need high concurrency, that wait loop is the first thing to revisit (switch to ThreadPool.RegisterWaitForSingleObject for a truly async wait, and consider STARTUPINFOEX + PROC_THREAD_ATTRIBUTE_HANDLE_LIST to remove the serialization).
  7. Specifically exercise NativeEnvironmentBlock.Merge (see "Batch scripts: cmd.exe argument injection" below) - it's new, hand-rolled Win32 environment-block marshaling that hasn't run on a real machine any more than the rest of this file has:
    • A Batch script with a couple of declared parameters that just does echo Name is %Name% (or similar) for each one, run with no run-as account, then again with one - confirm the values come through correctly both ways, and that ordinary profile variables (%USERPROFILE%, %PATH%) are still intact in the run-as case (i.e. the merge didn't drop the rest of the environment).
    • A batch parameter value containing %SCRIPTRUNNER_MASTER_KEY%, &, |, or " - as of the 2026-08-31 real-machine test this should now make the run fail immediately with BatchExecutor's "can't safely receive" error, not run at all - see "Batch scripts: cmd.exe argument injection" below for why the earlier "comes through as inert literal text" expectation here was itself wrong.
    • A batch script parameter name that collides with the reserved list (e.g. PATH) - confirm the run fails clearly with BatchExecutor's "collides with a reserved environment variable" error rather than silently clobbering something.

Batch scripts: cmd.exe argument injection ("BatBadBut") - two fixes, the first was NOT enough

This was the most serious issue found in a security review of this codebase, and the first fix for it turned out to be incomplete - caught only because it was actually run against real cmd.exe rather than trusted on paper. If you take one lesson from this section: don't consider the second fix below closed either until it's been re-run the same way.

BatchExecutor used to build a batch script's command line by quoting each parameter value with WindowsCommandLine.QuoteArgument - correct, standard CreateProcess/argv-style backslash-and-quote escaping - and running it via cmd.exe /d /c "<script.bat>" <args>. That quoting scheme is the right one for an ordinary executable, but cmd.exe does not parse its command line the way normal Win32 programs do: it re-scans the entire line itself, and critically, %VARIABLE% environment-variable expansion happens as an early, separate pass that is not suppressed by being inside double quotes. This is a well-documented vulnerability class - see GMO Flatt Security's "BatBadBut: you can't securely execute commands on Windows", which affected the standard library command-execution APIs of Python, Rust, Go, Node.js, Java, Erlang and others (each assigned its own CVE) for exactly this reason: quoting an argument correctly for CreateProcess does not make it safe to pass to a .bat/.cmd file run through cmd.exe.

Concretely, against this codebase: any user with mere Execute permission on a batch script (not Write - the permission this document elsewhere describes as the safe, content-can't-be-changed tier) could previously supply a parameter value like %SCRIPTRUNNER_MASTER_KEY% and have cmd.exe expand it to the real secret before the script even ran, putting the plaintext key into the child process's command line - and, per the same research, a single character read out of %CMDCMDLINE% was enough to break out of the quoting and inject arbitrary additional commands. PowerShellExecutor was never affected - powershell.exe uses normal Win32 argv parsing, not cmd.exe's line-rescanning/percent-expansion behavior.

Fix #1 (necessary, not sufficient on its own): BatchExecutor no longer puts parameter values on the command line at all. Each declared parameter is now delivered to the script as an environment variable of the same name (%ParamName% from inside the script, not %1/%2) instead of a positional argument - see BatchExecutor.BuildLaunchSpec's own doc comment for the full reasoning. That means the actual cmd.exe /d /c "script.bat" launch line never contains any attacker-controlled text, so there's nothing left for cmd.exe's expansion/rescanning to act on at launch. LaunchSpec.EnvironmentVariables carries the values through to WindowsProcessLauncher, which sets them on ProcessStartInfo.Environment for the no-run-as fast path, and merges them into the native environment block (on top of, not replacing, the profile's own environment) for the run-as path via the new NativeEnvironmentBlock.Merge helper. Parameter names are validated against a strict ^[A-Za-z_][A-Za-z0-9_]*$ pattern and a reserved-name list (PATH, COMSPEC, SCRIPTRUNNER_MASTER_KEY, etc.) in BatchExecutor before they ever become a real environment variable name - names come from whoever has Write access to the script (the same trust tier as script content itself), so this is defense-in-depth rather than the primary fix.

This alone was tested against real cmd.exe on 2026-08-31 and confirmed insufficient. The test script (test-scripts/3-batch-injection-fix-test.bat) declares a required TestValue parameter and does a plain echo %TestValue%. Run with TestValue set to %SCRIPTRUNNER_MASTER_KEY% & whoami & echo INJECTED_MARKER_XYZ, the actual output was:

%SCRIPTRUNNER_MASTER_KEY%
nt authority\system
INJECTED_MARKER_XYZ

whoami and the second echo both ran as their own commands - full command injection, under whatever account/privilege that run used. (The master key reference happened to come back unexpanded in this particular run rather than leaking the real key, but that's incidental to this run's environment, not something to rely on - see fix #2.) The reason fix #1 doesn't close this: cmd.exe's %VARIABLE% expansion isn't only a hazard on the original launch command line - it happens every time cmd.exe reads a line inside the script too, as a raw textual substitution done before that line is tokenized into separate commands. So echo %TestValue% inside the .bat file itself is exactly where the substituted value gets re-interpreted as cmd.exe syntax, regardless of how safely the variable was populated. ScriptRunner doesn't control how a script author references a parameter inside their own script content, so this can't be fixed by anything on the launch side alone.

Fix #2: BatchExecutor now rejects, before starting the process at all, any parameter value containing a character that's dangerous once re-expanded inside a script's own body: & | < > ^ " % or a bare CR/LF. The run fails immediately with a clear error naming the offending parameter, rather than silently letting cmd.exe reinterpret it. This is necessarily a blunt instrument - the BatBadBut research's own conclusion is that there is no generally-safe way to quote around this for cmd.exe - so a legitimate value that happens to need one of these characters (a literal & in a name, say) simply can't be passed to a Batch script parameter; use a PowerShell script instead for that case, since PowerShell's parameter binding doesn't re-parse a substituted value as code the same way.

This does change how batch scripts are authored: existing scripts written against the old %1/%2 positional convention need to be rewritten to read %ParamName% instead. Since this hasn't shipped to real users yet, now is the right time for that change.

Re-test needed: re-run 3-batch-injection-fix-test.bat with the same payload above against a rebuilt/restarted service and confirm the run now fails with fix #2's error message instead of executing anything. Also worth trying a value with just a bare % (no other metacharacters) to confirm that alone is rejected too, since it's the one most likely to look "safe" to a future script author.

SignalR run-log access - fixed, was relying on the REST layer instead of the hub itself

ExecutionHub.JoinRun(int runId) used to add the caller's SignalR connection to the "run-{runId}" group with no authorization check of its own - the hub's own doc comment said access was "enforced by RunHistoryController/ExecutionController before a client is ever told a run id exists." In practice, runId is a normal EF Core auto-incrementing integer primary key, which is sequential and therefore guessable/enumerable - it isn't a secret, and nothing stopped an authenticated user (any authenticated user, not just Admins) from calling JoinRun with a run ID they were never told about and were never granted Execute or Write permission on. RunHistoryController correctly filters non-admins to their own runs, but that only protected the REST history endpoints - ExecutionHub didn't apply the same ScriptPermissionRequirement/ownership check, so a low-privileged user could watch another user's script run live (stdout/stderr, non-secret parameters, exit codes, and anything a script prints - secret-marked parameters were, and still are, separately redacted by ScriptExecutionOrchestrator, so those specifically were never exposed by this gap, but everything else in the log stream was).

The fix: JoinRun now looks up the run's owning user and script, and applies the same authorization check ExecutionController.Run/Cancel already use (admin, run owner, or ScriptPermissionRequirement(Execute) on the script) before adding the caller's connection to the run's SignalR group - a caller without access gets the same "Run not found or access denied" HubException whether the run id doesn't exist or just isn't theirs, so the error itself can't be used to enumerate real run ids.

Choosing the service account

LogonUser, CreateProcessAsUser, and (especially) reading another account's linked elevated token generally require the caller to hold SeAssignPrimaryTokenPrivilege, SeIncreaseQuotaPrivilege, and effectively SeTcbPrivilege-level trust. LocalSystem has all of this by default, which is why it's the simplest choice and what the README's sc.exe create example implies. If your security policy requires avoiding LocalSystem, you'll need to explicitly grant the chosen service account the above privileges (Local Security Policy > User Rights Assignment) and test the elevation path again - don't assume a lesser account works without verifying it.

Either way, treat the ScriptRunner service account as highly privileged: anyone who can edit scripts with Write access, or who administers the credential store, can effectively run arbitrary code as any stored account. Restrict Admin and Write access accordingly.

Other things worth knowing

  • Secret parameters: a script parameter marked "secret" is masked with ******** in log output (RedactSecrets in ScriptExecutionOrchestrator) by literal string replacement. This catches the common case but not, e.g., a value the script itself re-encodes (base64, URL-encoding) before printing it. Don't rely on it as the only control for highly sensitive values - avoid having scripts echo secrets at all.
  • Script content is trusted, PowerShell/Batch itself is not sandboxed. Anyone with Write access to a script can make it do anything the run-as account can do. Execute access does not include Write access to the script's content by default - use that distinction deliberately.
  • JWTs are bearer tokens with an 8-hour default expiry (Jwt:ExpiryMinutes) and no refresh-token flow. A stolen token is valid until it expires; lower the expiry if that's a concern for your environment, or add refresh tokens/short-lived access tokens if you need tighter control.
  • CORS is only opened up for http://localhost:5173 (the Vite dev server) and only in the Development environment. Serve the built frontend from the same origin as the API in production (or add its origin to the CORS policy) rather than widening this.
  • SQLite is fine for a single-instance internal tool but gives you no built-in encryption-at-rest for the database file itself (only the credential passwords inside it are separately encrypted via AES-GCM). Protect the file with filesystem permissions/BitLocker if that matters for your environment.
  • Fixed: the data directory path is now validated against connection-string special characters. DataDirectoryProvider.ConnectionString builds "Data Source={dataDirectory}\scriptrunner.db" by plain string interpolation, and a data directory containing a semicolon (a legal character in a Windows folder name) would have injected extra Microsoft.Data.Sqlite connection-string keywords instead of being taken as a literal path segment. Low severity in practice - the only people who could set this value are whoever can reach the pre-setup wizard (already a documented trust boundary above) or an admin editing appsettings/the service's environment directly - but cheap to close, so DataDirectoryProvider.CompleteSetup now rejects any data directory containing ; alongside its existing absolute-path check.