Security hardening: fix critical, high, and medium issues from audit
CRITICAL: - A-001: SQL injection in snapshot — escape single quotes in backup path - A-002: Timing attack — always verify against dummy hash when user not found, preventing username enumeration - A-003: Notebook ownership — all authenticated endpoints now verify user_id before loading notebook data - A-004: Point data bounds — decodePoints returns error on misaligned data, >4MB payloads, and NaN/Inf values HIGH: - A-005: Error messages — generic errors in HTTP responses, no err.Error() - A-006: Share link authz — RevokeShareLink verifies notebook ownership - A-007: Scan errors — return 500 instead of silently continuing MEDIUM: - A-008: Web server TLS — optional TLS support (HTTPS when configured) - A-009: Input validation — page_size, stroke count, point_data alignment checked in SyncNotebook RPC - A-010: Graceful shutdown — 30s drain on SIGINT/SIGTERM, all servers shut down properly Added AUDIT.md with all 17 findings, status, and rationale for accepted risks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
179
AUDIT.md
Normal file
179
AUDIT.md
Normal file
@@ -0,0 +1,179 @@
|
||||
# AUDIT.md — eng-pad-server Security Audit
|
||||
|
||||
## Audit Date: 2026-03-24
|
||||
|
||||
### Summary
|
||||
|
||||
Comprehensive security and engineering review of the eng-pad-server
|
||||
codebase. 17 issues identified across critical, high, and medium
|
||||
severity levels. All critical and high issues resolved.
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### A-001: SQL Injection in Database Backup
|
||||
- **Severity**: Critical
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `cmd/eng-pad-server/snapshot.go`
|
||||
- **Description**: Backup path interpolated into SQL via `fmt.Sprintf`
|
||||
without escaping. Allows arbitrary SQL execution if path contains
|
||||
single quotes.
|
||||
- **Resolution**: Escape single quotes in the backup path before
|
||||
interpolation.
|
||||
|
||||
### A-002: Authentication Timing Attack
|
||||
- **Severity**: High
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `internal/auth/users.go`
|
||||
- **Description**: Early return when user not found skips Argon2id
|
||||
computation, allowing username enumeration via response timing.
|
||||
- **Resolution**: Always perform Argon2id verification against a dummy
|
||||
hash when user is not found, consuming constant time.
|
||||
|
||||
### A-003: Missing Notebook Ownership Checks
|
||||
- **Severity**: High
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `internal/server/notebooks_handler.go`
|
||||
- **Description**: Authenticated endpoints load notebooks by ID without
|
||||
verifying the notebook belongs to the requesting user. An
|
||||
authenticated user could access any notebook by guessing IDs.
|
||||
- **Resolution**: Added `user_id` check to all notebook queries in
|
||||
authenticated endpoints. Share link endpoints use the validated
|
||||
notebook ID from the share link.
|
||||
|
||||
### A-004: Unbounded Point Data Decoding
|
||||
- **Severity**: High
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `internal/render/svg.go`
|
||||
- **Description**: `decodePoints()` panics on data not aligned to 4
|
||||
bytes, has no size limit (OOM risk), and doesn't filter NaN/Inf.
|
||||
- **Resolution**: Added alignment check, 4MB size limit, NaN/Inf
|
||||
filtering. Function now returns error.
|
||||
|
||||
### A-005: Error Messages Leak Information
|
||||
- **Severity**: High
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: Multiple REST handlers
|
||||
- **Description**: `err.Error()` concatenated directly into HTTP JSON
|
||||
responses, leaking database structure and internal state.
|
||||
- **Resolution**: All error responses use generic messages. Detailed
|
||||
errors logged server-side only.
|
||||
|
||||
### A-006: No Authorization on Share Link Revocation
|
||||
- **Severity**: High
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `internal/grpcserver/share.go`
|
||||
- **Description**: Any authenticated user could revoke any share link
|
||||
by token, regardless of notebook ownership.
|
||||
- **Resolution**: Added JOIN with notebooks table to verify the calling
|
||||
user owns the notebook before allowing revocation.
|
||||
|
||||
### A-007: Silent Row Scan Errors
|
||||
- **Severity**: High
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: Multiple handlers
|
||||
- **Description**: `continue` on row scan errors silently returns
|
||||
incomplete data without indication.
|
||||
- **Resolution**: Scan errors now return 500 Internal Server Error in
|
||||
REST handlers.
|
||||
|
||||
### A-008: Web Server Missing TLS
|
||||
- **Severity**: Medium
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `internal/webserver/server.go`
|
||||
- **Description**: Web UI served over plain HTTP. Session cookies marked
|
||||
`Secure: true` are ineffective without TLS.
|
||||
- **Resolution**: Added TLS cert/key fields to web server config. Uses
|
||||
HTTPS when configured, falls back to HTTP for development.
|
||||
|
||||
### A-009: Missing Input Validation in Sync RPC
|
||||
- **Severity**: Medium
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `internal/grpcserver/sync.go`
|
||||
- **Description**: No validation of page_size, stroke count, or point
|
||||
data alignment in SyncNotebook RPC.
|
||||
- **Resolution**: Added validation: page_size must be REGULAR or LARGE,
|
||||
total strokes limited to 100,000, point_data must be 4-byte aligned.
|
||||
|
||||
### A-010: No Graceful Shutdown
|
||||
- **Severity**: Medium
|
||||
- **Status**: ~~Resolved~~
|
||||
- **Location**: `cmd/eng-pad-server/server.go`
|
||||
- **Description**: Signal handler terminates immediately without
|
||||
draining in-flight requests.
|
||||
- **Resolution**: Graceful shutdown with 30-second timeout: gRPC
|
||||
GracefulStop, HTTP Shutdown, database Close.
|
||||
|
||||
### A-011: Missing CSRF Protection on Web Forms
|
||||
- **Severity**: Medium
|
||||
- **Status**: Accepted
|
||||
- **Rationale**: Web UI is currently read-only (viewing synced
|
||||
notebooks). The only mutating form is login, which is not a CSRF
|
||||
target (attacker gains nothing by logging victim into their own
|
||||
account). Will add CSRF tokens when/if web UI gains write features.
|
||||
|
||||
### A-012: WebAuthn Sign Count Not Verified
|
||||
- **Severity**: Medium
|
||||
- **Status**: Accepted
|
||||
- **Rationale**: Sign count regression detection is defense-in-depth
|
||||
against cloned authenticators. Risk is low for a personal service.
|
||||
Will add verification when WebAuthn is fully wired into the web UI.
|
||||
|
||||
### A-013: gRPC Per-Request Password Auth
|
||||
- **Severity**: Medium (audit assessment) / Accepted (our assessment)
|
||||
- **Status**: Accepted
|
||||
- **Rationale**: By design. Password travels over TLS 1.3 (encrypted),
|
||||
stored in Android Keystore (hardware-backed). Sync is manual and
|
||||
infrequent. Token-based auth adds complexity without meaningful
|
||||
security gain for this use case.
|
||||
|
||||
### A-014: No Structured Logging
|
||||
- **Severity**: Medium
|
||||
- **Status**: Open
|
||||
- **Description**: Only `fmt.Printf` to stdout. No log levels, no
|
||||
structured output, no request tracking.
|
||||
- **Plan**: Add `log/slog` based logging in a future phase.
|
||||
|
||||
### A-015: Incomplete Config Validation
|
||||
- **Severity**: Medium
|
||||
- **Status**: Open
|
||||
- **Description**: TLS files not checked for existence at startup.
|
||||
Token TTL, WebAuthn config not validated.
|
||||
- **Plan**: Add file existence checks and config field validation.
|
||||
|
||||
### A-016: Inconsistent Error Types
|
||||
- **Severity**: Low
|
||||
- **Status**: Open
|
||||
- **Description**: String errors instead of sentinel errors make
|
||||
error handling difficult for callers.
|
||||
|
||||
### A-017: No Race Condition Testing
|
||||
- **Severity**: Low
|
||||
- **Status**: Open
|
||||
- **Description**: Test suite does not use `-race` flag.
|
||||
- **Plan**: Add `make test-race` target.
|
||||
|
||||
---
|
||||
|
||||
## Priority Summary
|
||||
|
||||
| ID | Severity | Status |
|
||||
|----|----------|--------|
|
||||
| A-001 | Critical | ~~Resolved~~ |
|
||||
| A-002 | High | ~~Resolved~~ |
|
||||
| A-003 | High | ~~Resolved~~ |
|
||||
| A-004 | High | ~~Resolved~~ |
|
||||
| A-005 | High | ~~Resolved~~ |
|
||||
| A-006 | High | ~~Resolved~~ |
|
||||
| A-007 | High | ~~Resolved~~ |
|
||||
| A-008 | Medium | ~~Resolved~~ |
|
||||
| A-009 | Medium | ~~Resolved~~ |
|
||||
| A-010 | Medium | ~~Resolved~~ |
|
||||
| A-011 | Medium | Accepted |
|
||||
| A-012 | Medium | Accepted |
|
||||
| A-013 | Medium | Accepted |
|
||||
| A-014 | Medium | Open |
|
||||
| A-015 | Medium | Open |
|
||||
| A-016 | Low | Open |
|
||||
| A-017 | Low | Open |
|
||||
Reference in New Issue
Block a user