diff --git a/.dockerignore b/.dockerignore index 98f84c0e..88486895 100644 --- a/.dockerignore +++ b/.dockerignore @@ -32,6 +32,8 @@ betterdesk-server/_test_* # Archive (old/deprecated files) archive/ + + # Documentation (not needed in image) docs/ screenshots/ diff --git a/.github/SECURITY_AUDIT_2026-03-01.md b/.github/SECURITY_AUDIT_2026-03-01.md deleted file mode 100644 index 77c5dd1f..00000000 --- a/.github/SECURITY_AUDIT_2026-03-01.md +++ /dev/null @@ -1,676 +0,0 @@ -# BetterDesk Security Audit Report - -**Date:** 2026-03-01 -**Auditor:** GitHub Copilot (Claude Opus 4.5) -**Scope:** `betterdesk-server/` (Go), `web-nodejs/` (Node.js) - ---- - -## Executive Summary - -This audit identified **24 security findings** across the BetterDesk project: -- **Critical:** 2 ✅ FIXED -- **High:** 5 ✅ FIXED -- **Medium:** 10 ✅ 6 FIXED, 4 LOW RISK (accepted) -- **Low:** 7 (tracked for future work) - -Many security best practices are already in place (CSRF protection, session fixation prevention, timing-safe auth, rate limiting, SQL parameterization). The findings below represent remaining gaps or areas for improvement. - ---- - -## Remediation Status (2026-03-01) - -| ID | Severity | Description | Status | -|----|----------|-------------|--------| -| C-1 | Critical | Command Injection in pingHost() | ✅ Fixed: spawn() with args array + host validation | -| C-2 | Critical | Password Logging in main.go | ✅ Fixed: Write to secure file with 0600 perms | -| H-1 | High | SQL LIKE Injection | ✅ Fixed: escapeLikePattern() + ESCAPE clause | -| H-2 | High | Error Leakage in Go API | ✅ Fixed: writeInternalError() helper | -| H-3 | High | Error Leakage in Token Handlers | ✅ Fixed: Generic error messages + logging | -| H-4 | High | Path Traversal in i18n | ✅ Fixed: isValidLangCode() validation | -| H-5 | High | MD5 for Hash | ✅ Fixed: SHA256 | -| M-1 | Medium | XSS via innerHTML | ✅ Fixed: Utils.escapeHtml() + SVG sanitization | -| M-2 | Medium | parseInt NaN validation | ✅ Fixed: isNaN() checks added | -| M-3 | Medium | Weak Random in generateId | ✅ Fixed: crypto.randomUUID() | -| M-4 | Medium | Cookie httpOnly: false | ✅ Documented: Intentional for client-side i18n | -| M-5 | Medium | Trust Proxy Default | ✅ Fixed: Default changed to false | -| M-6 | Medium | Missing Content-Type Check | ✅ Fixed: requireJsonContentType middleware | - ---- - -## Critical Findings - -### C-1: Command Injection in Network Monitor (CRITICAL) - -**File:** [web-nodejs/services/networkMonitor.js](../web-nodejs/services/networkMonitor.js#L60-L64) -**Severity:** Critical -**Description:** The `pingHost` function passes user-controllable `host` parameter directly to shell command without sanitization. - -**Code:** -```javascript -// Line 60-62 -const cmd = isWin - ? `ping -n 1 -w ${timeoutMs} ${host}` - : `ping -c 1 -W ${timeoutSec} ${host}`; - -const start = Date.now(); -exec(cmd, { timeout: timeoutMs + 2000 }, (err, stdout) => { -``` - -**Impact:** An attacker who can control the `host` parameter can execute arbitrary system commands (e.g., `; rm -rf /` or `& calc.exe`). - -**Recommended Fix:** -```javascript -// Validate hostname/IP format before use -const validHostRegex = /^[a-zA-Z0-9][a-zA-Z0-9.-]{0,253}[a-zA-Z0-9]$/; -if (!validHostRegex.test(host) && !net.isIP(host)) { - return resolve({ success: false, rtt_ms: null, error: 'Invalid host format' }); -} -// Use spawn() with array arguments instead of exec() -const { spawn } = require('child_process'); -const args = isWin ? ['-n', '1', '-w', String(timeoutMs), host] : ['-c', '1', '-W', String(timeoutSec), host]; -const proc = spawn('ping', args); -``` - ---- - -### C-2: Initial Admin Password Logged to Console (CRITICAL) - -**File:** [betterdesk-server/main.go](../betterdesk-server/main.go#L165) -**Severity:** Critical -**Description:** When a random admin password is generated, it is printed to logs in plaintext. - -**Code:** -```go -// Line 163-165 -if cfg.InitAdminPass == "" { - log.Printf(" Password: %s", adminPass) -} else { -``` - -**Impact:** The password may be visible in: -- Docker logs (`docker logs`) -- systemd journal (`journalctl`) -- Log files if stdout is redirected -- CI/CD build logs - -**Recommended Fix:** -```go -// Write password to a secure file with restricted permissions instead -if cfg.InitAdminPass == "" { - passFile := filepath.Join(cfg.DataDir, ".init_password") - os.WriteFile(passFile, []byte(adminPass), 0600) - log.Printf(" Password written to: %s (delete after reading)", passFile) - log.Printf(" (password not shown in logs for security)") -} else { - log.Printf(" Password: *** (user-provided, not logged)") -} -``` - ---- - -## High Findings - -### H-1: SQL LIKE Pattern Injection in dbAdapter.js (HIGH) - -**File:** [web-nodejs/services/dbAdapter.js](../web-nodejs/services/dbAdapter.js#L629) -**Severity:** High -**Description:** The `getAllPeers` function in the new dbAdapter does NOT escape `%` and `_` wildcards in the search parameter, unlike `database.js` which does. - -**Code:** -```javascript -// Line 629 - dbAdapter.js (NO escape) -if (filters.search) { where += ' AND (id LIKE ? OR note LIKE ? OR "user" LIKE ?)'; const s = `%${filters.search}%`; params.push(s, s, s); } - -// Compare with database.js (CORRECT - with escape) -// Line 435 - database.js -const escaped = escapeLikePattern(filters.search); -sql += " AND (id LIKE ? ESCAPE '\\' OR user LIKE ? ESCAPE '\\' OR note LIKE ? ESCAPE '\\')"; -``` - -**Impact:** User can inject `%` or `_` wildcards to match arbitrary patterns (information disclosure through pattern matching). - -**Recommended Fix:** -```javascript -if (filters.search) { - const escaped = filters.search.replace(/[%_\\]/g, '\\$&'); - where += " AND (id LIKE ? ESCAPE '\\' OR note LIKE ? ESCAPE '\\' OR \"user\" LIKE ? ESCAPE '\\')"; - const s = `%${escaped}%`; - params.push(s, s, s); -} -``` - -Also apply to lines: 903, 1510, 2096, 2297, 2889. - ---- - -### H-2: Error Message Information Leakage in Go API (HIGH) - -**File:** [betterdesk-server/api/server.go](../betterdesk-server/api/server.go#L327) -**Severity:** High -**Description:** Internal error messages are exposed to API clients via `err.Error()`. - -**Code:** -```go -// Multiple locations including line 327, 359, 380, 403, 423, 494, 519, 559, 721, 738 -writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) -``` - -**Impact:** Internal implementation details, database errors, file paths, and system information may leak to attackers. - -**Recommended Fix:** -```go -func handleDbError(w http.ResponseWriter, err error, action string) { - // Log full error internally - log.Printf("[api] %s error: %v", action, err) - // Return generic message to client - writeJSON(w, http.StatusInternalServerError, map[string]string{ - "error": "Internal server error", - }) -} -``` - ---- - -### H-3: Error Leakage in token_handlers.go (HIGH) - -**File:** [betterdesk-server/api/token_handlers.go](../betterdesk-server/api/token_handlers.go#L87) -**Severity:** High -**Description:** Database and system errors are directly exposed via `http.Error(w, err.Error(), ...)`. - -**Code:** -```go -// Lines 87, 150, 175, 199, 229, 249, 258, 343, 391, 409, 465 -http.Error(w, err.Error(), http.StatusInternalServerError) -``` - -**Impact:** Same as H-2 - information leakage. - -**Recommended Fix:** Same pattern as H-2. - ---- - -### H-4: Path Traversal Risk in i18n Language Upload (HIGH) - -**File:** [web-nodejs/routes/i18n.routes.js](../web-nodejs/routes/i18n.routes.js#L134-L145) -**Severity:** High -**Description:** Language code derived from uploaded filename or body is used in file path without full validation. - -**Code:** -```javascript -// Line 137-146 -const meta = translations._meta; -const code = meta?.code || req.body.code || req.file.originalname.replace('.json', ''); - -if (!code || code.length < 2 || code.length > 5) { - return res.status(400).json({ - success: false, - error: 'Invalid language code' - }); -} - -const result = manager.saveLanguage(code, translations); -``` - -**Impact:** An attacker could potentially submit `../../../etc/passwd` as code (though length check provides some protection). The `saveLanguage` function uses `path.join(config.langDir, ${code}.json)` which may still be vulnerable. - -**Recommended Fix:** -```javascript -// Strict validation: only allow alphanumeric + dash, 2-5 chars -if (!/^[a-z]{2}(-[A-Z]{2})?$/.test(code)) { - return res.status(400).json({ - success: false, - error: 'Invalid language code format (use: xx or xx-XX)' - }); -} -// Also validate in saveLanguage(): -const safeName = path.basename(code); // Extra protection -const filePath = path.join(config.langDir, `${safeName}.json`); -``` - ---- - -### H-5: MD5 Used for Sysinfo Hash (HIGH) - -**File:** [web-nodejs/routes/rustdesk-api.routes.js](../web-nodejs/routes/rustdesk-api.routes.js#L323) -**Severity:** High -**Description:** MD5 is used for creating content hashes, which is cryptographically weak. - -**Code:** -```javascript -// Line 323 -const hash = require('crypto').createHash('md5') - .update(JSON.stringify(sysinfo.raw_json)) - .digest('hex') - .substring(0, 16); -``` - -**Impact:** While used only for cache invalidation (not security), MD5 is deprecated and could lead to collisions. Using a deprecated algorithm in security-critical software sets a bad precedent. - -**Recommended Fix:** -```javascript -const hash = require('crypto').createHash('sha256') - .update(JSON.stringify(sysinfo.raw_json)) - .digest('hex') - .substring(0, 32); -``` - ---- - -## Medium Findings - -### M-1: XSS Risk via innerHTML in Frontend JS (MEDIUM) - -**File:** [web-nodejs/public/js/users.js](../web-nodejs/public/js/users.js#L65) -**Severity:** Medium -**Description:** User data is rendered via template literals and innerHTML without consistent escaping. - -**Code:** -```javascript -// Line 65 - users.js -tableBody.innerHTML = users.map(user => ` -