mirror of
https://codeberg.org/PostERG/xamxam.git
synced 2026-05-06 19:19:19 +02:00
security: fix all HIGH priority items from TODO.SECURITY.md
Items resolved: - #3 (HIGH): Move file uploads outside webroot to STORAGE_ROOT (/var/www/posterg/storage). Uploads were previously stored in public/admin/actions/data/ which is web-accessible. - #4 (HIGH): Align file paths and add media.php controller. DB paths are now storage-relative (theses/YEAR/ID/file, covers/file). New public/media.php serves files with path-traversal jail, MIME allow-list, and proper caching headers. memoire.php and search.php updated to use /media.php?path=. Also fixed: cover images were never recorded in thesis_files (broken INSERT). - #5 (HIGH): RateLimit::getClientIdentifier() now uses REMOTE_ADDR only. HTTP_X_FORWARDED_FOR and HTTP_CLIENT_IP are attacker-controlled headers that allowed unlimited rate-limit bypass by rotating spoofed IPs. - #6 (HIGH): Port public/admin/.htaccess security rules to nginx/posterg.conf. Apache .htaccess directives are silently ignored by nginx; none were active. CSP added to /admin/ location block, .log file denial added globally, autoindex off made explicit. Documented in nginx/HTACCESS_TO_NGINX.md. Supporting changes: - config/bootstrap.php: add STORAGE_ROOT constant - nginx/SECURITY_HEADERS.md: updated to reflect admin CSP and pending public CSP - docs/TODO.SECURITY.md: items #3-6 moved to resolved; priority order updated
This commit is contained in:
Théophile Gervreau-Mercier
@@ -6,6 +6,11 @@
|
||||
// Define application root
|
||||
define('APP_ROOT', dirname(__DIR__));
|
||||
|
||||
// Storage directory for uploaded files — intentionally outside the webroot
|
||||
// so no uploaded content is ever directly web-accessible (items #3 & #4).
|
||||
// Files are served through public/media.php which validates paths and MIME types.
|
||||
define('STORAGE_ROOT', '/var/www/posterg/storage');
|
||||
|
||||
// Error reporting
|
||||
if (php_sapi_name() === 'cli-server') {
|
||||
// Development mode
|
||||
|
||||
@@ -12,6 +12,10 @@
|
||||
| # | Issue | Severity | Resolution |
|
||||
|---|-------|----------|------------|
|
||||
| 1 | No HTTPS — admin credentials exposed in transit | 🔴 CRITICAL | **TLS is terminated upstream by the reverse proxy** (outside nginx). nginx.conf does not need to handle TLS directly. |
|
||||
| 3 | Uploaded files stored inside the webroot | 🟠 HIGH | Storage moved to `STORAGE_ROOT` (`/var/www/posterg/storage/`), defined in `config/bootstrap.php`. `formulaire.php` updated. |
|
||||
| 4 | File path mismatch — media files broken and insecure | 🟠 HIGH | DB paths are now storage-relative (`theses/YEAR/ID/file`, `covers/file`). New controller `public/media.php` serves files safely. `memoire.php` and `search.php` updated to use `/media.php?path=…`. Cover recording in `formulaire.php` fixed (was never inserted into DB). |
|
||||
| 5 | Rate limiter bypassed by IP spoofing (`X-Forwarded-For`) | 🟠 HIGH | `lib/RateLimit.php` `getClientIdentifier()` now uses `REMOTE_ADDR` only. |
|
||||
| 6 | `.htaccess` security rules silently ignored by nginx | 🟠 HIGH | All rules ported to `nginx/posterg.conf` (CSP to `/admin/` block, `.log` denial, `autoindex off`). See `nginx/HTACCESS_TO_NGINX.md`. |
|
||||
| 13 | Deprecated `X-XSS-Protection` header | 🔵 LOW | **Removed** from `nginx/posterg.conf`; see `nginx/SECURITY_HEADERS.md` for rationale. |
|
||||
|
||||
---
|
||||
@@ -57,9 +61,7 @@
|
||||
|
||||
| # | Issue | Severity | Files |
|
||||
|---|-------|----------|-------|
|
||||
| 6 | `.htaccess` security rules silently ignored by nginx | 🟠 HIGH | `public/admin/.htaccess` → move all headers/rules into nginx server block |
|
||||
| 11 | Missing Content-Security-Policy on public pages | 🟡 MEDIUM | `nginx/posterg.conf` → add `Content-Security-Policy` header |
|
||||
| 13 | Deprecated `X-XSS-Protection` header | 🔵 LOW | ✅ `nginx/posterg.conf` — header removed; documented in `nginx/SECURITY_HEADERS.md` |
|
||||
| 11 | Missing Content-Security-Policy on public pages | 🟡 MEDIUM | `nginx/posterg.conf` → add `Content-Security-Policy` header to main server block (admin already has CSP via item #6) |
|
||||
|
||||
---
|
||||
|
||||
@@ -72,40 +74,27 @@
|
||||
|
||||
---
|
||||
|
||||
### File Upload / Storage
|
||||
### Admin Panel — Error Logging
|
||||
|
||||
| # | Issue | Severity | Files |
|
||||
|---|-------|----------|-------|
|
||||
| 3 | Uploaded files stored inside the webroot | 🟠 HIGH | `public/admin/actions/formulaire.php` → move uploads to `/var/www/posterg/storage/` |
|
||||
| 4 | File path mismatch — media files broken and insecure | 🟠 HIGH | `formulaire.php`, `memoire.php`, `search.php` → align stored paths with a controller-served endpoint |
|
||||
| 9 | `error.log` written to a web-accessible path | 🟡 MEDIUM | `public/admin/actions/formulaire.php` → use absolute path outside webroot |
|
||||
|
||||
---
|
||||
|
||||
### Rate Limiting
|
||||
|
||||
| # | Issue | Severity | Files |
|
||||
|---|-------|----------|-------|
|
||||
| 5 | Rate limiter bypassed by IP spoofing (`X-Forwarded-For`) | 🟠 HIGH | `lib/RateLimit.php` → use `REMOTE_ADDR` only |
|
||||
|
||||
---
|
||||
|
||||
### File Import
|
||||
|
||||
| # | Issue | Severity | Files |
|
||||
|---|-------|----------|-------|
|
||||
| 12 | CSV import missing server-side MIME validation | 🟡 MEDIUM | `public/admin/import.php` → add `finfo` MIME check |
|
||||
|
||||
|
||||
|
||||
---
|
||||
|
||||
## Priority Order for Remaining Work
|
||||
|
||||
1. 🔴 **CRITICAL** — Item 2 (add simple PHP session auth guard)
|
||||
2. 🟠 **HIGH** — Items 3, 4, 5, 6 (file storage overhaul + nginx hardening + rate-limiter fix)
|
||||
3. 🟡 **MEDIUM** — Items 7, 8, 9, 11, 12 (sanitisation standardisation, session hardening, CSP, error log, CSV MIME)
|
||||
4. 🔵 **LOW** — ✅ All done (items 13–16)
|
||||
2. 🟡 **MEDIUM** — Items 7, 8, 9, 11, 12 (sanitisation standardisation, session hardening, CSP, error log, CSV MIME)
|
||||
3. 🔵 **LOW** — ✅ All done (items 13–16)
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -30,17 +30,15 @@ class RateLimit {
|
||||
* Get client identifier (IP address)
|
||||
* @return string Client identifier
|
||||
*/
|
||||
private function getClientIdentifier() {
|
||||
// Try to get real IP if behind proxy
|
||||
if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
|
||||
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
|
||||
} elseif (!empty($_SERVER['HTTP_CLIENT_IP'])) {
|
||||
$ip = $_SERVER['HTTP_CLIENT_IP'];
|
||||
} else {
|
||||
$ip = $_SERVER['REMOTE_ADDR'] ?? 'unknown';
|
||||
}
|
||||
|
||||
return $ip;
|
||||
private function getClientIdentifier(): string {
|
||||
// Use REMOTE_ADDR only — HTTP_X_FORWARDED_FOR and HTTP_CLIENT_IP
|
||||
// are fully attacker-controlled request headers and must never be
|
||||
// trusted for rate-limiting purposes (an attacker can rotate them
|
||||
// freely to bypass the limiter). Nginx-level rate limiting also
|
||||
// uses $binary_remote_addr for the same reason. If this app is
|
||||
// ever placed behind a trusted reverse-proxy, IP extraction should
|
||||
// be handled at the nginx level, not here.
|
||||
return $_SERVER['REMOTE_ADDR'] ?? 'unknown';
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
36
nginx/HTACCESS_TO_NGINX.md
Normal file
36
nginx/HTACCESS_TO_NGINX.md
Normal file
@@ -0,0 +1,36 @@
|
||||
# `.htaccess` → nginx migration (item #6)
|
||||
|
||||
> **Problem:** `public/admin/.htaccess` contained Apache-specific security
|
||||
> directives that nginx **silently ignores**. None of the rules were active
|
||||
> in production.
|
||||
|
||||
---
|
||||
|
||||
## Rules migrated into `nginx/posterg.conf`
|
||||
|
||||
| Apache `.htaccess` rule | nginx equivalent | Location |
|
||||
|---|---|---|
|
||||
| `Header always set X-Frame-Options "SAMEORIGIN"` | `add_header X-Frame-Options "SAMEORIGIN" always;` | main `server` block (already present) |
|
||||
| `Header always set X-Content-Type-Options "nosniff"` | `add_header X-Content-Type-Options "nosniff" always;` | main `server` block (already present) |
|
||||
| `Header always set X-XSS-Protection "1; mode=block"` | **Intentionally omitted** — deprecated & counterproductive; see `SECURITY_HEADERS.md` | — |
|
||||
| `Header always set Referrer-Policy "strict-origin-when-cross-origin"` | `add_header Referrer-Policy "strict-origin-when-cross-origin" always;` | main `server` block (already present) |
|
||||
| `Header always set Content-Security-Policy "..."` | `add_header Content-Security-Policy "..." always;` | `/admin/` location block (**added**) |
|
||||
| `Options -Indexes` | `autoindex off;` | `/admin/` location block (**added**; nginx default is off, explicit for clarity) |
|
||||
| `<FilesMatch "^\."> Require all denied` | `location ~ /\.(?!well-known).*` deny | main `server` block (already present) |
|
||||
| `<FilesMatch "(composer\.(json\|lock)\|error\.log)$"> Require all denied` | `location ~* \.(md\|txt\|sql\|sh\|json\|gitignore)$` deny + `location ~* \.log$` deny | main `server` block (`log` rule **added**) |
|
||||
| `php_flag display_errors Off` | Handled by `config/bootstrap.php` (`ini_set('display_errors', '0')`) | PHP |
|
||||
| `php_flag log_errors On` | Handled by `config/bootstrap.php` (`ini_set('log_errors', '1')`) | PHP |
|
||||
| `php_value error_log error.log` | Handled by `config/bootstrap.php`; should use absolute path (item #9) | PHP |
|
||||
|
||||
---
|
||||
|
||||
## Status of `public/admin/.htaccess`
|
||||
|
||||
The file is now **dead code** on this nginx server. It has been left in place
|
||||
(harmless) so it would still work if the project were ever tested behind Apache
|
||||
(e.g., `php -S` built-in server doesn't read it either). All security rules it
|
||||
previously attempted to set are now enforced by nginx directly.
|
||||
|
||||
---
|
||||
|
||||
*Added: 2026-02-08 — security item #6*
|
||||
@@ -1,6 +1,6 @@
|
||||
# Security Headers — nginx/posterg.conf
|
||||
|
||||
## Headers in use
|
||||
## Headers in use (main server block — all pages)
|
||||
|
||||
| Header | Value | Purpose |
|
||||
|--------|-------|---------|
|
||||
@@ -9,6 +9,17 @@
|
||||
| `Referrer-Policy` | `strict-origin-when-cross-origin` | Limit referrer leakage |
|
||||
| `Permissions-Policy` | `geolocation=(), microphone=(), camera=()` | Disable unused browser APIs |
|
||||
|
||||
## Headers in use (`/admin/` location block)
|
||||
|
||||
| Header | Value | Purpose |
|
||||
|--------|-------|---------|
|
||||
| `Content-Security-Policy` | `default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; object-src 'none'; frame-ancestors 'none';` | Restrict resource origins; block embedding |
|
||||
| `X-Robots-Tag` | `noindex, nofollow` | Prevent search-engine indexing of admin |
|
||||
|
||||
These were previously declared in `public/admin/.htaccess` as Apache
|
||||
`mod_headers` directives, which nginx silently ignores. They are now
|
||||
enforced directly; see `HTACCESS_TO_NGINX.md` for the full migration log.
|
||||
|
||||
## Intentionally omitted headers
|
||||
|
||||
### `X-XSS-Protection`
|
||||
@@ -21,10 +32,11 @@ This header was **removed** (was `"1; mode=block"`).
|
||||
to expose response bodies that would otherwise be blocked. Sending it provides
|
||||
no protection and may introduce risk.
|
||||
|
||||
**Correct mitigation:** a proper `Content-Security-Policy` header (todo item #11).
|
||||
**Correct mitigation:** a proper `Content-Security-Policy` header (now done for
|
||||
`/admin/`; public-page CSP is todo item #11).
|
||||
|
||||
## Pending headers
|
||||
|
||||
| Header | Status |
|
||||
|--------|--------|
|
||||
| `Content-Security-Policy` | ⏳ todo item #11 |
|
||||
| Header | Scope | Status |
|
||||
|--------|-------|--------|
|
||||
| `Content-Security-Policy` | Public pages (non-admin) | ⏳ todo item #11 |
|
||||
|
||||
@@ -74,6 +74,13 @@ server {
|
||||
# Rate limiting for admin
|
||||
limit_req zone=admin burst=5 nodelay;
|
||||
|
||||
# Content-Security-Policy — ported from public/admin/.htaccess which nginx ignores (item #6).
|
||||
# Tighter policy for admin: no external sources, no object embedding.
|
||||
add_header Content-Security-Policy "default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; object-src 'none'; frame-ancestors 'none';" always;
|
||||
|
||||
# Disable directory listing (nginx default is off; explicit for defence-in-depth, item #6)
|
||||
autoindex off;
|
||||
|
||||
# Allow access to public assets without authentication
|
||||
location ~ ^/admin/(css|js|images)/ {
|
||||
auth_basic off;
|
||||
@@ -134,6 +141,14 @@ server {
|
||||
try_files $uri $uri/ =404;
|
||||
}
|
||||
|
||||
# Deny access to log files — public/admin/.htaccess had this rule but Apache
|
||||
# .htaccess directives are silently ignored by nginx (item #6).
|
||||
location ~* \.log$ {
|
||||
deny all;
|
||||
access_log off;
|
||||
log_not_found off;
|
||||
}
|
||||
|
||||
# Deny access to .htaccess files (if Apache's document root concurs with nginx's)
|
||||
location ~ /\.ht {
|
||||
deny all;
|
||||
|
||||
@@ -194,9 +194,10 @@ try {
|
||||
|
||||
// ===== HANDLE FILE UPLOADS =====
|
||||
|
||||
// Create necessary directories
|
||||
$uploadBaseDir = __DIR__ . "/data/theses/{$annee}/{$identifier}/";
|
||||
$coverDir = __DIR__ . "/data/covers/";
|
||||
// Create necessary directories — outside the webroot (security items #3 & #4).
|
||||
// Files are served through /media.php, never directly via a URL path.
|
||||
$uploadBaseDir = STORAGE_ROOT . "/theses/{$annee}/{$identifier}/";
|
||||
$coverDir = STORAGE_ROOT . "/covers/";
|
||||
|
||||
if (!file_exists($uploadBaseDir)) {
|
||||
mkdir($uploadBaseDir, 0755, true);
|
||||
@@ -228,11 +229,18 @@ try {
|
||||
|
||||
if (move_uploaded_file($couverture["tmp_name"], $targetFile)) {
|
||||
chmod($targetFile, 0644);
|
||||
$coverPath = "data/covers/" . $safeFileName;
|
||||
// Path stored relative to STORAGE_ROOT; served via /media.php?path=...
|
||||
$coverPath = "covers/" . $safeFileName;
|
||||
|
||||
// Update thesis record with cover path
|
||||
$stmt = $pdo->prepare("UPDATE theses SET identifier = ? WHERE id = ?");
|
||||
// Store cover path in remarks for now (we could add a cover_path column)
|
||||
// Record cover in thesis_files (type = 'cover')
|
||||
$db->insertThesisFile(
|
||||
$thesisId,
|
||||
'cover',
|
||||
$coverPath,
|
||||
basename($couverture["name"]),
|
||||
$couverture["size"],
|
||||
$mimeType
|
||||
);
|
||||
error_log("Cover image uploaded: " . $safeFileName);
|
||||
}
|
||||
} else {
|
||||
@@ -284,11 +292,11 @@ try {
|
||||
$fileType = 'main';
|
||||
}
|
||||
|
||||
// Insert file record
|
||||
// Insert file record — path relative to STORAGE_ROOT
|
||||
$db->insertThesisFile(
|
||||
$thesisId,
|
||||
$fileType,
|
||||
"data/theses/{$annee}/{$identifier}/" . $safeFileName,
|
||||
"theses/{$annee}/{$identifier}/" . $safeFileName,
|
||||
basename($files["name"][$i]),
|
||||
$files["size"][$i],
|
||||
$mimeType
|
||||
|
||||
90
public/media.php
Normal file
90
public/media.php
Normal file
@@ -0,0 +1,90 @@
|
||||
<?php
|
||||
/**
|
||||
* Media file serving controller
|
||||
*
|
||||
* Serves uploaded files that are stored outside the webroot (STORAGE_ROOT).
|
||||
* This is the sole access point for thesis files, covers, and annexes — they
|
||||
* are never exposed as direct filesystem paths from the web server.
|
||||
*
|
||||
* Security:
|
||||
* - Strict character whitelist on the path parameter (no path traversal)
|
||||
* - realpath() jail: resolved path must stay inside STORAGE_ROOT
|
||||
* - MIME type verified against an allow-list before serving
|
||||
*/
|
||||
require_once __DIR__ . '/../config/bootstrap.php';
|
||||
|
||||
// --- 1. Extract and validate the requested path -------------------------------
|
||||
|
||||
$requestedPath = $_GET['path'] ?? '';
|
||||
|
||||
// Only allow safe characters: letters, digits, forward slashes, dots, hyphens, underscores.
|
||||
// This explicitly rejects null bytes, backslashes, and sequences like "../".
|
||||
if (!preg_match('#^[a-zA-Z0-9/_\-.]+$#', $requestedPath) || $requestedPath === '') {
|
||||
http_response_code(400);
|
||||
exit;
|
||||
}
|
||||
|
||||
// --- 2. Resolve full filesystem path and enforce storage jail -----------------
|
||||
|
||||
$storageRoot = defined('STORAGE_ROOT') ? STORAGE_ROOT : '/var/www/posterg/storage';
|
||||
$fullPath = $storageRoot . '/' . $requestedPath;
|
||||
|
||||
$realStorage = realpath($storageRoot);
|
||||
$realFull = realpath($fullPath);
|
||||
|
||||
if (
|
||||
$realFull === false
|
||||
|| $realStorage === false
|
||||
|| strpos($realFull, $realStorage . '/') !== 0
|
||||
) {
|
||||
http_response_code(404);
|
||||
exit;
|
||||
}
|
||||
|
||||
if (!is_file($realFull)) {
|
||||
http_response_code(404);
|
||||
exit;
|
||||
}
|
||||
|
||||
// --- 3. Verify MIME type from file content (not extension) --------------------
|
||||
|
||||
$finfo = new finfo(FILEINFO_MIME_TYPE);
|
||||
$mimeType = $finfo->file($realFull);
|
||||
|
||||
$allowedMimes = [
|
||||
'image/jpeg',
|
||||
'image/png',
|
||||
'image/gif',
|
||||
'application/pdf',
|
||||
'video/mp4',
|
||||
'application/zip',
|
||||
];
|
||||
|
||||
if (!in_array($mimeType, $allowedMimes, true)) {
|
||||
http_response_code(403);
|
||||
exit;
|
||||
}
|
||||
|
||||
// --- 4. Send response headers -------------------------------------------------
|
||||
|
||||
header('Content-Type: ' . $mimeType);
|
||||
header('Content-Length: ' . filesize($realFull));
|
||||
header('X-Content-Type-Options: nosniff');
|
||||
|
||||
$ext = strtolower(pathinfo($realFull, PATHINFO_EXTENSION));
|
||||
|
||||
if (in_array($ext, ['jpg', 'jpeg', 'png', 'gif'], true)) {
|
||||
// Images: cache publicly for 7 days
|
||||
header('Cache-Control: public, max-age=604800');
|
||||
} elseif ($ext === 'pdf') {
|
||||
// PDFs: cache for 1 day, display inline
|
||||
header('Cache-Control: public, max-age=86400');
|
||||
header('Content-Disposition: inline');
|
||||
} else {
|
||||
// Everything else: no public caching
|
||||
header('Cache-Control: private, no-store');
|
||||
}
|
||||
|
||||
// --- 5. Stream file -----------------------------------------------------------
|
||||
|
||||
readfile($realFull);
|
||||
@@ -125,16 +125,16 @@ include APP_ROOT . '/includes/header.php'; ?>
|
||||
<div class="block">
|
||||
<?php if ($ext === 'pdf'): ?>
|
||||
<!-- Display PDF files using the embed element -->
|
||||
<embed src="<?= htmlspecialchars($file['file_path']); ?>" type="application/pdf" width="100%" height="600px" />
|
||||
<embed src="/media.php?path=<?= urlencode($file['file_path']); ?>" type="application/pdf" width="100%" height="600px" />
|
||||
<?php elseif (in_array($ext, ['jpg', 'jpeg', 'png', 'gif', 'bmp'])): ?>
|
||||
<!-- Display image files using the img element -->
|
||||
<figure>
|
||||
<img src="<?= htmlspecialchars($file['file_path']); ?>" alt="<?= htmlspecialchars($file['file_name']); ?>">
|
||||
<img src="/media.php?path=<?= urlencode($file['file_path']); ?>" alt="<?= htmlspecialchars($file['file_name']); ?>">
|
||||
</figure>
|
||||
<?php elseif ($ext === 'mp4'): ?>
|
||||
<!-- Display MP4 video files using the video element -->
|
||||
<video width="100%" height="auto" controls>
|
||||
<source src="<?= htmlspecialchars($file['file_path']); ?>" type="video/mp4">
|
||||
<source src="/media.php?path=<?= urlencode($file['file_path']); ?>" type="video/mp4">
|
||||
Your browser does not support the video tag.
|
||||
</video>
|
||||
<?php endif; ?>
|
||||
|
||||
@@ -341,9 +341,9 @@ include APP_ROOT . '/includes/header.php'; ?>
|
||||
if (!empty($item['id'])) {
|
||||
$files = $db->getThesisFiles($item['id']);
|
||||
foreach ($files as $file) {
|
||||
$ext = strtolower(pathinfo($file['file_path'], PATHINFO_EXTENSION));
|
||||
if (in_array($ext, ['jpg', 'jpeg', 'png', 'gif']) && $file['file_type'] === 'main') {
|
||||
$coverImage = $file['file_path'];
|
||||
// Look for dedicated cover file (type = 'cover')
|
||||
if ($file['file_type'] === 'cover') {
|
||||
$coverImage = '/media.php?path=' . urlencode($file['file_path']);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user