GUACAMOLE-2305: Do not reflect sensitive parameter values streamed via argv.#1232
Open
ciroiriarte wants to merge 1 commit into
Open
GUACAMOLE-2305: Do not reflect sensitive parameter values streamed via argv.#1232ciroiriarte wants to merge 1 commit into
ciroiriarte wants to merge 1 commit into
Conversation
…a argv. When the server streams a connection parameter back to the client through an "argv" stream, ManagedClient reflects the received value into the UI-visible arguments model. For a sensitive parameter this surfaces the value in the connection-parameters panel, so a password or passphrase supplied by the connection configuration becomes readable by any user able to view the connection. Parameter names matching password, passphrase, private-key, client-key, secret or token are no longer reflected. Public material (host-key, public-key, pubkey) is deliberately not matched and remains visible, since it is not secret and is useful to display. Note that this addresses only the display path. Mutation of an immutable argument toward guacd is already rejected server-side, so this does not change what a user can send -- only what they can read back.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves GUACAMOLE-2305.
When the server streams a connection parameter back to the client through an
argvstream,
ManagedClientreflects the received value into the UI-visible argumentsmodel, which backs the connection parameters panel.
There is no filtering by parameter name, so when the streamed parameter holds a secret
— a password, a passphrase, a private key — the value becomes readable by any user
able to view that connection. This matters most where a connection is configured
centrally with credentials the user is not otherwise meant to see.
This is a display concern only. Mutating an immutable argument toward guacd is already
rejected server-side, so the issue is what a user can read, not what they can send.
The change: skip reflection for parameter names matching
password|passphrase|private-key|client-key|secret|token, case-insensitive. Publicmaterial (
host-key,public-key,pubkey) is deliberately not matched, since it isnot secret and is useful to display. One file, 20 lines added.
Two things I'd rather have decided by someone with more context, both noted on the
issue:
not be streaming sensitive values back at all, the durable fix belongs there. I
don't know whether any protocol relies on streaming these back.
tokencould match something that isn't secret, and aprotocol could name a sensitive parameter something the pattern misses. A
per-protocol flag in the protocol descriptor would be precise, at the cost of
touching every protocol definition. Happy to do that instead if preferred.
Found while auditing a SPICE protocol implementation built on Guacamole
(GUACAMOLE-261); the issue is not SPICE-specific.