diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md new file mode 100644 index 0000000..3a83340 --- /dev/null +++ b/SECURITY_REVIEW.md @@ -0,0 +1,275 @@ +# Security Review - Simple Object Server + +Date: 2025-01-10 +Reviewed by: Security Audit Team + +## Executive Summary + +This comprehensive security review analyzes the Simple Object Server C++23 application for potential vulnerabilities and security issues. The application provides a REST API for file storage with deduplication, metadata management, and token-based authentication. The review covered authentication, input validation, SQL injection risks, file upload security, rate limiting, CORS configuration, cryptographic implementations, error handling, and third-party dependencies. + +## Critical Issues (MUST FIX) + +### 1. **Hardcoded Authentication Tokens in Test Configuration** +- **Location**: `testing/sos_config.json:2-6` +- **Risk**: CRITICAL - Exposed authentication tokens in repository +- **Issue**: Test configuration contains hardcoded plaintext tokens ("fizzle1", "fizzle2", "fizzle3") +- **Evidence**: Tokens visible in version control history +- **Recommendation**: + - Remove hardcoded tokens from repository immediately + - Use environment variables or external configuration + - Add `sos_config.json` to `.gitignore` + - Provide a `sos_config.json.example` template instead + - Rotate all existing tokens + +### 2. **No Token Hashing/Encryption** +- **Location**: `src/server.cpp:70` +- **Risk**: CRITICAL - Tokens stored and compared in plaintext +- **Issue**: Authentication tokens are stored in memory and compared directly as strings +- **Impact**: Token compromise exposes actual credentials +- **Recommendation**: + - Implement token hashing using bcrypt or argon2 + - Store only hashed tokens in configuration + - Hash incoming tokens before comparison + - Consider implementing JWT or OAuth2 for better security + +### 3. **Weak Cryptographic Hash for Content** +- **Location**: `src/hash.cpp:12-56` +- **Risk**: HIGH - Using XXHash (non-cryptographic) for content identification +- **Issue**: XXHash is designed for speed, not security - vulnerable to collision attacks +- **Impact**: Potential for malicious file substitution through hash collisions +- **Recommendation**: + - Replace XXHash with SHA-256 or SHA-3 for content hashing + - Use cryptographic hashes for security-critical operations + - Keep XXHash only for non-security checksums if needed + +## High-Risk Issues + +### 4. **Missing Security Headers** +- **Location**: HTTP response handling throughout +- **Risk**: HIGH - Missing standard security headers +- **Issue**: No implementation of standard security headers +- **Recommendation**: Add security headers to all responses: + ```cpp + resp->addHeader("X-Frame-Options", "DENY"); + resp->addHeader("X-Content-Type-Options", "nosniff"); + resp->addHeader("X-XSS-Protection", "1; mode=block"); + resp->addHeader("Strict-Transport-Security", "max-age=31536000"); + resp->addHeader("Content-Security-Policy", "default-src 'self'"); + ``` + +### 5. **Insufficient Input Validation** +- **Location**: Multiple endpoints (put_handler.cpp, update_handler.cpp) +- **Risk**: HIGH - Limited validation of input data +- **Issues**: + - No maximum length validation for label:tag pairs + - Limited JSON schema validation for metadata + - No sanitization of special characters in labels/tags + - File names from uploads used without proper sanitization +- **Recommendation**: + - Implement comprehensive input validation schemas + - Add length limits for all string inputs (e.g., max 255 chars for labels) + - Validate against allowed character sets (alphanumeric + limited special chars) + - Sanitize all user inputs before processing + +## Medium-Risk Issues + +### 6. **Information Disclosure in Error Messages** +- **Location**: Error handling throughout (server.cpp:251, 263, 336-339) +- **Risk**: MEDIUM - Verbose error messages +- **Issue**: Error messages expose internal details: + - File paths in error responses (`server.cpp:263`) + - Database error details logged to console + - Internal hash values in error messages +- **Recommendation**: + - Implement generic error messages for production + - Log detailed errors server-side only + - Use error codes instead of descriptive messages + - Implement structured logging with log levels + +### 7. **Weak Rate Limiting Configuration** +- **Location**: `src/server.cpp:123-126`, `testing/sos_config.json:8-9` +- **Risk**: MEDIUM - Rate limiting only on authentication +- **Issues**: + - Rate limiting only applies to failed auth attempts + - No rate limiting on read operations + - Test config has very weak limits (5 attempts in 2 seconds) + - No distributed rate limiting for clustered deployments +- **Recommendation**: + - Implement rate limiting for all endpoints + - Use stronger default limits (e.g., 10 attempts per 300 seconds) + - Consider per-IP and per-token rate limiting + - Add configurable rate limits per endpoint type + +### 8. **CORS Wildcard Support** +- **Location**: `src/server.cpp:207-212` +- **Risk**: MEDIUM - Potential for overly permissive CORS +- **Issue**: Support for wildcard (*) origins can lead to security misconfigurations +- **Recommendation**: + - Require explicit origin listing in production + - Validate origin format before accepting + - Consider removing wildcard support entirely + - Log CORS violations for monitoring + +## Low-Risk Issues + +### 9. **No Request Signing/HMAC** +- **Risk**: LOW - No message integrity verification +- **Issue**: Requests are not signed, allowing potential tampering +- **Recommendation**: Consider implementing HMAC signing for write operations + +### 10. **Limited Monitoring/Alerting** +- **Risk**: LOW - Insufficient security monitoring +- **Issue**: No built-in security event monitoring or alerting +- **Recommendation**: + - Add metrics for failed auth attempts + - Monitor for unusual access patterns + - Implement alerting for security events + +## Good Security Practices Observed + +### Strengths +1. **SQL Injection Protection**: Excellent use of prepared statements with parameter binding throughout +2. **Memory Safety**: Modern C++23 with RAII patterns and smart pointers +3. **File Upload Security**: + - Appropriate size limits (6GB max) + - Content deduplication prevents storage exhaustion + - Proper temporary file cleanup on errors +4. **Path Traversal Protection**: Files stored by hash, eliminating path traversal risks +5. **Thread Safety**: Proper use of mutexes for shared resources +6. **Build Security**: Static linking reduces dependency risks + +## Dependency Analysis + +### Third-Party Libraries +- **Drogon Framework**: Modern, actively maintained web framework +- **nlohmann/json**: Well-tested JSON library with good security track record +- **SQLite3**: Battle-tested database with good security history +- **OpenSSL**: Industry standard, though requires regular updates + +### Recommendations for Dependencies +1. Implement automated dependency scanning +2. Set up security alerts for known vulnerabilities +3. Regular dependency updates schedule +4. Consider using package pinning for reproducible builds + +## Security Architecture Recommendations + +### 1. **Defense in Depth** +Implement multiple layers of security: +```cpp +// Example: Layered validation +bool validateRequest(const Request& req) { + if (!checkRateLimit(req)) return false; + if (!authenticateToken(req)) return false; + if (!validateInput(req)) return false; + if (!authorizeAction(req)) return false; + return true; +} +``` + +### 2. **Secure Configuration Management** +```json +{ + "security": { + "token_hash_algorithm": "argon2id", + "token_min_length": 32, + "enable_audit_log": true, + "max_request_size": "100MB", + "session_timeout": 3600 + } +} +``` + +### 3. **Audit Logging Implementation** +```cpp +class AuditLogger { + void logAuthAttempt(const std::string& ip, bool success); + void logWriteOperation(const std::string& token, const std::string& operation); + void logSecurityEvent(const std::string& event_type, const json& details); +}; +``` + +## Priority Action Items + +1. **CRITICAL - Immediate**: + - Remove hardcoded tokens from repository + - Implement token hashing + - Replace XXHash with SHA-256 for content identification + +2. **HIGH - Before Public Release**: + - Add security headers to all responses + - Implement comprehensive input validation + - Improve rate limiting coverage + +3. **MEDIUM - Within 30 Days**: + - Add security documentation (SECURITY.md) + - Implement audit logging + - Set up dependency scanning + +4. **LOW - Continuous Improvement**: + - Add request signing + - Implement security monitoring + - Regular security audits + +## Testing Recommendations + +### Security Testing Checklist +- [ ] Static analysis with clang-tidy and cppcheck +- [ ] Dynamic application security testing (DAST) +- [ ] Fuzzing for input validation +- [ ] Penetration testing for authentication bypass +- [ ] Load testing for DoS resilience +- [ ] Dependency vulnerability scanning + +### Example Security Test Cases +```bash +# Test authentication bypass +curl -X POST http://localhost:7703/upload -H "Authorization: Bearer invalid" + +# Test SQL injection +curl "http://localhost:7703/metadata?key=test' OR '1'='1" + +# Test path traversal +curl "http://localhost:7703/object/../../../etc/passwd" + +# Test rate limiting +for i in {1..20}; do + curl -X POST http://localhost:7703/upload -H "Authorization: Bearer wrong" +done +``` + +## Compliance and Regulatory Considerations + +1. **Data Protection**: + - Implement data retention policies + - Add secure deletion capabilities + - Consider encryption at rest + +2. **Privacy**: + - Add privacy policy endpoint + - Implement data subject rights (GDPR) + - Metadata anonymization options + +3. **Audit Requirements**: + - Tamper-proof audit logs + - Log retention policies + - Security event correlation + +## Conclusion + +The Simple Object Server demonstrates solid foundational security practices, particularly in SQL injection prevention and memory safety. However, critical vulnerabilities including hardcoded tokens, lack of token hashing, and use of non-cryptographic hashes for content identification must be addressed before public release. + +The application would benefit from: +1. Immediate remediation of critical issues +2. Implementation of defense-in-depth strategies +3. Comprehensive security testing +4. Ongoing security monitoring and updates + +With the recommended improvements, this application can achieve a robust security posture suitable for production deployment. + +## Resources + +- [OWASP Top 10](https://owasp.org/Top10/) +- [CWE Top 25](https://cwe.mitre.org/top25/) +- [NIST Cybersecurity Framework](https://www.nist.gov/cyberframework) +- [ISO 27001 Standards](https://www.iso.org/isoiec-27001-information-security.html) \ No newline at end of file diff --git a/build.sh b/build.sh index 1e90fa1..619d9ff 100755 --- a/build.sh +++ b/build.sh @@ -1,25 +1,57 @@ #!/bin/bash - set -euo pipefail +# Get script directory - handle different execution contexts SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" +PROJECT="$(basename "${SCRIPT_DIR}")" +# Debug output for CI +echo "${PROJECT} build script running from: ${SCRIPT_DIR}" -export CMAKE_BUILD_TYPE="Debug" +# handle running locally, or docker in docker via gitea runner. +if [ -n "${GITEA_CONTAINER_NAME:-}" ]; then + echo "We're in a gitea container: ${GITEA_CONTAINER_NAME}" + VOLUME_OPTS=("--volumes-from=${GITEA_CONTAINER_NAME}") + WORKING_DIR=("-w" "${GITHUB_WORKSPACE}") + BUILD_DIR="${GITHUB_WORKSPACE}/build" + OUTPUT_DIR="${GITHUB_WORKSPACE}/output" +else + VOLUME_OPTS=("-v" "${SCRIPT_DIR}:/app") + WORKING_DIR=("-w" "/app") + BUILD_DIR="${SCRIPT_DIR}/build" + OUTPUT_DIR="${SCRIPT_DIR}/output" +fi -rm -rf "${SCRIPT_DIR}/output" -mkdir -p "${SCRIPT_DIR}/output" +# Create output directory +mkdir -p "${OUTPUT_DIR}" -PROJECT="simple-object-server" +# Create build directory with proper permissions +mkdir -p "${BUILD_DIR}" -# Enable Docker buildkit for cache support -export DOCKER_BUILDKIT=1 +# Run build in container with mounted directories +# Note: We run as root in the container and then chown the results +COMMAND_TO_RUN="mkdir -p ./build && \ + cmake -G Ninja -S . -B ./build \ + -DCMAKE_BUILD_TYPE=\${CMAKE_BUILD_TYPE} \ + -DPROJECT_NAME=${PROJECT} && \ + cmake --build ./build && \ + chown -R $(id -u):$(id -g) ./build" -docker build \ - -t "${PROJECT}-build" \ - -f "${SCRIPT_DIR}/Dockerfile.dropshell-build" \ - --build-arg PROJECT="${PROJECT}" \ - --build-arg CMAKE_BUILD_TYPE="${CMAKE_BUILD_TYPE}" \ - --output "${SCRIPT_DIR}/output" \ - "${SCRIPT_DIR}" +echo "Building in new docker container" +docker run --rm \ + "${VOLUME_OPTS[@]}" \ + "${WORKING_DIR[@]}" \ + -e CMAKE_BUILD_TYPE="${CMAKE_BUILD_TYPE:-Debug}" \ + gitea.jde.nz/public/dropshell-build-base:latest \ + bash -c "${COMMAND_TO_RUN}" +# Copy built executable to output directory +if [ -f "${BUILD_DIR}/${PROJECT}" ]; then + cp "${BUILD_DIR}/${PROJECT}" "${OUTPUT_DIR}/" + echo "✓ Build successful - ${PROJECT} copied to ${OUTPUT_DIR}/" +else + echo "✗ Build failed - ${PROJECT} not found in ${BUILD_DIR}/" + exit 1 +fi + +echo "Build complete"