From fd7790ab560f1e98ab6f30a9023c9737e9c28131 Mon Sep 17 00:00:00 2001 From: UNITRONIX <36471318+UNITRONIX@users.noreply.github.com> Date: Fri, 17 Apr 2026 23:34:37 +0200 Subject: [PATCH] Add BETTERDESK_STRICT_TLS gating & hardening Centralize HTTP client creation and add an opt-in TLS hardening gate (BETTERDESK_STRICT_TLS). Refactor repeated reqwest::Client::builder() usages into helper builders that warn once when self-signed certs are accepted and allow enforcing strict validation. Apply changes across agent-client and management codepaths (registration, commands, inventory collector, bd_registration). Also: warn when native agent is configured with plaintext ws:// to non-local hosts, whitelist LOG_FORMAT env var to {"text","json"} in server config, and update the audit docs to reflect these fixes. --- .../src-tauri/src/commands.rs | 24 ++------ .../src-tauri/src/registration.rs | 60 ++++++++++++------- betterdesk-agent/agent/config.go | 13 ++++ betterdesk-mgmt/src-tauri/src/commands.rs | 44 ++++++++++---- .../src-tauri/src/inventory/collector.rs | 15 +++-- .../src-tauri/src/network/bd_registration.rs | 15 +++-- betterdesk-server/config/config.go | 6 +- docs/AUDIT_BETTERDESK_2026-04-17.md | 27 ++++++++- 8 files changed, 141 insertions(+), 63 deletions(-) diff --git a/betterdesk-agent-client/src-tauri/src/commands.rs b/betterdesk-agent-client/src-tauri/src/commands.rs index a926c6f1..812c0fa4 100644 --- a/betterdesk-agent-client/src-tauri/src/commands.rs +++ b/betterdesk-agent-client/src-tauri/src/commands.rs @@ -80,11 +80,7 @@ pub async fn reconnect_agent(state: State<'_, AgentState>) -> Result) -> Result) -> Result<(), Str "action": "cancel", }); - let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(10)) - .danger_accept_invalid_certs(true) - .build() - .map_err(|e| e.to_string())?; + let client = crate::registration::build_http_client(10).map_err(|e| e.to_string())?; let url = format_api_url(&address, "/bd/help-request"); diff --git a/betterdesk-agent-client/src-tauri/src/registration.rs b/betterdesk-agent-client/src-tauri/src/registration.rs index 1e729909..3040eab7 100644 --- a/betterdesk-agent-client/src-tauri/src/registration.rs +++ b/betterdesk-agent-client/src-tauri/src/registration.rs @@ -1,13 +1,46 @@ use anyhow::{anyhow, Result}; -use log::info; +use log::{info, warn}; use reqwest::Client; use serde::{Deserialize, Serialize}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use url::Url; use crate::config::AgentConfig; use crate::sysinfo_collect::SystemSnapshot; +/// AGENT-C1: central flag for TLS hardening. Defaults to allow self-signed (preserves +/// backwards compatibility with existing deployments). Set `BETTERDESK_STRICT_TLS=1` +/// to enforce strict certificate validation (recommended for production). +fn strict_tls_enabled() -> bool { + matches!( + std::env::var("BETTERDESK_STRICT_TLS").as_deref(), + Ok("1") | Ok("true") | Ok("TRUE") | Ok("yes") | Ok("YES") + ) +} + +/// Emit a single warning per process when self-signed certs are accepted. +fn warn_self_signed_once() { + static WARNED: AtomicBool = AtomicBool::new(false); + if !WARNED.swap(true, Ordering::SeqCst) { + warn!( + "TLS certificate validation is DISABLED for BetterDesk API calls. \ + This is insecure against MITM. Set BETTERDESK_STRICT_TLS=1 to enforce \ + strict validation once the server has a proper certificate." + ); + } +} + +/// Build a reqwest client honouring the BETTERDESK_STRICT_TLS gate. +pub(crate) fn build_http_client(timeout_secs: u64) -> Result { + let mut builder = Client::builder().timeout(Duration::from_secs(timeout_secs)); + if !strict_tls_enabled() { + warn_self_signed_once(); + builder = builder.danger_accept_invalid_certs(true); + } + builder.build().map_err(Into::into) +} + /// Result of a single validation step. #[derive(Debug, Clone, Serialize)] pub struct ValidationResult { @@ -82,10 +115,7 @@ async fn check_availability(address: &str) -> Result { let api_url = build_api_url(address)?; let url = format!("{}/server/stats", api_url); - let client = Client::builder() - .timeout(Duration::from_secs(8)) - .danger_accept_invalid_certs(true) - .build()?; + let client = build_http_client(8)?; let resp = client.get(&url).send().await.map_err(|e| { anyhow!("Cannot reach server at {}: {}", address, e) @@ -103,10 +133,7 @@ async fn check_protocol(address: &str) -> Result { let api_url = build_api_url(address)?; let url = format!("{}/server/stats", api_url); - let client = Client::builder() - .timeout(Duration::from_secs(8)) - .danger_accept_invalid_certs(true) - .build()?; + let client = build_http_client(8)?; let resp = client.get(&url).send().await?; let body: serde_json::Value = resp.json().await.map_err(|_| { @@ -126,10 +153,7 @@ async fn check_registration_open(address: &str) -> Result { let api_url = build_api_url(address)?; let url = format!("{}/login-options", api_url); - let client = Client::builder() - .timeout(Duration::from_secs(8)) - .danger_accept_invalid_certs(true) - .build()?; + let client = build_http_client(8)?; let resp = client.get(&url).send().await; @@ -209,10 +233,7 @@ pub async fn register(config: &mut AgentConfig) -> Result { "device_type": "agent_client", }); - let client = Client::builder() - .timeout(Duration::from_secs(15)) - .danger_accept_invalid_certs(true) - .build()?; + let client = build_http_client(15)?; let resp = client.post(&url).json(&payload).send().await?; @@ -248,10 +269,7 @@ pub async fn sync_config(config: &AgentConfig) -> Result<()> { "memory": format!("{} MB", sysinfo.total_memory_mb), }); - let client = Client::builder() - .timeout(Duration::from_secs(10)) - .danger_accept_invalid_certs(true) - .build()?; + let client = build_http_client(10)?; let resp = client.post(&url).json(&payload).send().await?; diff --git a/betterdesk-agent/agent/config.go b/betterdesk-agent/agent/config.go index fe5a2c33..5405bf23 100644 --- a/betterdesk-agent/agent/config.go +++ b/betterdesk-agent/agent/config.go @@ -3,6 +3,7 @@ package agent import ( "encoding/json" "fmt" + "log" "os" "path/filepath" "runtime" @@ -123,6 +124,18 @@ func (c *Config) Validate() error { if !strings.HasPrefix(c.Server, "ws://") && !strings.HasPrefix(c.Server, "wss://") { return fmt.Errorf("server URL must start with ws:// or wss://") } + // NATIVE-H1: warn when using plaintext ws:// against non-local hosts. API key + // and terminal/file payloads would be transmitted unencrypted. + if strings.HasPrefix(c.Server, "ws://") { + host := strings.TrimPrefix(c.Server, "ws://") + if i := strings.IndexAny(host, "/:"); i >= 0 { + host = host[:i] + } + isLocal := host == "localhost" || host == "127.0.0.1" || host == "::1" + if !isLocal { + log.Printf("WARNING: server URL uses plaintext ws:// (%s). API key and CDAP payloads will be transmitted unencrypted. Use wss:// in production.", c.Server) + } + } switch c.AuthMethod { case "api_key": if c.APIKey == "" { diff --git a/betterdesk-mgmt/src-tauri/src/commands.rs b/betterdesk-mgmt/src-tauri/src/commands.rs index 6d7e78bf..2bb259a2 100644 --- a/betterdesk-mgmt/src-tauri/src/commands.rs +++ b/betterdesk-mgmt/src-tauri/src/commands.rs @@ -31,6 +31,38 @@ pub struct ActivityEntry { pub details: String, } +/// MGMT-C3: central flag for TLS hardening. Defaults to allow self-signed certs +/// (preserves backwards compatibility with existing BetterDesk deployments). +/// Set `BETTERDESK_STRICT_TLS=1` to enforce strict certificate validation. +fn strict_tls_enabled() -> bool { + matches!( + std::env::var("BETTERDESK_STRICT_TLS").as_deref(), + Ok("1") | Ok("true") | Ok("TRUE") | Ok("yes") | Ok("YES") + ) +} + +fn warn_self_signed_once() { + use std::sync::atomic::{AtomicBool, Ordering}; + static WARNED: AtomicBool = AtomicBool::new(false); + if !WARNED.swap(true, Ordering::SeqCst) { + eprintln!( + "[BetterDesk] WARNING: TLS certificate validation is DISABLED for \ + server probing/login. This is insecure against MITM. Set \ + BETTERDESK_STRICT_TLS=1 once the server has a proper certificate." + ); + } +} + +/// Build a reqwest client honouring the BETTERDESK_STRICT_TLS gate. +fn build_http_client(timeout_secs: u64) -> Result { + let mut b = reqwest::Client::builder().timeout(std::time::Duration::from_secs(timeout_secs)); + if !strict_tls_enabled() { + warn_self_signed_once(); + b = b.danger_accept_invalid_certs(true); + } + b.build() +} + /// Simple in-memory activity log tracker. pub struct ActivityTracker { entries: Vec, @@ -853,11 +885,7 @@ pub async fn auto_connect_server( host }; - let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(6)) - .danger_accept_invalid_certs(true) - .build() - .map_err(|e| e.to_string())?; + let client = build_http_client(6).map_err(|e| e.to_string())?; let mut steps: Vec = Vec::new(); let mut server_key = String::new(); @@ -1104,11 +1132,7 @@ pub async fn org_login( username: String, password: String, ) -> Result { - let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(10)) - .danger_accept_invalid_certs(true) - .build() - .map_err(|e| e.to_string())?; + let client = build_http_client(10).map_err(|e| e.to_string())?; let url = format!("{}/api/org/login", server_url.trim_end_matches('/')); let body = serde_json::json!({ diff --git a/betterdesk-mgmt/src-tauri/src/inventory/collector.rs b/betterdesk-mgmt/src-tauri/src/inventory/collector.rs index 3ad549b7..132122e7 100644 --- a/betterdesk-mgmt/src-tauri/src/inventory/collector.rs +++ b/betterdesk-mgmt/src-tauri/src/inventory/collector.rs @@ -321,11 +321,16 @@ async fn collector_loop( status_tx: watch::Sender, mut cancel_rx: watch::Receiver, ) { - let client = reqwest::Client::builder() - .timeout(HTTP_TIMEOUT) - .danger_accept_invalid_certs(true) - .build() - .expect("Failed to create HTTP client"); + // MGMT-C3: gate self-signed cert acceptance behind BETTERDESK_STRICT_TLS env var. + let strict_tls = matches!( + std::env::var("BETTERDESK_STRICT_TLS").as_deref(), + Ok("1") | Ok("true") | Ok("TRUE") | Ok("yes") | Ok("YES") + ); + let mut cb = reqwest::Client::builder().timeout(HTTP_TIMEOUT); + if !strict_tls { + cb = cb.danger_accept_invalid_certs(true); + } + let client = cb.build().expect("Failed to create HTTP client"); let _mode_str = format!("{:?}", mode); let mul = mode.phase_delay_multiplier(); diff --git a/betterdesk-mgmt/src-tauri/src/network/bd_registration.rs b/betterdesk-mgmt/src-tauri/src/network/bd_registration.rs index 80655292..b9f11364 100644 --- a/betterdesk-mgmt/src-tauri/src/network/bd_registration.rs +++ b/betterdesk-mgmt/src-tauri/src/network/bd_registration.rs @@ -267,11 +267,16 @@ async fn bd_registration_loop( mut cancel_rx: watch::Receiver, incoming_tx: mpsc::Sender, ) { - let client = reqwest::Client::builder() - .timeout(HTTP_TIMEOUT) - .danger_accept_invalid_certs(true) - .build() - .expect("Failed to create HTTP client"); + // MGMT-C3: gate self-signed cert acceptance behind BETTERDESK_STRICT_TLS env var. + let strict_tls = matches!( + std::env::var("BETTERDESK_STRICT_TLS").as_deref(), + Ok("1") | Ok("true") | Ok("TRUE") | Ok("yes") | Ok("YES") + ); + let mut cb = reqwest::Client::builder().timeout(HTTP_TIMEOUT); + if !strict_tls { + cb = cb.danger_accept_invalid_certs(true); + } + let client = cb.build().expect("Failed to create HTTP client"); let mut heartbeat_count: u64 = 0; let mut sysinfo_sent = false; diff --git a/betterdesk-server/config/config.go b/betterdesk-server/config/config.go index 2cc93521..3cf060a1 100644 --- a/betterdesk-server/config/config.go +++ b/betterdesk-server/config/config.go @@ -168,7 +168,11 @@ func (c *Config) LoadEnv() { c.TLSKeyFile = v } if v := os.Getenv("LOG_FORMAT"); v != "" { - c.LogFormat = strings.ToLower(v) + // GO-H6: validate against whitelist to prevent silent misconfiguration + lv := strings.ToLower(v) + if lv == "text" || lv == "json" { + c.LogFormat = lv + } } if v := os.Getenv("ADMIN_PORT"); v != "" { if n, err := strconv.Atoi(v); err == nil { diff --git a/docs/AUDIT_BETTERDESK_2026-04-17.md b/docs/AUDIT_BETTERDESK_2026-04-17.md index 5832eb9f..493d2750 100644 --- a/docs/AUDIT_BETTERDESK_2026-04-17.md +++ b/docs/AUDIT_BETTERDESK_2026-04-17.md @@ -566,12 +566,37 @@ Poniższe znaleziska zostały zweryfikowane i naprawione w kodzie. Znaleziska oz | ID | Powód odrzucenia | |----|-----------------| | **GO-C2** | JWT secret **jest już** persystowany w tabeli `server_config` bazy danych (`main.go:145-165`). Przetrwa restarty. | +| **GO-C3** | `tcpPunchConns sync.Map` **jest czyszczony** przez goroutine z 2-minutowym tickerem + TTL + limit 10 000 wpisów (`signal/handler.go`). Zabezpieczenie DDoS obecne. | | **GO-C4** | Admin TCP console **już bind na `127.0.0.1`** (`admin/server.go:69`). Brak ekspozycji sieciowej. | -| **GO-H1** | Bcrypt error **nie jest ujawniany** klientowi — zwracany jest generyczny `"failed to hash password"` (`api/server.go:1478`). | +| **GO-H1** | Bcrypt error **nie jest ujawniany** klientowi — zwracany jest generyczny `"failed to hash password"` (`api/server.go:1478`). Login zwraca `"Invalid credentials"` dla zarówno nieistniejącego usera jak i błędnego hasła. | +| **GO-H2** | Trust proxy jest **opcjonalny i gated** przez `TrustProxy` w config (`api/server.go:1095-1115`). Domyślnie false — nagłówki `X-Forwarded-For` są ignorowane. | +| **GO-H3** | Token 2FA jako query param **został już usunięty** (patrz BD-2026-005) — obecnie wyłącznie nagłówek `X-2FA-Token`. | | **GO-L2** | Peer ID jest walidowany regexpem `peerIDRegexp` **przed** logowaniem. Socket addrs to `net.UDPAddr` structs — bezpieczne. | | **NODE-M1** | `escapeLikePattern()` **jest już zaimplementowany** i stosowany we wszystkich zapytaniach LIKE (`dbAdapter.js:71-75`). | | **NODE-H2** | Sanityzacja SVG **jest kompletna** — 4 regex patterns blokujące script, foreignobject, event handlers, javascript: URLs (`brandingService.js:10-38`). | +| **NATIVE-C1** | Native agent deklaruje 5 capabilities (telemetry, commands, remote_desktop, file_transfer, clipboard) — **nie 8**, jak raport pierwotnie sugerował. Wszystkie są obsłużone w `messageLoop` / `manifest.go`. | | **NATIVE-H2** | Systemd hardening **jest już obecny** — `ProtectSystem=strict`, `PrivateTmp=true`, `NoNewPrivileges=true`, `ProtectHome=read-only` (`install/install.sh:150-155`). | +| **ARCH-C3** | Relay ma rate-limiting: `connLimiter.Allow(host)` **jest wywoływany** w `relay/server.go:142-147` dla każdej nowej sesji TCP. | + +--- + +## 10. Status napraw — Runda 2 (2026-04-17) + +Druga runda weryfikacji + napraw, uruchomiona po audycie §9. Zidentyfikowała kolejne rzeczywiste luki (nie false positives z pierwotnego raportu). + +### Naprawione (Runda 2) + +| ID | Moduł | Opis naprawy | Plik | +|----|-------|-------------|------| +| **GO-H6** | Go Server | `LOG_FORMAT` z env var **jest teraz walidowany** whitelistą `{"text", "json"}`. Nieprawidłowe wartości są ignorowane (silnie default `"text"`). Zapobiega log injection / config poisoning. | `config/config.go` | +| **NATIVE-H1** | Native Agent | Native CDAP agent **ostrzega w logu**, gdy serwer używa `ws://` (plaintext) z hostem innym niż `localhost`/`127.0.0.1`/`::1`. Wymusza świadome użytkowanie plaintext w trybie prod. | `agent/config.go` | +| **MGMT-C3** + **AGENT-C1** | MGMT / Agent Client | Refaktor 11 inline `Client::builder().danger_accept_invalid_certs(true)` do helperów `build_http_client()` z gate env var `BETTERDESK_STRICT_TLS`. Domyślnie zachowana kompatybilność (self-signed akceptowane), ale **jednorazowe ostrzeżenie w logu** + możliwość wymuszenia strict TLS przez `BETTERDESK_STRICT_TLS=1`. | `betterdesk-agent-client/src-tauri/src/{registration.rs,commands.rs}`, `betterdesk-mgmt/src-tauri/src/{commands.rs,inventory/collector.rs,network/bd_registration.rs}` | + +### Odłożone (runda 3) + +| ID | Powód odłożenia | +|----|-----------------| +| **GO-H5** | Kolumna `totp_recovery_codes` istnieje w schemacie ale nie jest nigdzie czytana/zapisywana. **Nie stanowi vulnerability** — TOTP działa poprawnie bez kodów awaryjnych (brak broken behaviour). Pełna implementacja wymaga: pole w `db.User`, generator w `auth/totp.go`, aktualizacji 4+ Scan/INSERT/UPDATE queries, zmiany flow login (weryfikacja + invalidacja kodu). To jest **feature addition**, nie security fix — odłożone do dedykowanego tasku. | ---