fix(configSync): require HTTPS for sync host to prevent MITM RCE#11228
Merged
Eugeny merged 1 commit intoMay 7, 2026
Merged
Conversation
Owner
|
Thank you! @all-contributors please add @sebastiondev for code |
Contributor
|
We had trouble processing your request. Please try again later. |
Contributor
Author
|
Appreciate the review. @lewiswigmore |
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
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
ConfigSyncService.request()intabby-settings/src/services/configSync.service.tsissues sync requests to whatever host the user has configured, including plainhttp://URLs. The response body is parsed as YAML and merged into the local Tabby configuration. Because Tabby profiles include fields likecommandandenvthat are later executed by the terminal, a network attacker able to tamper with the sync response can achieve arbitrary command execution on the victim's machine on the next sync.This is a CWE-295 / CWE-319 issue (improper certificate / cleartext transport for security-sensitive data). Severity is high in the http:// configuration scenario because the impact is local code execution, and the precondition (on-path network position) is realistic on untrusted networks (rogue Wi‑Fi, ARP/DNS spoofing on a LAN, hostile ISP, etc.).
Data flow
configSync.hosttohttp://...(the UI accepts arbitrary URLs).ConfigSyncService.request()concatenates the host with the API path and callsaxios.request({...}).getConfig()/ sync, the response body is YAML‑parsed and merged into the in‑processconfig.store.command⇒ RCE.There is no parallel HTTP entrypoint for this service; every API call funnels through
request(), so guarding it is sufficient.Fix
In
request(), before performing the axios call, validate that the configured host begins withhttps://(case‑insensitive). If not, log and throw. The check uses/^https:\/\//iso it correctly rejectshttp://,ftp://, scheme‑confusion (https-evil://), protocol‑relative URLs (//host), and leading‑whitespace tricks, while accepting any case ofhttps://.Axios on Node validates TLS certificates by default, so requiring
https://provides meaningful integrity and authenticity guarantees against an on‑path attacker.The diff is intentionally minimal (12 added lines, 1 changed) and confined to the single chokepoint.
What was tested
http://example.com,HTTP://example.com,ftp://example.com,https-evil://x,//example.com,https://example.com(leading whitespace), empty string.https://example.com,HTTPS://example.com,HtTpS://example.com.request()inconfigSync.service.ts(getConfigs,getConfig,setConfig,deleteConfig) — all flow through the guard.request()in this service.command/envare subsequently executed by the terminal subsystem (the sink that makes this exploitable rather than merely an information disclosure).Security analysis
The exploit chain is concrete: MITM the YAML response → inject a profile with a malicious
command→ user runs that profile (or, depending on how merged config is consumed, code path triggers automatically) → arbitrary code execution under the user's account. HTTPS with default certificate validation (which axios performs on Node) breaks the chain by preventing silent tampering and impersonation by an on‑path attacker.The trade‑off is that users who deliberately configured an
http://sync endpoint (e.g., a LAN service without TLS) will now see an explicit error instead of a silent insecure sync. That seems clearly correct: the sync payload is executable‑equivalent, and there is no safe way to fetch it over cleartext.Adversarial review
Before submitting, we tried to disprove this finding. The questions we considered were: (1) does axios or the framework already enforce HTTPS? — no, axios honours whatever scheme the URL specifies. (2) Does the YAML merge actually reach an executable sink, or is it inert configuration? — profile
command/envfields are spawned by the terminal layer, so the merged data is executable‑equivalent. (3) Is the precondition (on‑path network position) already equivalent to code execution, making the fix pointless? — no, MITM on a public/Wi‑Fi/LAN network does not by itself yield RCE on the victim host; that step is precisely what the cleartext sync grants the attacker. The finding holds.cc @lewiswigmore