165 lines
8.0 KiB
Markdown
165 lines
8.0 KiB
Markdown
# Security Audit Report - Dealspace Go Codebase
|
|
**Date:** 2026-02-28
|
|
**Auditor:** James (automated security review)
|
|
**Scope:** All Go source files in lib/, api/, cmd/, migrations/
|
|
|
|
---
|
|
|
|
## Executive Summary
|
|
|
|
Security audit of the Dealspace M&A workflow platform codebase. Found **2 CRITICAL**, **2 HIGH**, **4 MEDIUM**, and **3 LOW/INFO** issues. Critical and high severity issues have been fixed in this commit.
|
|
|
|
### Findings by Severity
|
|
- **CRITICAL:** 2 (fixed)
|
|
- **HIGH:** 2 (fixed)
|
|
- **MEDIUM:** 4 (documented, recommended for next sprint)
|
|
- **LOW/INFO:** 3 (acceptable risk)
|
|
|
|
---
|
|
|
|
## CRITICAL Findings (Fixed)
|
|
|
|
### [CRITICAL] OTP Code Comparison Timing Attack
|
|
**Location:** lib/dbcore.go:605 (ChallengeVerify)
|
|
**Issue:** OTP code comparison used `c.Code != code` which is vulnerable to timing attacks. An attacker could measure response times to progressively guess the correct OTP, significantly reducing the brute-force space from 1M combinations.
|
|
**Fix:** Changed to `subtle.ConstantTimeCompare([]byte(c.Code), []byte(code)) != 1` which executes in constant time regardless of how many characters match.
|
|
**Status:** ✅ FIXED
|
|
|
|
### [CRITICAL] Backdoor Code Comparison Timing Attack
|
|
**Location:** api/handlers.go:327-333 (Verify endpoint)
|
|
**Issue:** Backdoor code comparison used `req.Code == h.Cfg.BackdoorCode` which is timing-vulnerable. Additionally, the original logic was confusing with redundant nested checks.
|
|
**Fix:** Simplified logic and changed to `subtle.ConstantTimeCompare([]byte(h.Cfg.BackdoorCode), []byte(req.Code)) == 1`. Also added `len(req.Code) > 0` check to prevent length leakage.
|
|
**Status:** ✅ FIXED
|
|
|
|
---
|
|
|
|
## HIGH Findings (Fixed)
|
|
|
|
### [HIGH] CORS Policy Wildcard
|
|
**Location:** api/middleware.go:135
|
|
**Issue:** CORS policy was `Access-Control-Allow-Origin: *` allowing any website to make authenticated cross-origin requests. This enables CSRF-like attacks if a user visits a malicious site while logged in.
|
|
**Fix:** Replaced with origin allowlist containing only trusted domains (muskepo.com, dealspace.io, localhost for development). Added `Vary: Origin` header for proper caching.
|
|
**Status:** ✅ FIXED
|
|
|
|
### [HIGH] Missing Security Headers
|
|
**Location:** api/middleware.go (missing)
|
|
**Issue:** No security headers were set on responses:
|
|
- No X-Frame-Options (clickjacking risk)
|
|
- No X-Content-Type-Options (MIME sniffing risk)
|
|
- No Content-Security-Policy (XSS risk)
|
|
**Fix:** Added `SecurityHeadersMiddleware` with:
|
|
- `X-Frame-Options: DENY`
|
|
- `X-Content-Type-Options: nosniff`
|
|
- `X-XSS-Protection: 1; mode=block`
|
|
- `Referrer-Policy: strict-origin-when-cross-origin`
|
|
- Restrictive CSP policy
|
|
**Status:** ✅ FIXED
|
|
|
|
---
|
|
|
|
## MEDIUM Findings (Documented)
|
|
|
|
### [MEDIUM] No File Type Validation on Upload
|
|
**Location:** api/handlers.go:640-675 (UploadObject)
|
|
**Issue:** Any file type can be uploaded. While files are served with `Content-Disposition: attachment` and `Content-Type: application/octet-stream`, this could be a vector for abuse (e.g., uploading malware, hosting phishing pages if served differently in future).
|
|
**Mitigation in place:** Files are served as downloads only, not rendered in browser.
|
|
**Recommendation:** Add allowlist of permitted MIME types (PDF, DOCX, XLSX, images). Validate magic bytes, not just extension.
|
|
|
|
### [MEDIUM] OTP Challenge Rate Limit Too Permissive
|
|
**Location:** api/routes.go:19
|
|
**Issue:** OTP challenge endpoint uses global rate limit of 120 req/min. This is too high for an OTP endpoint. At 120 req/min, an attacker could try all 1M OTP combinations in ~139 hours. With timing attack fixed, this is less critical but still concerning.
|
|
**Recommendation:** Add specific rate limit for `/api/auth/challenge` and `/api/auth/verify` (e.g., 5 requests per 10 minutes per email, 10 requests per IP per hour).
|
|
|
|
### [MEDIUM] Session Token Not Bound to IP/Fingerprint
|
|
**Location:** lib/dbcore.go (SessionCreate/SessionByID)
|
|
**Issue:** Sessions store fingerprint (User-Agent) but don't validate it on subsequent requests. A stolen session token can be used from any device/IP.
|
|
**Recommendation:** Add optional strict mode that validates User-Agent and/or IP hasn't changed dramatically since session creation.
|
|
|
|
### [MEDIUM] No Account Lockout After Failed OTP Attempts
|
|
**Location:** api/handlers.go (Verify endpoint)
|
|
**Issue:** Failed OTP verification attempts are not tracked or limited per user. An attacker can make unlimited OTP guesses for a specific email (limited only by global rate limit).
|
|
**Recommendation:** Implement exponential backoff or temporary lockout after 3-5 failed attempts per email address.
|
|
|
|
---
|
|
|
|
## LOW/INFO Findings
|
|
|
|
### [LOW] Audit Log Actions Not Encrypted
|
|
**Location:** migrations/001_initial.sql:99, lib/dbcore.go (AuditLog)
|
|
**Issue:** Audit log stores action and details as BLOB (encrypted) but `action` field is currently stored as plaintext in the handlers. This could leak operation types.
|
|
**Recommendation:** Ensure audit entries use Pack() for all sensitive fields.
|
|
|
|
### [INFO] HSTS Not Set by Application
|
|
**Location:** api/middleware.go
|
|
**Issue:** Application doesn't set HSTS header.
|
|
**Status:** Acceptable - documented that Caddy handles HSTS at the proxy layer. Verify Caddy config includes `Strict-Transport-Security: max-age=31536000; includeSubDomains`.
|
|
|
|
### [INFO] Empty Responses on Challenge Don't Reveal User Existence
|
|
**Location:** api/handlers.go:265-273
|
|
**Issue:** N/A - This is correctly implemented. Challenge endpoint returns same response whether email exists or not, preventing user enumeration.
|
|
**Status:** ✅ GOOD PRACTICE
|
|
|
|
---
|
|
|
|
## Positive Security Findings
|
|
|
|
The codebase demonstrates good security practices in several areas:
|
|
|
|
1. **Cryptography:**
|
|
- ✅ AES-256-GCM for encryption (FIPS 140-3 compliant)
|
|
- ✅ HKDF-SHA256 for key derivation with proper context separation
|
|
- ✅ Fresh random nonce for each encryption (crypto/rand)
|
|
- ✅ No hardcoded keys (MASTER_KEY from environment)
|
|
- ✅ Per-project encryption keys (compromise isolation)
|
|
|
|
2. **SQL Injection:**
|
|
- ✅ All queries use parameterized statements
|
|
- ✅ No string concatenation in SQL queries
|
|
|
|
3. **Authentication:**
|
|
- ✅ Session tokens generated with crypto/rand (32 bytes, 256-bit)
|
|
- ✅ JWT signature comparison uses hmac.Equal (constant-time)
|
|
- ✅ Passwordless auth reduces password-related attack surface
|
|
|
|
4. **Authorization:**
|
|
- ✅ RBAC consistently enforced via CheckAccess* functions
|
|
- ✅ Project access is explicit (no implicit org-based access)
|
|
- ✅ super_admin bypass is correctly implemented
|
|
|
|
5. **File Storage:**
|
|
- ✅ Content-addressable storage (SHA-256 hash as filename)
|
|
- ✅ No path traversal risk (filenames are hashes)
|
|
- ✅ Files encrypted at rest with project-specific keys
|
|
|
|
6. **Watermarking:**
|
|
- ✅ Applied at serve time (cannot be bypassed)
|
|
- ✅ Tied to authenticated user identity
|
|
- ✅ Original files stored clean (can re-watermark)
|
|
|
|
7. **Chat/AI Endpoint:**
|
|
- ✅ Specific rate limit (20 req/IP/hour)
|
|
- ✅ Zero retention noted for AI provider (Fireworks)
|
|
|
|
---
|
|
|
|
## Recommendations for Future Work
|
|
|
|
1. **Add File Type Validation** - Implement MIME type allowlist with magic byte verification
|
|
2. **Implement Per-Email Rate Limiting** - Add specific rate limits for auth endpoints
|
|
3. **Add Failed Attempt Tracking** - Implement account lockout after failed OTP attempts
|
|
4. **Security Headers Tuning** - Review CSP policy after deployment to ensure no breakage
|
|
5. **Penetration Testing** - Schedule professional pentest before production launch
|
|
6. **Dependency Audit** - Run `go mod audit` periodically for known vulnerabilities
|
|
|
|
---
|
|
|
|
## Changes Made in This Commit
|
|
|
|
1. `lib/dbcore.go` - Added crypto/subtle import, changed OTP comparison to constant-time
|
|
2. `api/handlers.go` - Added crypto/subtle import, fixed backdoor code comparison
|
|
3. `api/middleware.go` - Replaced wildcard CORS with origin allowlist, added SecurityHeadersMiddleware
|
|
4. `api/routes.go` - Added SecurityHeadersMiddleware to middleware chain
|
|
5. `api/middleware_test.go` - Updated CORS test for new allowlist behavior
|
|
|
|
All code compiles and passes updated tests.
|