security: fix all LOW priority items from TODO.SECURITY.md

Item 13 — Remove deprecated X-XSS-Protection header
- nginx/posterg.conf: header removed (was '1; mode=block')
- nginx/SECURITY_HEADERS.md: new file documenting header decisions
  and explaining why X-XSS-Protection is counterproductive

Item 14 — Add rel="noreferrer" to external target="_blank" link
- public/admin/thanks.php: rel="noopener" → rel="noopener noreferrer"

Item 15 — Explicit (int) casts on all integer HTML outputs
- public/index.php: (int) on item id, page numbers
- public/search.php: (int) on totalItems, year options, item id, pagination

Item 16 — Remove unused DATABASE_PATH constant
- config/bootstrap.php: define('DATABASE_PATH', ...) removed

docs/TODO.SECURITY.md updated: items 13-16 marked resolved and
moved to the  Resolved section.
This commit is contained in:
Théophile Gervreau-Mercier
2026-02-08 11:58:51 +01:00
parent 94d110438f
commit f5d3281c43
8 changed files with 490 additions and 221 deletions

View File

@@ -1,277 +1,406 @@
# Security Analysis - Search Feature
# 🔒 Security Vulnerability Analysis — posterg-website
## Current Security Status
### ✅ Protections in Place
1. **SQL Injection Prevention**
- ✅ Uses PDO prepared statements
- ✅ All parameters bound with `bindValue()`
- ✅ No direct concatenation of user input into SQL
- ✅ Dynamic WHERE clause built from hardcoded strings only
2. **XSS (Cross-Site Scripting) Prevention**
- ✅ All output uses `htmlspecialchars()`
- ✅ Form values escaped when displayed
- ✅ Search results escaped before rendering
3. **Access Control**
- ✅ Only published theses searchable (`is_published = 1`)
- ✅ Uses read-only view (`v_theses_public`)
4. **Type Safety**
- ✅ Year parameter uses `intval()`
- ✅ Boolean values properly cast
> **Date:** 2026-02-08
> **Scope:** Full static code review of all PHP, nginx config, and deployment files.
> **Analyst:** Claude Code
---
## ⚠️ Security Vulnerabilities
## Good Practices Already in Place
### 1. LIKE Wildcard Injection (Low Severity)
- ✅ SQL injection: all queries use PDO prepared statements consistently
- ✅ XSS output: `htmlspecialchars()` applied on all user-controlled output
- ✅ CSRF: tokens generated with `bin2hex(random_bytes(32))` and validated with `hash_equals()` (timing-safe) on all state-changing forms
- ✅ File upload: MIME type validated with `finfo` for cover images and thesis files
- ✅ Input validation: year, IDs, and pagination values cast to integers
- ✅ LIKE wildcard escaping implemented in public search (`Database::escapeLikeString`)
**Issue:** Users can inject SQL LIKE wildcards (`%`, `_`) to match unintended patterns.
---
**Example Attack:**
```
Search query: "%"
Result: Matches ALL theses (bypasses search intent)
## 🔴 CRITICAL
Search query: "a%b%c%d%e%f%g%h%i%j%k%l%m%n%o%p%q%r%s%t%u%v%w%x%y%z"
Result: Forces inefficient pattern matching, potential DoS
### 1. Admin HTTP Basic Auth Over Plain HTTP (No TLS/HTTPS)
**File:** `nginx/posterg.conf`
The nginx config listens only on port 80. HTTP Basic Authentication Base64-encodes
credentials but **does not encrypt them**. Anyone intercepting network traffic can
decode the admin password trivially (`echo "dXNlcjpwYXNz" | base64 -d`).
```nginx
listen 80 default_server; # ← No HTTPS configured at all
auth_basic "Admin Access - Post-ERG";
auth_basic_user_file /etc/nginx/.htpasswd-posterg;
```
**Current Code:**
```php
$bindings[':query'] = '%' . $params['query'] . '%';
```
**Fix:** Add a TLS/HTTPS server block (e.g., Let's Encrypt via certbot) and redirect
all port 80 traffic to HTTPS:
**Impact:**
- Not SQL injection (still uses prepared statements)
- Allows overly broad searches
- Performance degradation with complex patterns
- Information disclosure through pattern matching
**Fix:** Escape wildcards before using in LIKE:
```php
private function escapeLikeString($string) {
return str_replace(['\\', '%', '_'], ['\\\\', '\\%', '\\_'], $string);
```nginx
server {
listen 80;
return 301 https://$host$request_uri;
}
// In query:
$bindings[':query'] = '%' . $this->escapeLikeString($params['query']) . '%';
// In SQL:
"title LIKE :query ESCAPE '\\'"
```
---
### 2. No Input Length Validation (Medium Severity)
**Issue:** No limits on search string length.
**Example Attack:**
```php
// 10MB query string
$query = str_repeat('a', 10 * 1024 * 1024);
```
**Impact:**
- Memory exhaustion
- Database query slowdown
- Denial of Service (DoS)
**Fix:** Validate input length:
```php
if (strlen($params['query']) > 200) {
throw new InvalidArgumentException("Search query too long");
server {
listen 443 ssl;
ssl_certificate /etc/letsencrypt/live/posterg.erg.be/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/posterg.erg.be/privkey.pem;
# ... rest of config
}
```
---
### 3. No Rate Limiting (Medium Severity)
### 2. No PHP-Level Authentication in Admin Panel
**Issue:** Unlimited search requests allowed.
**Files:** `public/admin/index.php`, `add.php`, `edit.php`, `import.php`, `thanks.php`
**Example Attack:**
```bash
# Spam 10,000 requests
for i in {1..10000}; do
curl "http://site.com/search.php?query=test&page=$i" &
done
```
Every admin file calls `session_start()` only to generate a CSRF token. There is
**zero check that the user is authenticated in PHP**. If nginx is misconfigured,
restarted, or the app is accessed directly without nginx in front (e.g., via the
PHP CLI dev server), all admin functionality is fully open.
**Impact:**
- Database overload
- Server resource exhaustion
- Denial of Service for legitimate users
**Fix:** Implement rate limiting (see solution below)
---
### 4. No Pagination Limits (Low Severity)
**Issue:** Users can request excessive offset values.
**Example:**
```
search.php?page=999999999
```
**Impact:**
- Database scans large result sets
- Wasted resources on impossible pages
**Fix:** Validate pagination:
```php
$limit = max(1, min(100, intval($limit))); // Max 100 per page
$offset = max(0, intval($offset));
session_start();
if (empty($_SESSION['csrf_token'])) {
$_SESSION['csrf_token'] = bin2hex(random_bytes(32));
}
// ← No authentication check whatsoever
```
// Optionally limit max offset
if ($offset > 10000) {
throw new InvalidArgumentException("Page too high");
**Fix:** Implement a PHP login system with session-based authentication and add an
auth guard at the top of every admin page:
```php
// public/admin/auth.php
session_start();
if (empty($_SESSION['admin_authenticated'])) {
header('Location: /admin/login.php');
exit;
}
```
---
## 🔒 Recommended Security Improvements
## 🟠 HIGH
### Priority 1: Apply Input Validation (HIGH)
### 3. Uploaded Files Stored Inside the Webroot
Use the enhanced `Database_secure.php` class which includes:
- Wildcard escaping
- Length validation
- Range validation
- ESCAPE clause in LIKE queries
**File:** `public/admin/actions/formulaire.php` (lines ~127131)
### Priority 2: Implement Rate Limiting (MEDIUM)
Example using simple file-based rate limiting:
Thesis files and cover images are stored *inside* the public web directory:
```php
<?php
// rate_limit.php - Simple rate limiter
$uploadBaseDir = __DIR__ . "/data/theses/{$annee}/{$identifier}/";
$coverDir = __DIR__ . "/data/covers/";
// Resolves to: /var/www/posterg/public/admin/actions/data/...
```
function checkRateLimit($identifier, $maxRequests = 10, $timeWindow = 60) {
$cacheDir = __DIR__ . '/cache/rate_limit';
if (!is_dir($cacheDir)) {
mkdir($cacheDir, 0755, true);
}
This means uploaded PDFs, ZIPs, MP4s, and images sit in a potentially web-accessible
path. The nginx rule `location ^~ /data/ { deny all; }` only blocks the URL path
`/data/…` from the server root — it does **not** block `/admin/actions/data/…`.
$file = $cacheDir . '/' . md5($identifier) . '.json';
**Fix:** Store uploads outside the webroot and serve through a controller:
$data = file_exists($file) ? json_decode(file_get_contents($file), true) : [];
```php
$uploadBaseDir = '/var/www/posterg/storage/theses/' . $annee . '/' . $identifier . '/';
$coverDir = '/var/www/posterg/storage/covers/';
```
// Clean old entries
$now = time();
$data = array_filter($data, function($timestamp) use ($now, $timeWindow) {
return ($now - $timestamp) < $timeWindow;
});
---
// Check if limit exceeded
if (count($data) >= $maxRequests) {
return false;
}
### 4. File Path Mismatch — Media Files Cannot Be Served to Public
// Add new request
$data[] = $now;
file_put_contents($file, json_encode($data));
**Files:** `formulaire.php` (stores), `memoire.php` and `search.php` (reference)
return true;
}
Files are stored at `public/admin/actions/data/theses/YEAR/ID/file.ext` but recorded
in the database as `data/theses/YEAR/ID/file.ext`. When a browser loads `/memoire.php`,
relative links resolve to `/data/theses/…`, which nginx blocks:
// In search.php:
$userIP = $_SERVER['REMOTE_ADDR'];
if (!checkRateLimit($userIP, 20, 60)) { // 20 requests per minute
http_response_code(429);
die('Too many requests. Please try again later.');
```php
// memoire.php — tries to embed a URL that nginx denies
<embed src="<?= htmlspecialchars($file['file_path']); ?>" type="application/pdf">
```
```nginx
location ^~ /data/ {
deny all; # ← This kills the public file references
}
```
### Priority 3: Add Content Security Policy (LOW)
This is both a security misconfiguration and a **functional bug** (files never display
on the public site).
**Fix:** Use a consistent, intentionally accessible path (e.g., `/var/www/posterg/storage/`)
and serve files through a dedicated PHP endpoint that validates access rights.
---
### 5. Rate Limiter Vulnerable to IP Spoofing
**File:** `lib/RateLimit.php` — method `getClientIdentifier()`
Add to header:
```php
header("Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline' cdn.jsdelivr.net;");
header("X-Content-Type-Options: nosniff");
header("X-Frame-Options: DENY");
header("X-XSS-Protection: 1; mode=block");
if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
$ip = $_SERVER['HTTP_X_FORWARDED_FOR']; // ← Fully attacker-controlled
} elseif (!empty($_SERVER['HTTP_CLIENT_IP'])) {
$ip = $_SERVER['HTTP_CLIENT_IP']; // ← Also attacker-controlled
}
```
### Priority 4: Add Query Logging (LOW)
Any attacker can set `X-Forwarded-For: 1.2.3.4` to a new IP on every request,
completely bypassing the rate limiter. The `/search` endpoint has no other
brute-force protection.
**Fix:** Use `$_SERVER['REMOTE_ADDR']` only. If behind a trusted reverse proxy,
whitelist the proxy and extract the last trusted IP from the forwarded header:
Log suspicious search patterns:
```php
// Detect potential attacks
if (preg_match('/[%_]{10,}/', $params['query'])) {
error_log("Suspicious search pattern from {$_SERVER['REMOTE_ADDR']}: {$params['query']}");
private function getClientIdentifier(): string {
// Only trust REMOTE_ADDR unless explicitly behind a known proxy
return $_SERVER['REMOTE_ADDR'] ?? 'unknown';
}
```
---
## Security Best Practices Checklist
### 6. `.htaccess` Security Controls Silently Ignored by nginx
- [x] Use prepared statements (SQL injection)
- [x] Escape output with htmlspecialchars() (XSS)
- [ ] Escape LIKE wildcards (wildcard injection)
- [ ] Validate input lengths (DoS)
- [ ] Implement rate limiting (DoS)
- [ ] Validate pagination limits (resource waste)
- [x] Restrict to published data only (access control)
- [ ] Add security headers (defense in depth)
- [ ] Log suspicious activity (monitoring)
- [ ] Use HTTPS in production (encryption)
**File:** `public/admin/.htaccess`
This file contains X-Frame-Options, CSP, X-Content-Type-Options, `Options -Indexes`,
and file-protection rules — all **Apache-specific directives**. The production server
runs nginx, which never reads `.htaccess`. None of these controls are active.
```apache
Header always set Content-Security-Policy "..." # ← Dead letter on nginx
Options -Indexes # ← Never applied
<FilesMatch "(error\.log)$"> # ← nginx doesn't parse this
Require all denied
```
**Fix:** Move all security rules into the nginx `server` block directly.
---
## Testing Security
## 🟡 MEDIUM
### Test 1: SQL Injection
```bash
# These should NOT cause errors or expose data
curl "search.php?query=' OR 1=1--"
curl "search.php?query='; DROP TABLE theses;--"
curl "search.php?year=' OR '1'='1"
```
**Expected:** Treated as literal search strings, no SQL execution
### 7. LIKE Wildcard Injection in Admin Search
### Test 2: XSS
```bash
curl "search.php?query=<script>alert('XSS')</script>"
```
**Expected:** Script tags displayed as text, not executed
**File:** `public/admin/index.php` (lines ~3842)
### Test 3: Wildcard Injection
```bash
curl "search.php?query=%"
```php
$searchParam = "%$searchQuery%"; // ← % and _ characters not escaped
$params[] = $searchParam;
```
**Current:** Returns all results ❌
**After fix:** Searches for literal "%" character ✅
### Test 4: DoS via Long Input
```bash
curl "search.php?query=$(python3 -c 'print("a"*100000)')"
While SQL injection is prevented by prepared statements, unescaped `%` and `_`
trigger unintended SQL wildcard matching (e.g., `%` matches every row, `_` matches
any single character). The public `Database::searchTheses()` already does this
correctly — the admin search does not.
**Fix:**
```php
$escaped = str_replace(['\\', '%', '_'], ['\\\\', '\\%', '\\_'], $searchQuery);
$searchParam = "%" . $escaped . "%";
```
**Current:** Processes full string ❌
**After fix:** Rejects with error ✅
---
## Conclusion
### 8. Session Cookies Not Hardened
**Current Status:** The search system has **good baseline security** against SQL injection and XSS, but needs hardening for production use.
**Files:** All admin PHP files using `session_start()`
**Recommended Actions:**
1. Apply wildcard escaping (use `Database_secure.php`)
2. Add input length validation
3. Implement rate limiting
4. Add security headers
5. Monitor for suspicious patterns
No secure cookie parameters are configured before `session_start()`:
**Risk Level:**
- Current: **Medium** (suitable for internal/development use)
- After improvements: **Low** (production-ready)
- No `Secure` flag → cookies sent over plain HTTP
- No `HttpOnly` flag → accessible to JavaScript (XSS cookie theft)
- No `SameSite` → vulnerable to cross-site request forgery via cookies
**Fix:** Add before every `session_start()`:
```php
session_set_cookie_params([
'lifetime' => 0,
'path' => '/admin',
'secure' => true,
'httponly' => true,
'samesite' => 'Strict',
]);
session_start();
```
---
### 9. Error Log in Potentially Web-Accessible Path
**File:** `public/admin/actions/formulaire.php`
```php
ini_set('error_log', 'error.log'); // Relative path → public/admin/actions/error.log
```
The nginx config denies `.md`, `.txt`, `.sql`, `.sh`, `.json` files — but **not
`.log` files**. The `.htaccess` rule blocking `error.log` is ignored by nginx (see #6).
Error logs can contain PHP stack traces, internal file paths, and database details.
**Fix:** Use an absolute path outside the webroot:
```php
ini_set('error_log', '/var/log/posterg/error.log');
```
---
### 10. External CDN Stylesheet Without Subresource Integrity (SRI)
**File:** `public/admin/inc/head.php`
```html
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/water.css@2/out/water.css">
```
If the CDN is compromised, attackers can inject malicious CSS into the admin panel.
CSS-based data exfiltration (capturing form inputs via attribute selectors) is a
known attack vector.
**Fix:** Pin the resource with an integrity hash:
```html
<link rel="stylesheet"
href="https://cdn.jsdelivr.net/npm/water.css@2/out/water.css"
integrity="sha256-<computed-hash>"
crossorigin="anonymous">
```
Generate the hash: `curl -s https://cdn.jsdelivr.net/npm/water.css@2/out/water.css | openssl dgst -sha256 -binary | base64`
---
### 11. Missing Content Security Policy on Public Pages
**File:** `nginx/posterg.conf`
The nginx config sets `X-Frame-Options`, `X-Content-Type-Options`, etc., but defines
**no `Content-Security-Policy`** header for public routes. This leaves the public
site without a last line of defence if any output escaping is ever missed.
**Fix:** Add to the main `server` block in nginx:
```nginx
add_header Content-Security-Policy
"default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; object-src 'none'; frame-ancestors 'none';"
always;
```
---
### 12. CSV Import Without Server-Side MIME Validation
**File:** `public/admin/import.php`
The cover image upload validates MIME type with `finfo`, but the CSV upload does not:
```php
// Only HTML-side restriction — trivially bypassed:
<input type="file" id="csv_file" name="csv_file" accept=".csv" required>
```
**Fix:**
```php
$finfo = new finfo(FILEINFO_MIME_TYPE);
$mime = $finfo->file($_FILES['csv_file']['tmp_name']);
if (!in_array($mime, ['text/plain', 'text/csv', 'application/csv'])) {
throw new Exception("Type de fichier invalide. Seuls les fichiers CSV sont acceptés.");
}
```
---
## 🔵 LOW
### 13. Deprecated `X-XSS-Protection` Header
**Files:** `nginx/posterg.conf`, `public/admin/.htaccess`
`X-XSS-Protection "1; mode=block"` has been removed from Chrome and is ignored by
Firefox. In some legacy browsers it can actually *introduce* reflected-XSS
vulnerabilities by blocking legitimate content.
**Fix:** Remove the header. Rely on a properly configured CSP instead.
---
### 14. Missing `rel="noreferrer"` on External Links
**File:** `public/admin/thanks.php`
```php
<a href="..." target="_blank" rel="noopener">
```
`noopener` prevents `window.opener` hijacking, but `noreferrer` is also needed to
prevent the internal admin URL from leaking in the HTTP `Referer` header to external
sites.
**Fix:** Use `rel="noopener noreferrer"` on all `target="_blank"` links.
---
### 15. Unescaped Integer Outputs (Cosmetic / Defence in Depth)
**Files:** `public/index.php`, `public/search.php`
```php
<?= $item["id"] ?> // integer from DB
<?= $year; ?> // integer from DB
<?= $totalItems; ?> // integer from COUNT()
```
Safe in practice (integers cannot carry XSS payloads), but inconsistent with the
rest of the codebase which uses `htmlspecialchars()` everywhere.
**Fix:** Apply explicit integer cast for clarity: `<?= (int) $item['id'] ?>`.
---
### 16. Redundant `DATABASE_PATH` Constant
**File:** `config/bootstrap.php`
```php
define('DATABASE_PATH', APP_ROOT . '/database/test.db');
```
This constant is never used anywhere. `Database.php` uses `getDatabasePath()` from
`lib/config.php`. The duplicate creates confusion about which configuration is
authoritative and could lead to future bugs if someone uses the wrong one.
**Fix:** Remove the `DATABASE_PATH` define from `bootstrap.php`.
---
## Summary Table
| # | Issue | Severity | File(s) |
|---|-------|----------|---------|
| 1 | No HTTPS — admin credentials exposed in transit | 🔴 CRITICAL | nginx/posterg.conf |
| 2 | No PHP-level authentication in admin | 🔴 CRITICAL | public/admin/\*.php |
| 3 | Uploaded files stored inside webroot | 🟠 HIGH | admin/actions/formulaire.php |
| 4 | File path mismatch — media broken & insecure | 🟠 HIGH | formulaire.php, memoire.php |
| 5 | Rate limiter bypassed by IP spoofing | 🟠 HIGH | lib/RateLimit.php |
| 6 | .htaccess rules ignored by nginx | 🟠 HIGH | public/admin/.htaccess |
| 7 | LIKE wildcard injection in admin search | 🟡 MEDIUM | public/admin/index.php |
| 8 | Session cookies not hardened | 🟡 MEDIUM | All admin PHP files |
| 9 | error.log in web-accessible path | 🟡 MEDIUM | admin/actions/formulaire.php |
| 10 | CDN stylesheet without SRI | 🟡 MEDIUM | admin/inc/head.php |
| 11 | Missing CSP on public pages | 🟡 MEDIUM | nginx/posterg.conf |
| 12 | CSV upload missing MIME validation | 🟡 MEDIUM | admin/import.php |
| 13 | Deprecated X-XSS-Protection header | 🔵 LOW | nginx/posterg.conf |
| 14 | Missing rel="noreferrer" on external links | 🔵 LOW | admin/thanks.php |
| 15 | Unescaped integers (defence in depth) | 🔵 LOW | index.php, search.php |
| 16 | Redundant DATABASE_PATH constant | 🔵 LOW | config/bootstrap.php |
---
*End of report.*