fix: address Shannon security assessment findings (37 vulnerabilities) (#254)
Remediate 4 Critical, 18 High, 10 Medium, and 5 Low vulnerabilities identified in the Shannon security assessment (2026-03-20). Critical fixes: - Command injection via rsync SSH key path (INJ-VULN-01) - Self-escalation to super_admin role (AUTHZ-VULN-11) - Invite super_admin backdoor (AUTHZ-VULN-12) - Handlebars SSTI in email templates (INJ-VULN-05) Authentication hardening: - Rate limit on share-link login (AUTH-VULN-01) - X-Forwarded-For spoofing bypass (AUTH-VULN-02) - reCAPTCHA fails closed when misconfigured (AUTH-VULN-03) - Token revocation on admin/gallery logout (AUTH-VULN-04/05) - Cookie Secure flag defaults true in production (AUTH-VULN-06) - Remove JWT from admin login response body (AUTH-VULN-07) - Timing-safe gallery slug validation (AUTH-VULN-09) - Account lockout fails closed on DB error (AUTH-VULN-12) - Session endpoint checks token revocation Path traversal & file access: - checksums endpoint path containment (INJ-VULN-03) - manifest validate path containment (INJ-VULN-04) XSS prevention: - Block SVG data URIs in CSS sanitizer (XSS-VULN-01) - Email preview iframe sandbox (XSS-VULN-02) - SSR branding HTML escaping (XSS-VULN-03) - User-Agent sanitization in feedback (XSS-VULN-04) Authorization (IDOR): - Event ownership middleware for all admin routes - Cross-admin user profile read restriction (AUTHZ-VULN-10) SSRF & infrastructure: - Private IP validation for SMTP, S3, rsync hosts - Replace inline JWT with standard adminAuth middleware - CSRF Content-Type enforcement on mutating API endpoints - CSP headers in nginx location blocks Token revocation fix: - Remove overly broad orWhere clause that invalidated all future tokens - Allow empty-body POST requests (logout) in CSRF middleware Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
a63f1a8dd9
commit
23cd9cb680
@@ -261,7 +261,7 @@ async function checkAccountLockout(identifier, ipAddress) {
|
||||
return { isLocked: false };
|
||||
} catch (error) {
|
||||
logger.error('Error checking account lockout:', error);
|
||||
return { isLocked: false }; // Fail open to avoid locking users out due to errors
|
||||
return { isLocked: true, remainingTime: 300 }; // Fail closed on DB error
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -29,8 +29,8 @@ const FORBIDDEN_PATTERNS = [
|
||||
/on\w+\s*=/gi, // onclick=, onload=, etc.
|
||||
];
|
||||
|
||||
// Pattern for external URLs (block external, allow data: for images)
|
||||
const EXTERNAL_URL_PATTERN = /url\s*\(\s*["']?(?!data:image)/gi;
|
||||
// Pattern for external URLs (block external, allow only safe raster data: images)
|
||||
const EXTERNAL_URL_PATTERN = /url\s*\(\s*["']?(?!data:image\/(?:jpeg|jpg|png|gif|webp))/gi;
|
||||
|
||||
// Maximum CSS size in bytes (100KB)
|
||||
const MAX_CSS_SIZE = 100 * 1024;
|
||||
@@ -50,7 +50,8 @@ function sanitizeCss(css) {
|
||||
/@charset[^;]+;?/gi,
|
||||
/expression\s*\([^)]*\)/gi,
|
||||
/url\s*\(\s*(['"])\s*javascript:[^)]*\)/gi,
|
||||
/url\s*\(\s*(['"])\s*data:text\/javascript[^)]*\)/gi
|
||||
/url\s*\(\s*(['"])\s*data:text\/javascript[^)]*\)/gi,
|
||||
/url\s*\(\s*(['"]?)\s*data:image\/svg\+xml[^)]*\)/gi
|
||||
];
|
||||
|
||||
disallowedPatterns.forEach((pattern) => {
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
const { URL } = require('url');
|
||||
const net = require('net');
|
||||
|
||||
/**
|
||||
* Check if a hostname or IP resolves to a private/internal network address.
|
||||
* Blocks SSRF attempts targeting internal infrastructure.
|
||||
*/
|
||||
function isPrivateIP(hostname) {
|
||||
if (!hostname || typeof hostname !== 'string') return true;
|
||||
|
||||
const lower = hostname.toLowerCase().trim();
|
||||
|
||||
// Block known metadata / loopback hostnames
|
||||
const blockedHostnames = [
|
||||
'localhost',
|
||||
'metadata.google.internal',
|
||||
'metadata.google',
|
||||
'169.254.169.254',
|
||||
'0.0.0.0',
|
||||
'::1',
|
||||
'[::1]',
|
||||
];
|
||||
if (blockedHostnames.includes(lower)) return true;
|
||||
|
||||
// If it's an IP address, check ranges directly
|
||||
if (net.isIPv4(lower)) {
|
||||
return isPrivateIPv4(lower);
|
||||
}
|
||||
|
||||
// IPv6 checks
|
||||
if (net.isIPv6(lower) || lower.startsWith('[')) {
|
||||
const cleanIp = lower.replace(/^\[|\]$/g, '');
|
||||
return isPrivateIPv6(cleanIp);
|
||||
}
|
||||
|
||||
// Hostname patterns that resolve to internal services
|
||||
if (lower.endsWith('.internal') || lower.endsWith('.local') || lower.endsWith('.localhost')) {
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
function isPrivateIPv4(ip) {
|
||||
const parts = ip.split('.').map(Number);
|
||||
if (parts.length !== 4 || parts.some(p => isNaN(p))) return true;
|
||||
|
||||
const [a, b] = parts;
|
||||
|
||||
// 127.0.0.0/8 — loopback
|
||||
if (a === 127) return true;
|
||||
// 10.0.0.0/8 — private
|
||||
if (a === 10) return true;
|
||||
// 172.16.0.0/12 — private
|
||||
if (a === 172 && b >= 16 && b <= 31) return true;
|
||||
// 192.168.0.0/16 — private
|
||||
if (a === 192 && b === 168) return true;
|
||||
// 169.254.0.0/16 — link-local
|
||||
if (a === 169 && b === 254) return true;
|
||||
// 0.0.0.0/8
|
||||
if (a === 0) return true;
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
function isPrivateIPv6(ip) {
|
||||
const lower = ip.toLowerCase();
|
||||
// ::1 loopback
|
||||
if (lower === '::1' || lower === '0000:0000:0000:0000:0000:0000:0000:0001') return true;
|
||||
// fc00::/7 — unique local
|
||||
if (lower.startsWith('fc') || lower.startsWith('fd')) return true;
|
||||
// fe80::/10 — link-local
|
||||
if (lower.startsWith('fe80')) return true;
|
||||
// :: unspecified
|
||||
if (lower === '::') return true;
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate a URL string, rejecting private/internal targets.
|
||||
* @param {string} urlString - URL to validate
|
||||
* @returns {{ valid: boolean, error?: string }}
|
||||
*/
|
||||
function validateExternalUrl(urlString) {
|
||||
try {
|
||||
const parsed = new URL(urlString);
|
||||
if (isPrivateIP(parsed.hostname)) {
|
||||
return { valid: false, error: 'URL points to a private or internal network address' };
|
||||
}
|
||||
return { valid: true };
|
||||
} catch {
|
||||
return { valid: false, error: 'Invalid URL format' };
|
||||
}
|
||||
}
|
||||
|
||||
module.exports = { isPrivateIP, validateExternalUrl };
|
||||
@@ -213,17 +213,17 @@ async function validatePasswordInContext(password, context, userData = {}) {
|
||||
// Base validation with gallery-specific options
|
||||
const result = validatePassword(password, galleryOptions);
|
||||
|
||||
// Override validation for common date formats
|
||||
// Allow passwords like "04.07.2025", "04/07/2025", "04-07-2025"
|
||||
const datePattern = /^\d{1,2}[.\/-]\d{1,2}[.\/-]\d{4}$/;
|
||||
if (datePattern.test(password)) {
|
||||
// Date format is valid for gallery passwords
|
||||
return {
|
||||
valid: true,
|
||||
errors: [],
|
||||
score: 2,
|
||||
feedback: {}
|
||||
};
|
||||
// Only allow date-format passwords when complexity is 'simple'
|
||||
if (complexityLevel === 'simple') {
|
||||
const datePattern = /^\d{1,2}[.\/-]\d{1,2}[.\/-]\d{4}$/;
|
||||
if (datePattern.test(password)) {
|
||||
return {
|
||||
valid: true,
|
||||
errors: [],
|
||||
score: 2,
|
||||
feedback: {}
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
// Additional gallery-specific checks
|
||||
|
||||
@@ -9,28 +9,8 @@ function getClientIp(req) {
|
||||
if (!req) {
|
||||
return '';
|
||||
}
|
||||
|
||||
const forwardedFor = req.headers['x-forwarded-for'];
|
||||
|
||||
if (typeof forwardedFor === 'string' && forwardedFor.length > 0) {
|
||||
const [firstIp] = forwardedFor.split(',').map(part => part.trim()).filter(Boolean);
|
||||
if (firstIp) {
|
||||
return firstIp;
|
||||
}
|
||||
} else if (Array.isArray(forwardedFor) && forwardedFor.length > 0) {
|
||||
const [firstIp] = forwardedFor;
|
||||
if (firstIp) {
|
||||
return firstIp.trim();
|
||||
}
|
||||
}
|
||||
|
||||
return (
|
||||
req.ip ||
|
||||
req.connection?.remoteAddress ||
|
||||
req.socket?.remoteAddress ||
|
||||
req.connection?.socket?.remoteAddress ||
|
||||
''
|
||||
);
|
||||
// Use req.ip which respects Express 'trust proxy' setting
|
||||
return req.ip || req.connection?.remoteAddress || '';
|
||||
}
|
||||
|
||||
module.exports = { getClientIp };
|
||||
|
||||
@@ -57,11 +57,6 @@ async function isTokenRevoked(decodedToken) {
|
||||
|
||||
const revoked = await db('revoked_tokens')
|
||||
.where('token_id', tokenId)
|
||||
.orWhere((builder) => {
|
||||
builder
|
||||
.where('user_id', decodedToken.id)
|
||||
.where('revoked_at', '<=', new Date(decodedToken.iat * 1000).toISOString());
|
||||
})
|
||||
.first();
|
||||
|
||||
return !!revoked;
|
||||
|
||||
@@ -8,9 +8,8 @@ const secureCookie = (() => {
|
||||
if (typeof process.env.COOKIE_SECURE === 'string') {
|
||||
return process.env.COOKIE_SECURE.toLowerCase() === 'true';
|
||||
}
|
||||
// Default to false so native HTTP installs stay functional. Operators can
|
||||
// opt-in via COOKIE_SECURE=true when serving behind HTTPS.
|
||||
return false;
|
||||
// Default to true in production (HTTPS expected), false in development
|
||||
return process.env.NODE_ENV === 'production';
|
||||
})();
|
||||
const sameSiteDefault = process.env.COOKIE_SAMESITE || 'Lax';
|
||||
const cookieDomain = process.env.COOKIE_DOMAIN;
|
||||
|
||||
Reference in New Issue
Block a user