Skip to content
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.

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

server: statement diagnostics download endpoint does not correctly validate user auth #99049

Closed
dhartunian opened this issue Mar 20, 2023 · 0 comments · Fixed by #99051
Closed
Assignees
Labels
A-observability-inf branch-release-21.2 Used to mark GA and release blockers, technical advisories, and bugs for 21.2 branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-technical-advisory Caused a technical advisory

Comments

@dhartunian
Copy link
Collaborator

dhartunian commented Mar 20, 2023

Describe the problem

The HTTP endpoint for downloading statement diagnostics bundles does not correctly validate the authenticated user's privileges. A user without admin privileges can download a bundle given the URL or an ID that they receive or guess.

To Reproduce

start a server.

  • create a non-privileged user e.g. create user foo with password abc
  • have root create a statement diagnostic bundle, e.g.: explain analyze (debug) select 'secret';
  • separately, have user foo get a login token from the HTTP API using their password
  • make the foo user access the stmt bundle endpoint using their token. For example:

Expected behavior
The user should receive an error when they attempt to download the bundle

Environment
CRDB v20.1 and later is affected (since this feature was added)

Additional Context
The code that implements the zip download endpoints is unconventional because it does not use gRPC for the file download and hence does not process authorization metadata in the same manner as code in other gRPC HTTP handlers.

Jira issue: CRDB-25680

@dhartunian dhartunian added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability-inf labels Mar 20, 2023
@dhartunian dhartunian self-assigned this Mar 20, 2023
dhartunian added a commit to dhartunian/cockroach that referenced this issue Mar 20, 2023
Previously, the statement diagnostics HTTP handler was initialized using the
incorrect `ctx` value. This caused the HTTP request context to not be correctly
handed down to the handler. Furthermore, the call to
`userFromIncomingRPCContext` was incorrect in this scenario since it relied on
a gRPC context being populated with HTTP session information, which did not
exist. That code sets a `root` user when context is missing because the gRPC
handlers are used for inter-node communication *and* HTTP with the DB console
and external tools.

This commit attaches the request context to the diagnostics bundle handler
correctly, and amends the authorization code to use
`userFromHTTPAuthInfoContext` which correctly reads the session cookie info
from the request (like many `api/v2` handlers do since those exist outside the
gRPC infrastructure).

Resolves cockroachdb#99049

Epic: None

Release note (security update): Previously, users could gain unauthorized
access to statement diagnostic bundles they did not create if they requested
the bundle through an HTTP request to `/_admin/v1/stmtbundle/<id>` and
correctly guessed its (non-secret) ID.  This change locks down this endpoint
behind the usual SQL gating that correctly uses the SQL user in the HTTP
session as identified by their cookie.
dhartunian added a commit to dhartunian/cockroach that referenced this issue Mar 21, 2023
Previously, the statement diagnostics HTTP handler was initialized using the
incorrect `ctx` value. This caused the HTTP request context to not be correctly
handed down to the handler. Furthermore, the call to
`userFromIncomingRPCContext` was incorrect in this scenario since it relied on
a gRPC context being populated with HTTP session information, which did not
exist. That code sets a `root` user when context is missing because the gRPC
handlers are used for inter-node communication *and* HTTP with the DB console
and external tools.

This commit attaches the request context to the diagnostics bundle handler
correctly, and amends the authorization code to use
`userFromHTTPAuthInfoContext` which correctly reads the session cookie info
from the request (like many `api/v2` handlers do since those exist outside the
gRPC infrastructure).

Resolves cockroachdb#99049

Epic: None

Release note (security update): Previously, users could gain unauthorized
access to statement diagnostic bundles they did not create if they requested
the bundle through an HTTP request to `/_admin/v1/stmtbundle/<id>` and
correctly guessed its (non-secret) ID.  This change locks down this endpoint
behind the usual SQL gating that correctly uses the SQL user in the HTTP
session as identified by their cookie.
craig bot pushed a commit that referenced this issue Mar 21, 2023
99051: admin: statement diagnostics uses correct auth helpers r=knz a=dhartunian

Previously, the statement diagnostics HTTP handler was initialized using the incorrect `ctx` value. This caused the HTTP request context to not be correctly handed down to the handler. Furthermore, the call to `userFromIncomingRPCContext` was incorrect in this scenario since it relied on a gRPC context being populated with HTTP session information, which did not exist. That code sets a `root` user when context is missing because the gRPC handlers are used for inter-node communication *and* HTTP with the DB console and external tools.

This commit attaches the request context to the diagnostics bundle handler correctly, and amends the authorization code to use `userFromHTTPAuthInfoContext` which correctly reads the session cookie info from the request (like many `api/v2` handlers do since those exist outside the gRPC infrastructure).

Resolves #99049

Epic: None

Release note (security update): Previously, users could gain unauthorized access to statement diagnostic bundles they did not create if they requested the bundle through an HTTP request to `/_admin/v1/stmtbundle/<id>` and correctly guessed its (non-secret) ID.  This change locks down this endpoint behind the usual SQL gating that correctly uses the SQL user in the HTTP session as identified by their cookie.

Co-authored-by: David Hartunian <[email protected]>
@craig craig bot closed this as completed in 0db60c5 Mar 21, 2023
@rytaft rytaft added C-technical-advisory Caused a technical advisory branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 branch-release-21.2 Used to mark GA and release blockers, technical advisories, and bugs for 21.2 labels Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability-inf branch-release-21.2 Used to mark GA and release blockers, technical advisories, and bugs for 21.2 branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-technical-advisory Caused a technical advisory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants