Kyle Isom
c5469c6bdf
Single authenticated user by design. If multi-user is ever needed, auth migrates to MCIAS — standalone auth won't be extended. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
188 lines
6.8 KiB
Markdown
188 lines
6.8 KiB
Markdown
# 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 |
|
|
|
|
## Design Note: Single-User Model
|
|
|
|
This server is designed for a single authenticated user. The users
|
|
table and multi-user schema exist but are used for one account only.
|
|
If multi-user support is ever needed, authentication will migrate to
|
|
MCIAS (Metacircular Identity and Access Service) — the standalone
|
|
auth implementation here will not be extended for multiple users.
|