Repository navigation
Fix issues from security review - #66
Merged
Merged
Conversation
A host certificate signed by a trusted CA was accepted for any hostname matching the CA's hostname patterns, regardless of the certificate's principals. Any host with a valid certificate could impersonate the others. Require the hostname to be one of the certificate's principals. Like OpenSSH, a certificate with no principals is valid for any host. Only offer to trust the CA when the certificate is otherwise valid. Also add SECURITY-TODO.md to track the issues from the security review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Any output containing a ZMODEM signature (e.g. cat of a log file) started a receive session, saving files with attacker-chosen names to the Downloads folder, and typed protocol replies into the remote session's input. Similarly, the upload signature opened the file picker unprompted. Ask the user to confirm each transfer session before replying to the remote side. On decline, cancel the transfer and pass the remaining output through to the terminal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The progress percentage divided by the file size, which comes from the server or the browser. An empty file, or a server reporting a size of 0 for a file with content (e.g. files in /proc), caused an integer divide by zero. When this happened in the stream's pull goroutine, or in the drag-and-drop upload goroutine, the panic wasn't recovered and the whole app exited. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Content-Disposition header contained the raw file name. Header values must be ASCII, so the service worker threw when building the response, and the download, and the SFTP or ZMODEM session waiting for it, hung forever. Encode the file name with mime.FormatMediaType, which uses RFC 2231 encoding when needed. Also make the service worker cancel the stream if the response can't be built, so the download fails instead of hanging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SSH banners, host certificate fields, SFTP file names, symlink targets, the SFTP working directory, ZMODEM file names, and error messages were written to the terminal as is. A malicious server could use escape sequences to spoof the local prompts and messages. Add the termsafe package, which replaces control characters and bidi overrides, and use it for all the text that comes from the remote side. Like OpenSSH, this means that banners are displayed without colors. ZMODEM and SFTP download names are sanitized as well, and SFTP tab completion skips names that can't be displayed safely. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With agent forwarding, the remote host could request signatures without the user knowing about it. For WebAuthn keys, the browser's security key prompt looked the same as for the user's own logins. The remote host could also lock the agent, brute force the lock passphrase, or remove keys. Forward a restricted view of the agent that only allows listing keys and signing, and print a message for each signature request. Also, don't hold the agent's lock while signing, which could take up to 2 minutes with WebAuthn keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CSP is set with a meta tag, where frame-ancestors isn't supported, so another site could embed SSH Term in a frame for clickjacking. Refuse to start when loaded in a frame, and document the HTTP headers that deployments should set when they can. Also tighten the CSP: use 'wasm-unsafe-eval' instead of 'unsafe-eval', since only WASM compilation needs it, and add base-uri 'none' and form-action 'none'. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
xterm.js wraps pasted text in bracketed paste markers when the remote application enables bracketed paste mode, but it doesn't remove an end marker (ESC [201~) embedded in the text. Text copied from a malicious web page could end bracketed paste mode early and run commands in the remote shell. Remove escape characters from pasted text, for both the right/middle click handlers and the regular paste event. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Crafted files could crash or hang the app: - A WebAuthn key file without a valid PEM block caused a nil dereference. - A WebAuthn public key with an invalid curve point caused a panic later. - The PBKDF2 iteration count in an encrypted WebAuthn key wasn't bounded, so a huge value hung the tab. - A backup file shorter than 40 bytes caused an out of range slice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An invalid endpoint URL made the WebSocket constructor throw, which surfaced as a Go panic. A text message from the proxy made the message handler call arrayBuffer() on a string, and the panic in the JS callback made the whole app exit. Return an error when the WebSocket can't be created, receive messages as ArrayBuffers, and close the connection with an error when a message of another type is received. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the service worker wasn't available, ZMODEM downloads were buffered in memory with no limit, and a remote server could exhaust the memory. Refuse the download instead, like sftp get, and cancel the transfer on the remote side when a receive fails, instead of dumping the rest of the transfer to the terminal. Also reuse the app's stream helper instead of creating a new one for each file. Each new helper replaced the service worker message handler, which broke subsequent sftp downloads. ZMODEM transfers now also honor the test upload and download hooks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Backups were encrypted with a key derived with 50,000 iterations of PBKDF2-SHA256 by default, and --iter accepted any value down to 0. Use 600,000 iterations by default, as recommended by OWASP, and require at least 100,000. Existing backups can still be restored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The value of the __tlsproxySid cookie was sent as the x-csrf-token header to any identity provider URL. It is only meant for the proxy that serves the app, so only send it when the identity provider has the same origin. Today the CSP already blocks requests to other origins, but this shouldn't depend on it. Also anchor the cookie name in the regular expression, so that a cookie like x__tlsproxySid doesn't match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The keys listed in generateKeys in config.json were re-generated every time the app started, i.e. on every page load and in every new tab. For resident keys, this created a new credential on the security key each time, silently replacing the previous one, and required a touch. Only generate the keys that don't already exist in the database. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Only accept a stream's response from the client that requested it. Before, any same-origin client could answer any pending request, with any headers. - Only intercept same-origin requests whose path ends with /stream/<id>. Before, /stream/ could appear anywhere in the URL, e.g. in the query. - Use a Map for the pending streams instead of a plain object. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All the issues from the security review are fixed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Host certificates must now list the hostname as a principal, and the hostname was always the endpoint's name. Endpoints named with a short alias, e.g. myserver for myserver.example.com, failed validation. Add an optional hostname to endpoints, with ep add --hostname or "hostname" in config.json, which is used to validate host certificates instead of the name. The authority's hostname patterns can match either one. When a host certificate is signed by a trusted authority and is otherwise valid, but doesn't list the endpoint's hostname, offer to use one of the certificate's principals as the endpoint's hostname. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Xuy5tAMuqwfjBaDy7H6cL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… through a stack trace' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Like the service worker, log the error to the console and return a generic 500 response when the stream can't be sent. The error is still returned to the caller of Download. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Xuy5tAMuqwfjBaDy7H6cL
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
2 participants
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.
Summary
This PR fixes the issues found in a security review of the codebase. The most
important one is that host certificates were trusted for any hostname that the
signing CA was trusted for, regardless of the certificate's principals, so any
host with a valid certificate could impersonate the others.
Each fix is in its own commit, with tests. See CHANGELOG.md for the
user-facing release notes.
Changes users will notice
certificate's principals. Like OpenSSH, a certificate with no principals is
valid for any host.
ep add --hostname ..., or"hostname"inconfig.json) for when the endpoint's name isn't theserver's hostname.
hostname, the prompt offers to use one of its principals as the endpoint's
hostname. Existing setups can migrate with one prompt.
(e.g.
catof a file) could save files to Downloads, or open the file picker.ZMODEM downloads now require the service worker, like
sftp get.ssh -A, each signature requestis shown in the terminal. The remote host can only list keys and sign; it
can no longer lock, unlock, or remove keys.
certificate details, SFTP and ZMODEM file names, and error messages. As in
OpenSSH, banners lose their colors.
bracketed paste mode early and run commands.
headers to set when the server can.
'wasm-unsafe-eval'instead of'unsafe-eval', and addsbase-uriandform-action. The minimum browsers are now Chrome 97,Firefox 102 and Safari 16.
Old backups can still be restored.
generateKeyskeeps existing keys. Before, keys were re-generated onevery page load. For resident keys, that also replaced the credential on the
security key.
Other fixes
on the same origin, and the cookie match is anchored.
requested it, and URLs are matched on the path only.
sftp getafter a ZMODEM download. A failed ZMODEM receive cancels thetransfer instead of dumping it to the terminal.
Testing
run against the old code to confirm they fail.
termsafepackage, host certificate principals,and ZMODEM consent, sanitization, and cancellation.
./tests/run-headless-tests.sh,go test ./..., andgo vet/go fmtfor wasm all pass.
Notes for reviewers
(
myserver.example.com,foo,bar,baz), and it supports agentforwarding (
agent-test) and sending text WebSocket messages (?text=true).WebAuthn key parsing fixes are tested through
keys import.🤖 Generated with Claude Code