788 lines
26 KiB
Markdown
788 lines
26 KiB
Markdown
# Security Audit Report
|
|
|
|
## 1. Executive Summary
|
|
|
|
This audit reviewed Nexus Terminal, a Wails desktop SSH/Telnet/RAW/VNC/SFTP client with a Go backend and JavaScript frontend. Fix mode was requested, so confirmed issues were patched, regression-tested, and prepared for commit.
|
|
|
|
The highest-risk issues were SSH trust failures: regular SSH sessions were not wired to the existing known-host verification flow, jump-host SSH connections ignored host keys, and VNC SSH tunnels also ignored host keys. These could allow machine-in-the-middle attacks against SSH credentials and terminal/VNC traffic. This run wired all SSH paths to known-host verification and prompt handling.
|
|
|
|
Other fixed issues include a local noVNC WebSocket proxy access-control weakness, SSH key-name path traversal in public-key reads, unsafe update-result DOM rendering, inline secret leakage in non-secret profile exports, and current vulnerable dependency versions in `golang.org/x/crypto` and `esbuild`.
|
|
|
|
Residual risk remains in user-selected insecure modes such as direct unencrypted VNC, user-configured `HostKeyInsecure`, and large local file transfer workflows that still process complete files in memory.
|
|
|
|
## 2. Scope Reviewed
|
|
|
|
Reviewed:
|
|
|
|
- Go backend packages under `internal/`: API bindings, sessions, SSH, VNC, SFTP, storage, secrets, updater, raw/telnet clients, recording, validation, known hosts, config, app startup.
|
|
- Frontend JavaScript under `frontend/src/js`: settings/update UI, host-key event handling, terminal and SFTP/VNC/GitHub surfaces, common DOM helpers.
|
|
- Dependency manifests and lockfiles: `go.mod`, `go.sum`, `frontend/package.json`, `frontend/package-lock.json`.
|
|
- Build and release scripts: `Makefile`, `build.sh`, `release.sh`, Wails config.
|
|
- Tests under `internal/**/*_test.go`.
|
|
|
|
Not in scope:
|
|
|
|
- Live network penetration testing of remote hosts.
|
|
- Reverse engineering packaged binaries.
|
|
- Manual verification of Wails runtime origin values in a built app.
|
|
|
|
## 3. Methodology
|
|
|
|
- Static source review of trust boundaries, file operations, SSH/VNC flows, Wails API bindings, frontend DOM sinks, update checks, storage, crypto, and secret handling.
|
|
- Dependency review with `govulncheck` and `npm audit`.
|
|
- Configuration review of Wails, build scripts, update manifests, and release paths.
|
|
- Threat modelling of desktop renderer compromise, local web-origin attacks, remote SSH/VNC attackers, and malicious update metadata.
|
|
- Denial-of-service review of network timeouts, WebSocket proxy behavior, transfer paths, and file ingestion.
|
|
- Regression tests added for fixed Go issues.
|
|
|
|
## 4. Risk Rating Method
|
|
|
|
Severity considered exploitability, attacker position, affected security property, and impact:
|
|
|
|
- Critical: practical remote compromise or credential disclosure without meaningful preconditions.
|
|
- High: credential/session compromise or machine-in-the-middle exposure on primary connection paths.
|
|
- Medium: meaningful local, renderer, or network-assisted exploit with constraints.
|
|
- Low: hardening issue, local DoS, or user-assisted exposure.
|
|
- Informational: checked area or non-security note.
|
|
|
|
Confidence reflects direct code evidence and verification coverage.
|
|
|
|
## 5. Findings Summary Table
|
|
|
|
| ID | Severity | Confidence | Title | Component | Status |
|
|
|---|---:|---:|---|---|---|
|
|
| SEC-001 | High | High | SSH sessions and jump hosts bypassed known-host verification | SSH sessions | Fixed |
|
|
| SEC-002 | High | High | VNC SSH tunnels accepted any SSH host key | VNC over SSH | Fixed |
|
|
| SEC-003 | Medium | High | Local noVNC WebSocket proxy used a guessable root endpoint | VNC WebSocket proxy | Fixed |
|
|
| SEC-004 | Medium | High | SSH public-key read accepted traversal key names | SSH key API | Fixed |
|
|
| SEC-005 | Medium | High | Update metadata rendered through dynamic HTML and inline handler | Settings/update UI | Fixed |
|
|
| SEC-006 | Medium | High | Non-secret profile export leaked inline SSH secrets | Profile export | Fixed |
|
|
| SEC-007 | High | High | `golang.org/x/crypto` vulnerable in reachable SSH code | Go dependencies | Fixed |
|
|
| SEC-008 | Medium | High | `esbuild` dev dependency matched current npm advisory | Frontend dependency | Fixed |
|
|
| SEC-009 | Low | Medium | Large local file upload paths remain memory-heavy | File transfer UX | Open |
|
|
| SEC-010 | Low | Medium | Direct VNC remains unencrypted by protocol choice | VNC direct mode | Open |
|
|
|
|
## 6. Detailed Findings
|
|
|
|
### Finding ID: SEC-001
|
|
|
|
#### Title
|
|
|
|
SSH sessions and jump hosts bypassed known-host verification
|
|
|
|
#### Severity
|
|
|
|
High
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `internal/sessions/manager.go`, SSH connection setup, now fixed at lines 657 and 1450-1502.
|
|
- `internal/sshclient/sshclient.go`, jump-host config, now fixed at lines 599 and 646-652.
|
|
- `frontend/src/js/app.js`, host-key response correlation, now fixed at lines 334-342.
|
|
|
|
#### Description
|
|
|
|
The project already contained known-host storage, a host-key verifier, and frontend prompt UI, but the main SSH session manager previously called the SSH client's default `Connect` path. That default path used `ssh.InsecureIgnoreHostKey()`. Jump hosts also built their own `ssh.ClientConfig` with `ssh.InsecureIgnoreHostKey()`.
|
|
|
|
#### Evidence
|
|
|
|
The fix now calls `ConnectWithHostKeyCallback(...)` for regular SSH sessions and propagates the final host-key callback into jump-host configs. A request ID is included in host-key prompt responses to prevent cross-session prompt confusion.
|
|
|
|
#### Attack Scenario
|
|
|
|
An attacker on the network path presents a fake SSH server or jump host. Without host-key verification, Nexus Terminal would accept the attacker's key, allowing credential capture, terminal tampering, or traffic interception.
|
|
|
|
#### Impact
|
|
|
|
Possible SSH credential theft, terminal command tampering, and loss of server authenticity.
|
|
|
|
#### Recommendation
|
|
|
|
Always use the known-host verifier for every SSH hop. Unknown host keys should follow the configured profile policy: strict, ask, accept-new, or explicit insecure.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented: `Manager.hostKeyCallback` builds `sshclient.NewHostKeyVerifier(...)`, emits a prompt when needed, and `connectSSH` calls `ConnectWithHostKeyCallback(...)`.
|
|
|
|
#### Suggested Tests
|
|
|
|
Added regression coverage:
|
|
|
|
- `TestHostKeyCallbackAcceptNewStoresHost`
|
|
- `TestHostKeyCallbackStrictRejectsUnknownHost`
|
|
- `TestJumpHostClientConfigUsesFinalHostKeyCallback`
|
|
|
|
#### References
|
|
|
|
- CWE-295: Improper Certificate Validation, analogous host-key trust failure.
|
|
- OWASP ASVS 9.1.2: server certificate validation principles.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
Profiles may still explicitly choose `HostKeyInsecure`; that is a user-selected insecure mode and remains visible as residual risk.
|
|
|
|
### Finding ID: SEC-002
|
|
|
|
#### Title
|
|
|
|
VNC SSH tunnels accepted any SSH host key
|
|
|
|
#### Severity
|
|
|
|
High
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `internal/vncclient/vncclient.go`, VNC SSH config, now fixed at lines 49 and 89-95.
|
|
- `internal/sessions/manager.go`, VNC SSH callback wiring, now fixed at line 1016.
|
|
|
|
#### Description
|
|
|
|
VNC SSH tunnels established SSH connections with `ssh.InsecureIgnoreHostKey()`, allowing a network attacker to impersonate the SSH tunnel endpoint.
|
|
|
|
#### Evidence
|
|
|
|
The VNC client now requires `SSHHostKeyCallback` for tunneled SSH. The session manager supplies the same known-host/prompt path used by SSH sessions.
|
|
|
|
#### Attack Scenario
|
|
|
|
An attacker controlling DNS, Wi-Fi, or a gateway presents a fake SSH tunnel host for VNC traffic. The prior client accepted the key and tunneled VNC traffic through the attacker.
|
|
|
|
#### Impact
|
|
|
|
Possible VNC credential exposure, remote desktop traffic interception, and remote interaction tampering.
|
|
|
|
#### Recommendation
|
|
|
|
Require known-host verification for VNC SSH tunnels and reject tunnel setup if no callback is configured.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented: `vncclient.Connect` returns an error if `cfg.SSHHostKeyCallback` is nil, and uses the provided callback in `ssh.ClientConfig`.
|
|
|
|
#### Suggested Tests
|
|
|
|
Covered indirectly by session host-key callback tests and VNC package tests.
|
|
|
|
#### References
|
|
|
|
- CWE-295.
|
|
- OWASP ASVS 9.1.2.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
VNC tunnel host-key policy defaults to ask mode because VNC profiles do not currently expose a separate host-key policy field.
|
|
|
|
### Finding ID: SEC-003
|
|
|
|
#### Title
|
|
|
|
Local noVNC WebSocket proxy used a guessable root endpoint
|
|
|
|
#### Severity
|
|
|
|
Medium
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `internal/vncclient/vncclient.go`, WebSocket proxy setup, now fixed at lines 171-183 and 255-260.
|
|
|
|
#### Description
|
|
|
|
The local VNC WebSocket proxy listened on `127.0.0.1` with a random port, but accepted upgrades on `/` and allowed any Origin. A malicious local web page could scan localhost ports and attempt to connect to an active proxy.
|
|
|
|
#### Evidence
|
|
|
|
The proxy now generates a 128-bit random token and exposes only `ws://127.0.0.1:<port>/<token>` to the Wails frontend.
|
|
|
|
#### Attack Scenario
|
|
|
|
A user visits a malicious web page while a VNC session is active. The page scans localhost WebSocket ports and connects to the proxy endpoint, potentially reading or injecting VNC traffic.
|
|
|
|
#### Impact
|
|
|
|
Possible local cross-origin access to VNC session traffic if the port is discovered.
|
|
|
|
#### Recommendation
|
|
|
|
Require an unguessable per-session URL token and continue binding only to loopback.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented: `newProxyToken()` uses `crypto/rand` and the proxy route is registered only at the token path.
|
|
|
|
#### Suggested Tests
|
|
|
|
Added `TestNewProxyTokenIsRandomHex`.
|
|
|
|
#### References
|
|
|
|
- CWE-346: Origin Validation Error.
|
|
- OWASP ASVS 14.4.3: cross-origin resource restrictions.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
`CheckOrigin` still returns true because Wails/noVNC origin behavior needs built-app verification. The unguessable path materially reduces exposure.
|
|
|
|
### Finding ID: SEC-004
|
|
|
|
#### Title
|
|
|
|
SSH public-key read accepted traversal key names
|
|
|
|
#### Severity
|
|
|
|
Medium
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `internal/api/api.go`, SSH key API, now fixed at lines 1653-1708.
|
|
|
|
#### Description
|
|
|
|
`GetSSHPublicKey(name)` previously built `~/.ssh/<name>.pub` directly from a renderer-provided name. Unlike key generation, it did not restrict names to simple key basenames. Names such as `../somefile` could read `~/.ssh/../somefile.pub`.
|
|
|
|
#### Evidence
|
|
|
|
The fix centralizes key name validation through `sshKeyPairPaths(...)`, allowing only alphanumeric characters, hyphens, and underscores.
|
|
|
|
#### Attack Scenario
|
|
|
|
If renderer-side JavaScript is compromised or a malicious plugin/flow can invoke the Wails API, it can request public-key contents outside the intended `.ssh` key namespace if the target path ends in `.pub`.
|
|
|
|
#### Impact
|
|
|
|
Local file disclosure for matching `.pub` paths under paths reachable through traversal.
|
|
|
|
#### Recommendation
|
|
|
|
Treat key names as basenames, not paths. Apply the same validation to generate, read, and delete operations.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented: `GetSSHPublicKey`, `GenerateSSHKey`, and `DeleteSSHKey` now all use `sshKeyPairPaths`.
|
|
|
|
#### Suggested Tests
|
|
|
|
Added:
|
|
|
|
- `TestSSHKeyPairPathsAcceptsSafeName`
|
|
- `TestSSHKeyPairPathsRejectsTraversal`
|
|
- `TestSSHKeyPairPathsRejectsNestedPath`
|
|
|
|
#### References
|
|
|
|
- CWE-22: Path Traversal.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
The API still intentionally lists and reads legitimate SSH public keys in `~/.ssh`.
|
|
|
|
### Finding ID: SEC-005
|
|
|
|
#### Title
|
|
|
|
Update metadata rendered through dynamic HTML and inline handler
|
|
|
|
#### Severity
|
|
|
|
Medium
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `frontend/src/js/settings.js`, update-check UI, now fixed at lines 940-958.
|
|
|
|
#### Description
|
|
|
|
Update metadata from GitHub or a configured update manifest was inserted into `innerHTML`, including an inline `onclick` that interpolated `info.releaseURL`. A malicious or compromised update metadata source could inject script into the Wails renderer.
|
|
|
|
#### Evidence
|
|
|
|
The fix constructs DOM nodes, sets text via `textContent`, and attaches a click listener with `addEventListener`.
|
|
|
|
#### Attack Scenario
|
|
|
|
An attacker controlling a self-hosted update manifest returns a release URL containing a quote and JavaScript payload. The prior inline handler interpolation could execute renderer JavaScript.
|
|
|
|
#### Impact
|
|
|
|
Renderer code execution. In a Wails app, renderer execution can call exposed backend APIs, increasing impact.
|
|
|
|
#### Recommendation
|
|
|
|
Never render remote metadata through string-built HTML. Use `textContent` and event listeners.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented in `settings.js` with `createElement`, `document.createTextNode`, and `addEventListener`.
|
|
|
|
#### Suggested Tests
|
|
|
|
No frontend unit harness exists. Verified with `npm run build`.
|
|
|
|
#### References
|
|
|
|
- CWE-79: Cross-Site Scripting.
|
|
- OWASP ASVS 5.3: output encoding.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
Other static modal templates still use `innerHTML`; reviewed high-risk dynamic remote metadata paths first.
|
|
|
|
### Finding ID: SEC-006
|
|
|
|
#### Title
|
|
|
|
Non-secret profile export leaked inline SSH secrets
|
|
|
|
#### Severity
|
|
|
|
Medium
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `internal/api/api.go`, Wails export wrapper, now fixed at lines 683-684.
|
|
- `internal/storage/store.go`, storage export, now fixed at lines 356-358 and 726-738.
|
|
|
|
#### Description
|
|
|
|
The plain profile export path accepted `includeSecrets=false` but profiles can still contain inline `ssh.proxyPassword` and `ssh.totpSecret` fields. Those fields could be serialized despite a non-secret export request.
|
|
|
|
#### Evidence
|
|
|
|
The fix redacts `ProxyPassword` and `TOTPSecret` for non-secret exports in both API and storage layers.
|
|
|
|
#### Attack Scenario
|
|
|
|
A user creates a plain profile export believing secrets are excluded. The exported JSON contains proxy credentials or TOTP secrets that are then shared, backed up, or stored insecurely.
|
|
|
|
#### Impact
|
|
|
|
Credential leakage and potential 2FA bypass for affected profiles.
|
|
|
|
#### Recommendation
|
|
|
|
Redact inline secret fields whenever `includeSecrets=false`. Prefer encrypted export for any export that intentionally includes secrets.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented `redactProfileSecrets(...)` in storage and API packages.
|
|
|
|
#### Suggested Tests
|
|
|
|
Added:
|
|
|
|
- `TestExportProfilesRedactsInlineSSHSecrets`
|
|
- `TestRedactProfileSecretsClearsInlineSSHSecrets`
|
|
|
|
#### References
|
|
|
|
- CWE-200: Exposure of Sensitive Information.
|
|
- OWASP ASVS 8.3: sensitive data protection.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
The encrypted export path intentionally includes selected profile data and is password-protected.
|
|
|
|
### Finding ID: SEC-007
|
|
|
|
#### Title
|
|
|
|
`golang.org/x/crypto` vulnerable in reachable SSH code
|
|
|
|
#### Severity
|
|
|
|
High
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `go.mod`, `golang.org/x/crypto`, now fixed at line 14.
|
|
- `go.mod`, indirect `golang.org/x/sys`, now updated at line 45.
|
|
- Reachable SSH code in `internal/sshclient`, `internal/vncclient`, `internal/sftpclient`.
|
|
|
|
#### Description
|
|
|
|
`govulncheck` reported seven reachable vulnerabilities in `golang.org/x/crypto@v0.51.0`, including SSH DoS/panic issues, FIDO/U2F security-key interaction bypass risk, and SSH agent forwarding constraint handling issues.
|
|
|
|
#### Evidence
|
|
|
|
`govulncheck` identified GO-2026-5033, GO-2026-5020, GO-2026-5019, GO-2026-5018, GO-2026-5017, GO-2026-5013, and GO-2026-5006 as reachable before the update. The dependency is now `v0.52.0`, and rerun output reports: "No vulnerabilities found. Your code is affected by 0 vulnerabilities."
|
|
|
|
#### Attack Scenario
|
|
|
|
A malicious or compromised SSH server, SSH agent, key material, or protocol peer triggers vulnerable SSH library behavior during connection, channel use, key parsing, or agent forwarding.
|
|
|
|
#### Impact
|
|
|
|
Possible client panic, denial of service, weakened security-key interaction, or agent forwarding constraint mishandling depending on the CVE path.
|
|
|
|
#### Recommendation
|
|
|
|
Keep `golang.org/x/crypto` at or above `v0.52.0` and run `govulncheck` in CI.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented: `go get golang.org/x/crypto@v0.52.0`.
|
|
|
|
#### Suggested Tests
|
|
|
|
Ran `govulncheck` and `go test ./...`.
|
|
|
|
#### References
|
|
|
|
- https://pkg.go.dev/vuln/GO-2026-5033
|
|
- https://pkg.go.dev/vuln/GO-2026-5020
|
|
- https://pkg.go.dev/vuln/GO-2026-5019
|
|
- https://pkg.go.dev/vuln/GO-2026-5018
|
|
- https://pkg.go.dev/vuln/GO-2026-5017
|
|
- https://pkg.go.dev/vuln/GO-2026-5013
|
|
- https://pkg.go.dev/vuln/GO-2026-5006
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
`govulncheck` also noted vulnerabilities in required modules that are not called by this code.
|
|
|
|
### Finding ID: SEC-008
|
|
|
|
#### Title
|
|
|
|
`esbuild` dev dependency matched current npm advisory
|
|
|
|
#### Severity
|
|
|
|
Medium
|
|
|
|
#### Confidence
|
|
|
|
High
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `frontend/package.json`, `esbuild`, now fixed at line 22.
|
|
- `frontend/package-lock.json`.
|
|
|
|
#### Description
|
|
|
|
`npm audit` reported GHSA-67mh-4wv8-2f99 for `esbuild <=0.24.2`: any website could send requests to the dev server and read responses under affected versions.
|
|
|
|
#### Evidence
|
|
|
|
The frontend previously resolved `esbuild@0.24.2`. It now uses `^0.28.0`, and `npm audit --json` reports zero vulnerabilities.
|
|
|
|
#### Attack Scenario
|
|
|
|
During local development, a malicious website could interact with the esbuild dev server and read responses if a developer runs an affected dev server.
|
|
|
|
#### Impact
|
|
|
|
Possible local development data exposure.
|
|
|
|
#### Recommendation
|
|
|
|
Use a fixed esbuild release and keep npm audit in CI.
|
|
|
|
#### Safer Example
|
|
|
|
Implemented: `npm install esbuild@0.28.0`.
|
|
|
|
#### Suggested Tests
|
|
|
|
Ran `npm audit --json` and `npm run build`.
|
|
|
|
#### References
|
|
|
|
- https://github.com/advisories/GHSA-67mh-4wv8-2f99
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
This is a devDependency, so production exploitability is lower than runtime dependencies.
|
|
|
|
### Finding ID: SEC-009
|
|
|
|
#### Title
|
|
|
|
Large local file upload paths remain memory-heavy
|
|
|
|
#### Severity
|
|
|
|
Low
|
|
|
|
#### Confidence
|
|
|
|
Medium
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `frontend/src/js/terminal.js`, dropped-file paste/upload flows.
|
|
- `internal/api/api.go`, `SFTPUploadData`.
|
|
|
|
#### Description
|
|
|
|
Some upload paths read whole files into memory and send base64 payloads to Go before uploading. This can be expensive for very large local files.
|
|
|
|
#### Evidence
|
|
|
|
`SFTPUploadData` decodes the entire base64 payload before calling session upload. The frontend drag/drop paste path also reads file contents into strings.
|
|
|
|
#### Attack Scenario
|
|
|
|
A local user or compromised renderer submits very large files, causing high memory use and potential application unresponsiveness.
|
|
|
|
#### Impact
|
|
|
|
Local denial of service.
|
|
|
|
#### Recommendation
|
|
|
|
Add size limits for paste-as-text, prefer path-based streaming upload for large files, and surface user-facing warnings for files above a threshold.
|
|
|
|
#### Safer Example
|
|
|
|
Use file path streaming upload where possible and reject in-memory uploads above a configured maximum.
|
|
|
|
#### Suggested Tests
|
|
|
|
Add boundary tests for maximum accepted in-memory upload size and UI tests for large-file warnings.
|
|
|
|
#### References
|
|
|
|
- CWE-400: Uncontrolled Resource Consumption.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
This is primarily local/user-assisted. It was left open to avoid changing UX and transfer semantics during the security fix pass.
|
|
|
|
### Finding ID: SEC-010
|
|
|
|
#### Title
|
|
|
|
Direct VNC remains unencrypted by protocol choice
|
|
|
|
#### Severity
|
|
|
|
Low
|
|
|
|
#### Confidence
|
|
|
|
Medium
|
|
|
|
#### Affected Files / Components
|
|
|
|
- `internal/vncclient/vncclient.go`, direct TCP VNC mode.
|
|
- VNC profile settings.
|
|
|
|
#### Description
|
|
|
|
Direct VNC connections use raw TCP to the VNC server. VNC is commonly unencrypted unless tunneled or separately protected.
|
|
|
|
#### Evidence
|
|
|
|
The direct branch calls `net.DialTimeout` to the VNC endpoint. SSH tunnel mode is available and now host-key verified.
|
|
|
|
#### Attack Scenario
|
|
|
|
On an untrusted network, direct VNC traffic may expose screen contents or credentials depending on server configuration.
|
|
|
|
#### Impact
|
|
|
|
Sensitive remote desktop data exposure.
|
|
|
|
#### Recommendation
|
|
|
|
Warn users before direct VNC connections and prefer SSH tunnel mode by default.
|
|
|
|
#### Safer Example
|
|
|
|
Default new VNC profiles to SSH tunnel where feasible, or show an explicit "unencrypted direct VNC" warning.
|
|
|
|
#### Suggested Tests
|
|
|
|
Add UI tests that direct VNC profiles show a warning before connection.
|
|
|
|
#### References
|
|
|
|
- OWASP ASVS 9.1: communications security.
|
|
|
|
#### Assumptions / Limitations
|
|
|
|
Some VNC servers may provide their own encryption; this audit did not test remote server configurations.
|
|
|
|
## 7. Denial-of-Service Review
|
|
|
|
Fixed:
|
|
|
|
- Updated `golang.org/x/crypto` to resolve reachable SSH panic/deadlock/DoS vulnerabilities.
|
|
- Added random-token access control to the local VNC WebSocket proxy, reducing local cross-origin abuse.
|
|
|
|
Reviewed and not changed:
|
|
|
|
- HTTP clients for GitHub and update checks use finite timeouts.
|
|
- SSH/Telnet/RAW connection setup uses timeouts.
|
|
- VNC WebSocket bridge is intentionally long-lived and uses no read/write timeout.
|
|
- Large in-memory file upload/paste paths remain a local DoS risk; see SEC-009.
|
|
|
|
## 8. Dependency and Supply-Chain Review
|
|
|
|
Checks run:
|
|
|
|
- `govulncheck`: fixed reachable `golang.org/x/crypto` vulnerabilities by updating to `v0.52.0`; rerun found zero reachable vulnerabilities.
|
|
- `npm audit --json`: fixed `esbuild` advisory by updating to `0.28.0`; rerun found zero vulnerabilities.
|
|
- `npm run build`: passed after dependency update.
|
|
|
|
Notes:
|
|
|
|
- `xterm` packages are deprecated upstream in favor of `@xterm/*`, but no current audit advisory was reported in this run.
|
|
- `go.mod` still has transitive modules with vulnerabilities that `govulncheck` reports as not called by this code.
|
|
|
|
## 9. Secrets and Sensitive Data Review
|
|
|
|
Fixed:
|
|
|
|
- Non-secret profile export now redacts inline `ProxyPassword` and `TOTPSecret`.
|
|
- SSH host-key verification now protects credential submission from basic MITM paths.
|
|
|
|
Reviewed:
|
|
|
|
- Vault uses Argon2id and AES-256-GCM with random salts/nonces.
|
|
- Vault file is written with `0600` permissions and store directories with `0700`.
|
|
- OS keychain is preferred where available.
|
|
|
|
Residual:
|
|
|
|
- Secret values still exist in process memory while sessions need them.
|
|
- Proxy password is still present in profile model state when entered inline.
|
|
|
|
## 10. Authentication and Authorisation Review
|
|
|
|
Fixed:
|
|
|
|
- SSH, jump host, and VNC-over-SSH authentication now has host authenticity checks before credential use.
|
|
- SSH host-key prompt responses are request-correlated.
|
|
|
|
Reviewed:
|
|
|
|
- SSH/TOTP flows use profile-specific secret callbacks.
|
|
- Agent forwarding is explicit profile configuration.
|
|
- Known-host policies include strict, ask, accept-new, and insecure.
|
|
|
|
Residual:
|
|
|
|
- `HostKeyInsecure` remains available for user-selected insecure connections.
|
|
|
|
## 11. Input Validation and Output Encoding Review
|
|
|
|
Fixed:
|
|
|
|
- SSH key names now reject traversal and nested path separators.
|
|
- Update metadata is rendered with DOM APIs instead of string-built HTML.
|
|
- Non-secret export redacts inline secret fields.
|
|
|
|
Reviewed:
|
|
|
|
- SQL operations use parameterized queries.
|
|
- Profile validation checks host, port, username, protocol-specific fields, and port-forwarding basics.
|
|
- Frontend frequently uses `textContent` or `escapeHtml`; remaining `innerHTML` static templates should still be reviewed during future frontend hardening.
|
|
|
|
## 12. Infrastructure and Deployment Review
|
|
|
|
Reviewed:
|
|
|
|
- Wails app binds Go services directly to the renderer. This makes frontend XSS more impactful than in a browser-only app.
|
|
- Build scripts use `set -euo pipefail`.
|
|
- Update checking is opt-in and does not auto-install binaries.
|
|
|
|
Residual:
|
|
|
|
- No CI configuration was present in the reviewed file list, so dependency scanning is not currently enforced by repository automation.
|
|
|
|
## 13. Logging, Monitoring, and Error Handling Review
|
|
|
|
Reviewed:
|
|
|
|
- Audit logger records profile create/update/delete, connection events, secret access, import/export.
|
|
- Errors are generally returned to the frontend with enough context for troubleshooting.
|
|
|
|
Residual:
|
|
|
|
- Some error messages include file paths, which is acceptable for a local desktop tool but should be considered if telemetry is later added.
|
|
|
|
## 14. Test Coverage and Security Regression Gaps
|
|
|
|
Added tests:
|
|
|
|
- API SSH key path validation and export redaction.
|
|
- Session host-key callback storage and strict rejection behavior.
|
|
- Jump-host host-key callback propagation.
|
|
- Storage export redaction.
|
|
- VNC proxy token generation.
|
|
|
|
Remaining gaps:
|
|
|
|
- No frontend unit test harness for DOM injection regressions.
|
|
- No integration test for full Wails host-key prompt round trip.
|
|
- No VNC WebSocket integration test verifying token-only connection behavior.
|
|
- No large-file upload boundary tests.
|
|
|
|
## 15. Prioritised Remediation Plan
|
|
|
|
Immediate:
|
|
|
|
- Keep the fixes in this audit and ensure they are merged.
|
|
- Add dependency scanning to CI: `govulncheck ./...` and `npm audit --audit-level=moderate`.
|
|
|
|
Short-term:
|
|
|
|
- Add a frontend test harness for update UI and other dynamic DOM surfaces.
|
|
- Add direct VNC warning UX and default to SSH tunnel where possible.
|
|
- Add explicit in-memory upload size limits.
|
|
|
|
Medium-term:
|
|
|
|
- Expose host-key policy for VNC SSH tunnels instead of always using ask mode.
|
|
- Review all Wails-exposed methods under a compromised-renderer threat model.
|
|
- Migrate deprecated `xterm` packages to maintained `@xterm/*` packages.
|
|
|
|
Long-term:
|
|
|
|
- Consider capability gating or confirmation for high-risk renderer-callable APIs such as file deletion, SSH key deletion, local shell selection, and profile import/export.
|
|
- Add signed update metadata if self-hosted update manifests become a production channel.
|
|
|
|
## 16. False Positives / Needs Manual Verification
|
|
|
|
- Wails/noVNC Origin behavior in a packaged app should be manually verified. Tokenized VNC WebSocket paths are in place, but Origin policy may still be tightened later.
|
|
- Direct VNC encryption depends on remote VNC server configuration and network placement.
|
|
- `HostKeyInsecure` is intentionally user-configurable; confirm UI wording makes the risk clear.
|
|
- Large-file memory usage thresholds need product decisions before enforcement.
|
|
|
|
## 17. Final Notes
|
|
|
|
Fix mode completed with code changes, tests, and dependency updates. The strongest residual concern is the general Wails threat model: a frontend XSS can call powerful local backend APIs. This audit fixed the confirmed dynamic update UI injection path and several backend trust-boundary issues, but future work should continue reducing string-built HTML and adding capability checks around sensitive local operations.
|