356 lines
11 KiB
Markdown
356 lines
11 KiB
Markdown
# Remaining Security Gaps Analysis
|
|
|
|
## ✅ Fully Resolved Issues
|
|
|
|
### 1. ✅ Rate Limiting / OTP Throttling
|
|
**Status:** **RESOLVED**
|
|
- Rate limiting implemented per phone and per IP
|
|
- OTP request throttling (3 per 10 min, 10 per 24h per phone)
|
|
- OTP verification attempt limits (5 per OTP, 10 failed per hour)
|
|
- **Location:** `src/middleware/rateLimitMiddleware.js`
|
|
|
|
### 2. ✅ OTP Exposure in Logs
|
|
**Status:** **RESOLVED**
|
|
- Safe OTP logging helper created (`src/utils/otpLogger.js`)
|
|
- Only logs in development mode
|
|
- Never logs to production
|
|
- **Location:** `src/services/smsService.js` uses `logOtpForDebug()`
|
|
|
|
### 3. ✅ IP/Device Risk Controls
|
|
**Status:** **RESOLVED**
|
|
- IP blocking for configured CIDR ranges
|
|
- Risk scoring based on IP/device/user-agent changes
|
|
- Suspicious refresh detection
|
|
- **Location:** `src/services/riskScoring.js`, integrated in `src/routes/authRoutes.js`
|
|
|
|
### 4. ✅ Refresh Token Theft Mitigation
|
|
**Status:** **RESOLVED**
|
|
- Environment fingerprinting (IP, user-agent, device ID)
|
|
- Suspicious refresh detection with risk scoring
|
|
- Optional OTP re-verification for suspicious refreshes
|
|
- Enhanced logging of suspicious events
|
|
- **Location:** `src/routes/authRoutes.js` (refresh endpoint)
|
|
|
|
### 5. ✅ JWT Claims Validation
|
|
**Status:** **RESOLVED**
|
|
- Strict validation of `iss`, `aud`, `exp`, `iat`, `nbf`
|
|
- Centralized validation function
|
|
- Used in all token verification paths
|
|
- **Location:** `src/services/jwtKeys.js` - `validateTokenClaims()`
|
|
|
|
### 6. ✅ CORS Hardening
|
|
**Status:** **RESOLVED**
|
|
- Strict origin whitelisting
|
|
- No wildcard support when credentials involved
|
|
- Production requirement for explicit origins
|
|
- **Location:** `src/index.js`
|
|
|
|
### 7. ✅ CSRF Protection (Documented)
|
|
**Status:** **RESOLVED** (Not needed - using Bearer tokens)
|
|
- Comprehensive documentation provided
|
|
- Guidance for future cookie-based implementation
|
|
- **Location:** `CSRF_NOTES.md`
|
|
|
|
---
|
|
|
|
## ⚠️ Partially Resolved Issues
|
|
|
|
### 8. ⚠️ Access Token Replay Mitigation
|
|
**Status:** **PARTIALLY RESOLVED**
|
|
|
|
**What's Done:**
|
|
- ✅ Step-up authentication middleware created (`src/middleware/stepUpAuth.js`)
|
|
- ✅ Access tokens include `high_assurance` claim after OTP verification
|
|
- ✅ Middleware checks for recent OTP or high assurance token
|
|
|
|
**What's Missing:**
|
|
- ❌ **Step-up auth NOT applied to sensitive routes**
|
|
- ❌ Sensitive operations in `src/routes/userRoutes.js` don't use `requireRecentOtpOrReauth`:
|
|
- `PUT /users/me` - Profile updates (could change phone number)
|
|
- `DELETE /users/me/devices/:device_id` - Device revocation
|
|
- `POST /users/me/logout-all-other-devices` - Mass device logout
|
|
|
|
**Risk Level:** 🟡 **MEDIUM**
|
|
- Stolen access token can be used for sensitive operations within 15-minute window
|
|
- Attacker could revoke devices, update profile, etc.
|
|
|
|
**Recommendation:**
|
|
```javascript
|
|
// Apply to sensitive routes:
|
|
router.put('/me', auth, requireRecentOtpOrReauth, async (req, res) => { ... });
|
|
router.delete('/me/devices/:device_id', auth, requireRecentOtpOrReauth, async (req, res) => { ... });
|
|
router.post('/me/logout-all-other-devices', auth, requireRecentOtpOrReauth, async (req, res) => { ... });
|
|
```
|
|
|
|
---
|
|
|
|
### 9. ⚠️ Secrets Management & Rotation
|
|
**Status:** **PARTIALLY RESOLVED**
|
|
|
|
**What's Done:**
|
|
- ✅ JWT key rotation structure implemented
|
|
- ✅ Support for multiple keys with `kid` (key ID)
|
|
- ✅ Graceful rotation without breaking existing tokens
|
|
- ✅ Code structure ready for secrets manager integration
|
|
|
|
**What's Missing:**
|
|
- ❌ **Still reads secrets from `.env` file**
|
|
- ❌ No integration with secrets manager (AWS Secrets Manager, HashiCorp Vault, etc.)
|
|
- ❌ Manual key rotation process (no automation)
|
|
- ❌ Twilio credentials still in `.env`
|
|
|
|
**Risk Level:** 🟡 **MEDIUM-HIGH**
|
|
- If `.env` leaks or is committed to git, attacker can:
|
|
- Forge JWTs
|
|
- Send SMS via Twilio
|
|
- Access database
|
|
|
|
**Recommendation:**
|
|
1. **Immediate:** Ensure `.env` is in `.gitignore` and never committed
|
|
2. **Short-term:** Use environment variables from secure deployment platform
|
|
3. **Long-term:** Integrate with secrets manager (see TODOs in `src/services/jwtKeys.js`)
|
|
|
|
**TODO in Code:**
|
|
- `src/services/jwtKeys.js` line 169: Example implementation for AWS Secrets Manager
|
|
- Need to implement `loadKeysFromSecretsManager()` function
|
|
|
|
---
|
|
|
|
### 10. ⚠️ Audit Logs Active Monitoring
|
|
**Status:** **PARTIALLY RESOLVED**
|
|
|
|
**What's Done:**
|
|
- ✅ Enhanced audit logging with risk levels (INFO, SUSPICIOUS, HIGH_RISK)
|
|
- ✅ Structured logging with metadata
|
|
- ✅ Anomaly detection helper function (`checkAnomalies()`)
|
|
- ✅ Suspicious events logged (failed OTPs, suspicious refreshes, blocked IPs)
|
|
|
|
**What's Missing:**
|
|
- ❌ **No active alerting/monitoring integration**
|
|
- ❌ No automated alerts for HIGH_RISK events
|
|
- ❌ No integration with PagerDuty, Slack, email, etc.
|
|
- ❌ Anomaly detection only logs to console (line 183 in `auditLogger.js`)
|
|
|
|
**Risk Level:** 🟡 **MEDIUM**
|
|
- Attacks happen silently until manual log inspection
|
|
- No real-time notification of security events
|
|
|
|
**Recommendation:**
|
|
1. **Immediate:** Set up log aggregation (CloudWatch, Datadog, etc.)
|
|
2. **Short-term:** Implement alerting for HIGH_RISK events
|
|
3. **Long-term:** Integrate with SIEM system
|
|
|
|
**TODO in Code:**
|
|
- `src/services/auditLogger.js` line 183: `// TODO: Trigger alert (email, webhook, etc.)`
|
|
- `src/services/auditLogger.js` line 193: Example implementation for external alerting
|
|
|
|
---
|
|
|
|
### 11. ⚠️ Input Validation
|
|
**Status:** **PARTIALLY RESOLVED**
|
|
|
|
**What's Done:**
|
|
- ✅ Input validation middleware created (`src/middleware/validation.js`)
|
|
- ✅ Validation applied to all auth routes:
|
|
- `/auth/request-otp`
|
|
- `/auth/verify-otp`
|
|
- `/auth/refresh`
|
|
- `/auth/logout`
|
|
|
|
**What's Missing:**
|
|
- ❌ **Validation NOT applied to user routes** (`src/routes/userRoutes.js`):
|
|
- `PUT /users/me` - No validation for `name`, `user_type`
|
|
- `DELETE /users/me/devices/:device_id` - No validation for `device_id` param
|
|
- `POST /users/me/logout-all-other-devices` - No validation for `current_device_id`
|
|
|
|
**Risk Level:** 🟢 **LOW-MEDIUM**
|
|
- Less critical than auth endpoints, but still important
|
|
- Could allow unexpected payloads or edge-case bugs
|
|
|
|
**Recommendation:**
|
|
```javascript
|
|
// Add validation middleware to user routes:
|
|
const { validateUpdateProfileBody, validateDeviceIdParam } = require('../middleware/validation');
|
|
|
|
router.put('/me', auth, validateUpdateProfileBody, async (req, res) => { ... });
|
|
router.delete('/me/devices/:device_id', auth, validateDeviceIdParam, async (req, res) => { ... });
|
|
```
|
|
|
|
---
|
|
|
|
## ❌ Unaddressed Attack Scenarios
|
|
|
|
### 12. ❌ Database Compromise
|
|
**Status:** **NOT ADDRESSED**
|
|
|
|
**Risk:**
|
|
- If database is compromised, attacker can:
|
|
- See phone numbers, device metadata, audit logs
|
|
- Correlate patterns for phishing/SIM-swap attacks
|
|
- Access user data
|
|
|
|
**Mitigation Needed:**
|
|
- ✅ Already using parameterized queries (good)
|
|
- ❌ No encryption at rest for sensitive fields
|
|
- ❌ No field-level encryption for PII
|
|
- ❌ No database access logging/auditing
|
|
|
|
**Recommendation:**
|
|
- Encrypt sensitive fields (phone numbers) at rest
|
|
- Implement database access logging
|
|
- Use database encryption (TDE, etc.)
|
|
- Regular security audits
|
|
|
|
---
|
|
|
|
### 13. ❌ Man-in-the-Middle (Non-HTTPS)
|
|
**Status:** **NOT ADDRESSED** (Infrastructure concern)
|
|
|
|
**Risk:**
|
|
- If client app talks to service over HTTP (no TLS):
|
|
- Attacker can intercept access/refresh tokens
|
|
- Attacker can intercept OTPs
|
|
- Full account compromise
|
|
|
|
**Mitigation:**
|
|
- ✅ Server assumes TLS termination in front (reverse proxy)
|
|
- ❌ No enforcement of HTTPS-only connections
|
|
- ❌ No HSTS headers
|
|
- ❌ No certificate pinning guidance
|
|
|
|
**Recommendation:**
|
|
- Enforce HTTPS at reverse proxy/load balancer
|
|
- Add HSTS headers
|
|
- Document TLS requirements
|
|
- Consider certificate pinning for mobile apps
|
|
|
|
---
|
|
|
|
### 14. ❌ Misconfigured CORS + XSS
|
|
**Status:** **PARTIALLY ADDRESSED**
|
|
|
|
**What's Done:**
|
|
- ✅ CORS hardened with strict origin whitelisting
|
|
- ✅ Documentation warns about misconfiguration
|
|
|
|
**What's Missing:**
|
|
- ❌ No validation that CORS is properly configured in production
|
|
- ❌ No runtime checks for CORS misconfiguration
|
|
- ❌ No guidance for XSS prevention in frontend
|
|
|
|
**Risk Level:** 🟡 **MEDIUM**
|
|
- If frontend has XSS and CORS is misconfigured:
|
|
- Attacker can use victim's tokens from malicious page
|
|
- Account takeover possible
|
|
|
|
**Recommendation:**
|
|
- Add startup validation that CORS origins are configured in production
|
|
- Document XSS prevention best practices
|
|
- Consider Content Security Policy (CSP) headers
|
|
|
|
---
|
|
|
|
## Summary Table
|
|
|
|
| Issue | Status | Risk Level | Priority |
|
|
|-------|--------|------------|----------|
|
|
| 1. Rate Limiting | ✅ Resolved | - | - |
|
|
| 2. OTP Logging | ✅ Resolved | - | - |
|
|
| 3. IP/Device Risk | ✅ Resolved | - | - |
|
|
| 4. Refresh Token Theft | ✅ Resolved | - | - |
|
|
| 5. JWT Claims | ✅ Resolved | - | - |
|
|
| 6. CORS | ✅ Resolved | - | - |
|
|
| 7. CSRF | ✅ Documented | - | - |
|
|
| 8. Access Token Replay | ⚠️ Partial | 🟡 Medium | **HIGH** |
|
|
| 9. Secrets Management | ⚠️ Partial | 🟡 Medium-High | **HIGH** |
|
|
| 10. Audit Monitoring | ⚠️ Partial | 🟡 Medium | **MEDIUM** |
|
|
| 11. Input Validation | ⚠️ Partial | 🟢 Low-Medium | **MEDIUM** |
|
|
| 12. Database Compromise | ❌ Not Addressed | 🔴 High | **MEDIUM** |
|
|
| 13. MITM (HTTP) | ❌ Not Addressed | 🔴 High | **LOW** (Infra) |
|
|
| 14. CORS + XSS | ⚠️ Partial | 🟡 Medium | **LOW** |
|
|
|
|
---
|
|
|
|
## Immediate Action Items (Priority Order)
|
|
|
|
### 🔴 HIGH PRIORITY
|
|
|
|
1. **Apply Step-Up Auth to Sensitive Routes**
|
|
- Add `requireRecentOtpOrReauth` to user routes
|
|
- Prevents access token replay attacks
|
|
|
|
2. **Secrets Management**
|
|
- Move secrets to environment variables (not `.env` file)
|
|
- Plan integration with secrets manager
|
|
- Ensure `.env` is never committed
|
|
|
|
### 🟡 MEDIUM PRIORITY
|
|
|
|
3. **Active Monitoring/Alerting**
|
|
- Set up alerts for HIGH_RISK audit events
|
|
- Integrate with monitoring system (CloudWatch, Datadog, etc.)
|
|
|
|
4. **Complete Input Validation**
|
|
- Add validation middleware to user routes
|
|
- Validate all request parameters
|
|
|
|
### 🟢 LOW PRIORITY
|
|
|
|
5. **Database Security**
|
|
- Plan encryption at rest
|
|
- Implement database access logging
|
|
|
|
6. **Infrastructure Security**
|
|
- Document TLS/HTTPS requirements
|
|
- Add HSTS headers
|
|
- Validate CORS configuration at startup
|
|
|
|
---
|
|
|
|
## Code Locations for Fixes
|
|
|
|
### Step-Up Auth Application
|
|
**File:** `src/routes/userRoutes.js`
|
|
- Line 99: `PUT /users/me` - Add `requireRecentOtpOrReauth`
|
|
- Line 160: `DELETE /users/me/devices/:device_id` - Add `requireRecentOtpOrReauth`
|
|
- Line 203: `POST /users/me/logout-all-other-devices` - Add `requireRecentOtpOrReauth`
|
|
|
|
### Input Validation
|
|
**File:** `src/routes/userRoutes.js`
|
|
- Add validation middleware imports
|
|
- Apply to all routes
|
|
|
|
### Secrets Manager Integration
|
|
**File:** `src/services/jwtKeys.js`
|
|
- Line 169: Implement `loadKeysFromSecretsManager()` function
|
|
- Replace `.env` reads with secrets manager calls
|
|
|
|
### Alerting Integration
|
|
**File:** `src/services/auditLogger.js`
|
|
- Line 183: Implement alert triggering
|
|
- Line 193: Add external alerting integration
|
|
|
|
---
|
|
|
|
## Conclusion
|
|
|
|
**7 out of 14 issues are fully resolved** ✅
|
|
**4 issues are partially resolved** ⚠️ (need completion)
|
|
**3 issues are not addressed** ❌ (infrastructure/planning)
|
|
|
|
**Overall Security Posture:** 🟡 **GOOD** (with room for improvement)
|
|
|
|
The most critical remaining gaps are:
|
|
1. Step-up auth not applied to sensitive routes
|
|
2. Secrets still in `.env` file
|
|
3. No active monitoring/alerting
|
|
|
|
These should be addressed before production deployment.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|