fix: address aisafe.io security audit findings - #72
Merged
Conversation
Fix 15 security issues identified in the aisafe.io audit: Critical/High: - Safe UTF-8 byte slicing in subdomain extraction (RQS-015) - JWT type confusion: deny_unknown_fields on Claims (RQS-004) - Cache eviction O(N*M) loop: collect keys once outside loop (RQS-008) - Cross-tenant quota bypass: fix subdomain extraction for DNS keys (RQS-005) - Cache leak: recognize request: keys in extract_subdomain_from_key (RQS-009) - Request deletion index desync: scan list by ID instead (RQS-007) - DNS amplification: TXT record 512-char limit + UDP truncation bit (RQS-006) - TCP exhaustion: semaphore (100 conns) + 30s read timeout (RQS-013) - SMTP memory exhaustion: 64KB log cap, 500 cmd limit, 100 rcpt limit (RQS-003) - WebSocket memory burst: limit initial history to last 100 requests (RQS-014) - XSS mitigation: sandbox CSP + nosniff on served files (RQS-002) Low/Info: - Constant-time admin token comparison (RQS-001) - DNS per-IP rate limiting at 100 qps (RQS-012) - WebSocket message rate limiting at 30 msg/s (RQS-011) - Share token endpoint rate limiting (RQS-010) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract check_rate_limit_generic to deduplicate session/share rate limiters - Remove stale request index storage from DNS, TCP, and SMTP modules (indices were never read after delete_request switched to list scanning) - Remove test assertion for deleted index system - Call DNS rate limiter cleanup inline instead of spawning a task Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cargo-chef's dependency cargo-platform@0.3.2 requires rustc 1.88. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 15 security findings from the aisafe.io audit report. All changes are minimal, targeted, and avoid introducing new dependencies.
Triage
Accepted & Fixed
#[serde(deny_unknown_fields)]onClaimsstructwhileloopAccepted but Overstated Severity
max_requests_per_sessionalready. A per-connection memory burst doesn't OOM the server. Capped to last 100 anyway.request:keys leak in kv_storeFeedback for aisafe.io
RQS-015 severity is wrong. Rated Critical, but Tokio task panics are isolated — they kill one request handler, not the server. The report claims "continuously abort all Tokio worker tasks" and "severely disrupts overall server stability" which is false. A panic in a spawned task is caught by the Tokio runtime. This is Low severity at most.
RQS-014 impact is overstated. Claims "uncatchable Out-Of-Memory (OOM) panic terminating the application instance across the entire environment." The request list is already bounded by
max_requests_per_session. Loading 100 requests × a few KB each is not going to OOM anything.RQS-002 is by design. RequestRepo is a security research tool — serving custom HTTP responses with user-controlled headers and bodies is the primary feature. The report treats this as a vulnerability. We added CSP sandbox as defense-in-depth, but this isn't really a finding.
Multiple findings describe request-level failures as server-wide crashes. Tokio isolates task panics. A single request failing does not constitute a DoS against the server. The report repeatedly uses language like "entire environment" and "all tenants" for per-task issues.
RQS-001 timing attack is not practical. The report correctly notes this but still files it. Network jitter is orders of magnitude larger than comparison timing differences.
RQS-005 PoC is incorrect. The PoC shows unauthenticated API calls but the DNS endpoint requires JWT auth. The subdomain in the FQDN is constructed server-side from the authenticated session. The actual attack surface is narrower than described.
Performance notes
Mutex<HashMap>with periodic cleanup — minimal overhead at 100 qps thresholdTest plan
cargo clippy -- -D warningscleancargo fmtapplied🤖 Generated with Claude Code