#1138 Fixes for high severity issues found in audit Pull request opened by javuto Problem A security review of revision

fc7ed508

reported five high-severity findings: 1. Federated login could take over an account with the same username. Login matched users by username only, ignoring the IdP subject. A different identity that gained an existing federated username got that account's admin flag and environment permissions. The same was true across OIDC and SAML. 2. Enrolling with B's secret could overwrite a node in A. Re-enrollment checked for the UUID in its own environment, but then archived and updated whichever row with that UUID came back first across all environments. With B's enroll secret, a node's row in environment A could be overwritten. The node then kept A's numeric ID, so it could receive A's pending queries. Deleting from B had the same problem: the permission check was environment-scoped, but the delete was not. 3. Enrollment one-liners ran scripts with TLS verification off. If an environment had a stored certificate, the API and CLI one-liners used

curl -k

or a trust-all PowerShell callback, then executed the downloaded script. Anyone on the network path during bootstrap could replace that script. 4. A node could reset another node's file carve. Carve init updated every carve with the same

request_id

, without checking which node or environment it belonged to. Any enrolled node that knew a request ID could reset another node's carve transfer, across environments. Because every node targeted by one carve query shares that request ID, init also reset other nodes' legitimate carves. 5. Anonymous requests could grow metrics without limit. The osquery request-duration metric was labelled with the raw URL path. Every distinct

/{env}/...

value from an unauthenticated caller created a new label set, even when the request got a 400. The

log_type

label also took node-supplied values unchecked. Changes [1] Federated identity binding •

ResolvedIdentity

gains an

Issuer

field. OIDC fills it from the verified ID token, and SAML from the assertion's issuer. •

AdminUser

gets two new columns,

oidc_subject

and

saml_subject

. Each stores the user's IdP identity as

<issuer>#<subject>

. They are hidden from API JSON. • `resolveFederatedUser`: • looks the user up by binding first, so an IdP rename keeps the account; • falls back to the username only when that user holds this identity's binding, or has none yet for this protocol; • binds an unbound user on that login, only if the row is still unbound, and writes an audit entry; • rejects users bound to a different identity, including the same subject from a different issuer; • binds users created on first login (JIT) immediately. • The existing

linkLocalAccounts

rule for local password accounts is unchanged. [2] Environment-scoped node lifecycle •

NodeManager.Archive

,

Update

and

ArchiveDelete

now take the exact node row instead of looking it up again by UUID.

ArchiveDeleteByUUIDEnv

replaces

ArchiveDeleteByUUID

. • Enrollment finds the existing node with

GetByUUIDEnv(uuid, env.ID)

and updates only that row. • The API node delete removes only the row it already checked permissions for. • Overrides, extensions and osquery-version pins are stored per UUID only. They are cleared only if no other environment still has that UUID (in both the API handler and the CLI). •

osctrl-cli --db

node delete now uses

--env

. The shell's

DeleteNode

also uses the active environment. [3] Certificate pinning in one-liners • A stored environment certificate is now pinned, never used as a reason to skip verification: • shell:

curl -sk --pinnedpubkey 'sha256//…'

(curl still checks the pin with

-k

); • PowerShell: a validation callback that compares the server certificate's SHA-256 to the stored certificates. • A certificate that can't be parsed returns an error instead of falling back to no verification. • The API no longer infers insecure mode from having a certificate. Only the explicit CLI

--insecure

flag turns verification off. • The CLI now reports one-liner generation errors instead of ignoring them. [4] Carve init scoped to the requesting node •

InitCarve

updates exactly one row in one conditional

UPDATE

. It must match

carve_id

,

request_id

, the node, its environment and

status = scheduled

. Zero rows updated is an error, which also rejects a second init of a carve already in progress. • Removed

Carves.GetByRequest

, which no longer has callers. [5] Bounded metric labels •

PrometheusMiddleware

labels by

r.Pattern

, the registered route, instead of

r.URL.Path

. The osctrld middleware already did this. •

log_type

on

logProcessDuration

only records

result

or

status

. Anything else is counted as

other

. Security / operational impact • Database schema:

AutoMigrate

adds

admin_users.oidc_subject

and

admin_users.saml_subject

on startup. The change is additive. • Federated login rollout: • Existing federated users are bound on their next login. A takeover that happens before that first login is still possible. This is the accepted migration path. • Changing the IdP issuer will reject bound users. There is no reset command yet; clear the binding column for affected users and they bind again on next login. • Users who sign in over both OIDC and SAML get one binding per protocol. • One-liners: if an environment stores only a CA certificate and not the server's own certificate, the one-liners now fail instead of running without verification. • CLI:

osctrl-cli --db

node delete matches

--env

by environment name. • Duplicate UUIDs across environments: these are still allowed. Per-UUID side rows are kept while any environment still has the UUID. Validation • Full

go test ./...

and

go vet

on the changed packages pass. • New regression tests: •

TestFederatedLoginBindsSubjectNotUsername

and `TestFederatedLoginBindsLegacyRowOnFirstLogin`: a different subject reusing a bound username, the same subject from a different issuer, an IdP rename, binding an existing user on first login, and separate OIDC and SAML bindings. • `TestEnrollDoesNotTouchSameUUIDInOtherEnv`: re-enrolling and deleting in B leaves A's row unchanged. • `TestPrepareOneLinerPinsEnvironmentCertificate`: shell pin, PowerShell hash, explicit insecure opt-in, and an error for an unparseable certificate. • `TestInitCarveScopedToNode`: a carve init from another node is rejected; init only touches the node's own row; re-init is rejected. • `TestPrometheusMiddlewareLabelsByRoutePattern`: 50 distinct

{env}

paths add at most one label set, and

log_type

is limited. • Manually checked that

curl -sk --pinnedpubkey

(curl 8.7.1) runs the script from the pinned server and gets nothing from a server with a different certificate. • Not tested: the PowerShell callback was never run (no

pwsh

available); it is only checked as generated text. The SAML

Issuer

wiring has no end-to-end test. jmpsec/osctrl