mirror of
https://github.com/UNITRONIX/BetterDesk.git
synced 2026-09-10 01:27:11 +00:00
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.
This commit is contained in:
@@ -80,11 +80,7 @@ pub async fn reconnect_agent(state: State<'_, AgentState>) -> Result<String, Str
|
||||
(config.server_address.clone(), config.device_id.clone())
|
||||
};
|
||||
|
||||
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, "/heartbeat");
|
||||
let payload = serde_json::json!({ "id": device_id });
|
||||
@@ -123,11 +119,7 @@ pub async fn send_diagnostics(state: State<'_, AgentState>) -> Result<String, St
|
||||
"disk": format!("{} MB", sysinfo.total_disk_mb),
|
||||
});
|
||||
|
||||
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, "/sysinfo");
|
||||
|
||||
@@ -303,11 +295,7 @@ pub async fn request_help(
|
||||
"timestamp": chrono::Utc::now().to_rfc3339(),
|
||||
});
|
||||
|
||||
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");
|
||||
|
||||
@@ -341,11 +329,7 @@ pub async fn cancel_help_request(state: State<'_, AgentState>) -> 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");
|
||||
|
||||
|
||||
@@ -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<Client> {
|
||||
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<String> {
|
||||
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<String> {
|
||||
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<String> {
|
||||
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<String> {
|
||||
"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?;
|
||||
|
||||
|
||||
@@ -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 == "" {
|
||||
|
||||
@@ -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<reqwest::Client, reqwest::Error> {
|
||||
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<ActivityEntry>,
|
||||
@@ -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<serde_json::Value> = Vec::new();
|
||||
let mut server_key = String::new();
|
||||
@@ -1104,11 +1132,7 @@ pub async fn org_login(
|
||||
username: String,
|
||||
password: String,
|
||||
) -> Result<serde_json::Value, String> {
|
||||
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!({
|
||||
|
||||
@@ -321,11 +321,16 @@ async fn collector_loop(
|
||||
status_tx: watch::Sender<InventoryStatus>,
|
||||
mut cancel_rx: watch::Receiver<bool>,
|
||||
) {
|
||||
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();
|
||||
|
||||
@@ -267,11 +267,16 @@ async fn bd_registration_loop(
|
||||
mut cancel_rx: watch::Receiver<bool>,
|
||||
incoming_tx: mpsc::Sender<PendingConnection>,
|
||||
) {
|
||||
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;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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. |
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user