[Osquery] Resolve saved query references before authorizing runSavedQueries - #287882
[Osquery] Resolve saved query references before authorizing runSavedQueries#287882tomsonpl wants to merge 27 commits into
Conversation
…ueries A truthy saved_query_id or pack_id was treated as authorization, so a runSavedQueries-only caller could dispatch arbitrary SQL. Resolve the reference in the request space and dispatch stored query content instead. Relates to elastic/security#11789 and elastic/security#12007 Co-authored-by: Cursor <cursoragent@cursor.com>
tomsonpl
added
release_note:breaking
Team:Defend Workflows
tomsonpl
added
backport:all-open
tomsonpl
added
Team:Defend Workflows
|
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
1 similar comment
|
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the osquery runSavedQueries authorization hardening. The core refactor is solid: references are resolved to real saved objects with blank/whitespace ids and cross-space references correctly failing to resolve, queries[] is always rejected for the runSavedQueries-only persona, mismatched query/ecs_mapping are rejected rather than silently ignored, the unresolvable-reference and unreadable-alert paths now return 403 instead of 500, and the rule-response-action validators forward query/queries/ecs_mapping into the same checker so rule-run dispatch can't be used to bypass the boundary. Test coverage across the unit, route, and Scout RBAC layers is thorough. Left one inline note about a residual gap in the parameterised-query matching.
Generated by Claude Reviewer for #287882 · opus · 200.2 AIC · ⌖ 14.6 AIC · ⊞ 5.1K
| response_actions: [smuggledOsqueryAction()], | ||
| }); | ||
| const ndjson = `${JSON.stringify(rule)}\n`; | ||
| const boundary = '----scoutFormBoundary'; |
There was a problem hiding this comment.
Contribute to Scout when possible
Hand-assembling multipart/form-data with a fixed boundary in a spec file is fragile and non-reusable. Consider promoting this to a helper (ideally on apiServices) so the next rule-import test doesn't rebuild it from scratch.
See details
Rules import is a first-class detection-engine endpoint with no apiServices helper today. Every future test that wants to exercise it will need this same block, and each copy is a chance to drift on:
- boundary escaping (one wrong
\r\nand the request silently posts empty NDJSON), elastic-api-version/x-elastic-internal-originheaders,- content-type formatting.
Two directions, roughly ordered by scope:
- Local helper in this suite's fixtures — a
buildRulesImportMultipart(rules)returning{ headers, body }. Cheap, keeps the spec readable. - Contribute to
apiServices— e.g.detectionEngine.rules.import(rules, { auth }). Best long-term home, matches how packs/savedQueries are already exposed to these specs.
Either way, the spec should just call the helper and assert on the response.
Share feedback in the #appex-qa Slack channel.
|
|
||
| ## 9.6.0 [kibana-9.6.0-breaking-changes] | ||
|
|
||
| $$$kibana-287882$$$ |
There was a problem hiding this comment.
I'm not sure if we should be publicizing these breaking changes before the stack version is released. A separate PR that did something similar was approved though, so...perhaps it's not as much of an issue as I think it is?
On serverless, is this a breaking change?
There was a problem hiding this comment.
Thanks, good point! Yes it's also a breaking change on serverless. However I am not sure what the proper process look like. Do you know who can we check this with?
There was a problem hiding this comment.
Sorry for missing this rely. I pinged @lcawl for more help, since the Serverless release notes follow a different generation process these days.
There was a problem hiding this comment.
I've sent a private message but the gist is that you open an issue for review by the breaking changes committee. If it's approved, you add the text to the ## Release note section of the PR description and only then add the release_note: breaking label. That suffices to ensure it's picked up in both the Serverless and Stack release notes.
🔍 Preview links for changed docs |
…iour Reverts 5ae6085. The alert-response-actions e2e asserts on the outgoing request body: expect(request.body.query).to.not.contain('{{host.os.name}}') That assertion was added deliberately in elastic#286175 ("Assert take-action param substitution on the dispatched request"), so the request body carrying the substituted SQL is intended behaviour, not an artefact. Moving substitution to the server necessarily puts the template in the request body, which conflicts with it. The dispatched query was correct either way -- `defaultValues.alertIds` already flows to the flyout, so the server did load the alert document and substitute -- but that is not what the test guards, and reworking an intentional assertion to suit this PR is out of scope for a privilege fix. Client-side substitution and its pre-existing TODO are restored verbatim. The server-side work from 0169fc1 (pack ecs_mapping scoping, logger hoist, applies_to widening) is unrelated to substitution and stays. Konrad's substitution finding therefore remains open, to be handled separately alongside the alert_ids refactor the TODO already describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // rule is being *removed*, and there is nothing to authorize. Without this, a saved query | ||
| // that is later deleted or moved out of the space would pin the action in place forever, | ||
| // because the user could neither keep it (403) nor take it off the rule. | ||
| if (!isActionInPayload(actionData)) { |
There was a problem hiding this comment.
Consequence of the removal carve-out: the bulk-duplicate route validation is now a no-op.
bulk_actions/route.ts:344-352 calls validateRuleResponseActions with rulePayload: {}, so with this carve-out every existing osquery action is "being removed" and is skipped. The real duplicate is still denied — rulesClient.create at bulk_actions/route.ts:381 reaches alerting/.../create_rule.ts:167, where the duplicated params are the payload — which is why response_actions_rules.spec.ts:519-560 still passes. But the dry-run branch returns at route.ts:355-357, before create, so POST /api/detection_engine/rules/_bulk_action?dry_run=true with {action: 'duplicate'} now reports success where it previously surfaced the 403. API-only today (the rules table calls duplicate without a dry run, use_bulk_actions.tsx:189-190), and nothing covers it.
The carve-out itself is right — the fix belongs at the call site: pass the actions that would be created (the duplicated rule's response_actions) instead of {}, so dry run and real run agree and "absent from the payload" keeps meaning "being removed". Fine to accept as-is if you'd rather keep the change minimal, but it should be a conscious call rather than a side effect.
szwarckonrad
left a comment
szwarckonrad
left a comment
There was a problem hiding this comment.
Approving. All four blockers from my earlier review are genuinely fixed in 2a50e79 / 0169fc1, and I verified each against c240429 rather than taking the replies on faith:
- Unchanged osquery actions now dedupe across the snake/camel boundary and a removal is never re-authorized, so saved-query drift no longer pins an action on a rule. Both halves are unit-tested, and every existing Scout denial spec still passes because each one puts the action in the payload.
- Rule runs record
SAVED_QUERY_NOT_FOUNDon the action document instead of throwing into a swallowed.catch(); the dispatched SQL is forced toundefined, so there is no fallback to caller-supplied content. - Pack substitution is gated on
useStoredQuery, so an ad-hocwriteLiveQueriespack run dispatches templates exactly as before. - The flyout no longer posts single-query fields in pack mode, and authorization no longer leaks saved-object errors as unhandled 500s.
Two items are left open deliberately, neither blocking:
- The bulk-duplicate route validation is now a no-op as a side effect of the removal carve-out — the real duplicate is still denied via
rulesClient.create, butdry_run=truereports success. Left as its own thread; fine either fixed at the call site or consciously accepted. - The client/server substitution divergence stays open with #289296 filed. Correct trade-off given it is a false deny and the proper fix collides with the intentional e2e assertion from #286175 — it just wants a known-limitation line in the release note.
The remaining asks are documentation-only (pack case in the new 400 paragraph, and confirming which branches carry the entry given backport:all-open while the block sits under a 9.6.0 heading). Nice work on the churn on the substitution thread — reverting twice and filing the issue was the right call over forcing a fix that broke someone else's assertion.
…rrors, docs
Bulk duplicate passed `rulePayload: {}` with `existingRule`, so the osquery
removal carve-out added in 2a50e79 treated every action as being removed and
skipped validation entirely. The dry run therefore reported success for a
duplicate that `rulesClient.create` goes on to deny. A duplicate persists the
source rule's response actions on a new rule, so it is now validated exactly
like a create: the actions are the payload and there is no `existingRule`,
matching both `create_rule/route.ts` and the alerting `authorize.params`
layer that enforces this downstream. Uses the existing
`transformAlertToRuleResponseAction` converter rather than a cast.
A deleted or cross-space pack referenced by a rule action threw out of
`createActionHandler`; the throw was swallowed by `processResponseActionClientError`
and the rule run still reported `succeeded` with no action document. Rule runs
now record `PACK_NOT_FOUND` on the action so the failure is visible in the
alert's Osquery Results tab, mirroring the saved-query path. A missing pack no
longer falls through to caller-supplied SQL. Direct API callers still get the
throw.
Test cleanup guards in response_actions_rules.spec.ts registered created rule
IDs after the 403 assertion, making them unreachable: a regression to 200 would
leak the rule. Registration now precedes the assertion.
Docs: drop the `:applies_to:` line that contradicted the 9.6.0 heading it sat
under (the entry is the only one in the file using it, and the backport gives
each branch its own correctly-headed copy); extend the 400 note to cover packs,
which hit the same PARAMETER_NOT_FOUND path; add the known-limitation note for
the deferred client/server substitution divergence (elastic#289296).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…client substitution
Follow-up to review feedback on the runSavedQueries authz change.
- Stop substituting `{{params}}` client-side in the live-query form. The client
saw the ECS-shaped alert while the server re-substituted from the flattened
technical fields, and authz required the two strings to be equal, so
parameterized saved queries 403'd for runSavedQueries-only users from the
alert flyout. Send the template and let the route resolve it (elastic#289296).
- Derive `ecs_mapping` from the saved object when stored SQL is dispatched, and
treat an empty caller mapping as unset. The rule-action form defaults it to
`{}`, so a stored mapping never reached the agent from a rule.
- Stop treating an absent/empty `ecs_mapping` as an assertion in authz. It made
an action whose mapping did not match the referenced object unmodifiable by a
runSavedQueries author, leaving removal as the only possible edit. `{}` still
cannot strip a stored mapping: dispatch now derives it, which holds regardless
of what the caller sends.
- Require pack `queries[]` entries to match a stored pack query. Dispatch
rebuilds the query set from the pack so there was no SQL escalation, but the
persisted copy decides whether a run fans out per alert with substitution.
- Decide parameterization from the resolved stored content rather than the copy
persisted on the rule. Editing a saved query to add a parameter needs only
writeSavedQueries, and the stale copy still looked static, so the run took the
non-parameterized branch and dispatched nothing.
- Rename the scout case that no longer reaches the investigation-guide recovery
branch; that branch stays covered in create_live_query_route.test.ts.
Verified: osquery 197 suites / 2493 tests, security_solution server/lib 57
suites / 869 tests, eslint clean on all touched files.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…micQueries params - Register duplicated rule ids before asserting in the bulk-duplicate authz test. Playwright's `expect` throws synchronously, so a regression to 200 — the exact escalation this test guards — skipped the cleanup extraction and leaked the duplicates into the next run on the shared server. Matches the ordering the create-time tests above already use. Audited the rest of the scout API suite: the other `push`-after-`expect` sites are all guarded by a 200 assertion, where a failure means nothing was created, so this was the only affected case. - Narrow `containsDynamicQueries` to the fields it actually reads instead of taking a full `CreateLiveQueryRequestBodySchema`. Callers pass a rule's persisted params, not a live-query payload, and requiring every pack-query field (id/ecs_mapping/version/platform/removed/snapshot) broke type_check. Verified: osquery 197 suites / 2493 tests, eslint clean, and the osquery project now type-checks with no errors from this PR (the remaining `plugin.ts` `isCpsActive` error predates it, from the CPSServerStart drift in merged main). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng it from the client
Corrects the earlier attempt at the alert-flyout 403. Removing the client-side
`{{param}}` substitution broke `alert_response_actions.cy.ts` — those tests
assert on `request.body.query`, i.e. the outgoing request before it reaches the
server, so no amount of server-side substitution can satisfy them. Restore
`live_queries/form/index.tsx` exactly as it was (it is now byte-identical to the
state before this branch touched it) and fix the actual defect instead.
Root cause: `replaceParamsQuery` reads fields with `lodash.get`, which already
resolves a dotted path against a literal dotted key, so the two document shapes
were not the problem. Flattened `ParsedTechnicalFields` stores every value as an
array, so the server substituted `name='["Ubuntu"]'` instead of `name='Ubuntu'`,
the `queriesMatch` string comparison failed, and the request was rejected 403.
Unwrap the array so both the nested ECS object the client holds and the flattened
alert document the server reads produce the same string.
Verified: osquery 197 suites / 2496 tests, security_solution rule_response_actions
38 tests, eslint clean, no new type_check errors. No `public/` or `cypress/` file
differs from the PR head, so the Cypress suite sees unchanged client code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@dhurley14 hey, can you take a look at this please? :) Thanks! |
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]Async chunks
Test FailuresHistory
|
Saved query and pack ids are unconstrained strings, so joining them with `::` let two distinct references collide on one cache entry: a reference with `pack_id: '::my-pack'` and one with `saved_query_id` ending in `::` produce the same key. The second caller then reused the first's resolved object. The cache is keyed by KibanaRequest, and rule import and bulk duplicate authorize many response actions under a single request, so the collision was reachable there. Because a resolved-undefined result is cached too, the practical effect was a spurious 403 on an unrelated rule in the same request. Key on a structured value instead so ids cannot straddle the separator.
7 participants
Summary
runSavedQueriesnow authorizes against a resolved saved query or pack in the request space, and live-query / rule-run dispatch uses that stored content. Callers who holdwriteLiveQueriesare unchanged.This is a breaking API change for clients that send a
query,queries, orecs_mappingthat does not match the referenced object, or a reference that does not resolve: those requests now return 403. A matchingqueryis still accepted so the Kibana UI (which posts the selected saved query's SQL) keeps working; it does not override what is dispatched. Parameterized saved queries continue to accept client-substituted SQL when it matches the stored template; substitution is also applied server-side when alert context is present.The same authorization applies when attaching an osquery response action to a detection rule (create, update, patch, import, bulk duplicate).
Key changes
saved_query_id/pack_idbefore authorizingrunSavedQueries(blank ids do not resolve).runSavedQuerieswithoutwriteLiveQueries, dispatch stored SQL and mapping rather than the request body.query/queries/ecs_mapping(snake and camel case) into the same checker.Test plan
rbac_t1_analyst,rbac_t2_analyst,rbac_alert_test,response_actions_rules)AI-Assisted: This PR was assisted by Cursor Grok 4.6.
Made with Cursor