[SECURITY] Perbaikan & Hardening Validasi File Upload untuk Cegah Webshell/RCE#984
[SECURITY] Perbaikan & Hardening Validasi File Upload untuk Cegah Webshell/RCE#984pandigresik wants to merge 3 commits intorilis-devfrom
Conversation
🤖 AI Code Review — PR #984📊 Overall AssessmentNEEDS CHANGES — PR ini menambahkan security improvements yang solid untuk file upload, namun terdapat 3 isu CRITICAL dan 2 isu HIGH yang harus diperbaiki sebelum merge. Isu utama: missing authorization checks, IDOR vulnerabilities, dan race conditions yang bisa menyebabkan data loss. 🔴 Critical Issues (3)
🟠 High Issues (2)
🟡 Medium Issues (4)
✅ Yang Sudah Baik
📋 Summary per Area
🧪 Missing Tests
|
| * Determine if the user is authorized to make this request. | ||
| * | ||
| * @return bool | ||
| */ |
There was a problem hiding this comment.
[CRITICAL] Security: Missing Authorization Check
Masalah: Method authorize() selalu return true, artinya TIDAK ADA authorization check sama sekali. Semua user (bahkan unauthenticated) bisa upload gambar artikel.
Risiko/Dampak:
- Unauthenticated users bisa upload file ke server
- Resource exhaustion via spam uploads
- Tidak ada rate limiting atau access control
- Attacker bisa mengisi disk dengan junk files
Fix:
| */ | |
| public function authorize(): bool | |
| { | |
| // Pastikan user terautentikasi dan punya permission upload artikel | |
| return $this->user() && $this->user()->can('upload-artikel-image'); | |
| } |
| // Convert to validation error so it appears in $errors | ||
| return redirect()->back() | ||
| ->withInput() | ||
| ->withErrors(['foto' => 'Gagal mengunggah foto: ' . $e->getMessage()]); |
There was a problem hiding this comment.
[CRITICAL] Security: Insecure Direct Object Reference (IDOR)
Masalah: Tidak ada authorization check sebelum update employee. User bisa mengubah data employee lain dengan mengganti ID di URL.
Risiko/Dampak:
- User biasa bisa update data admin dengan
PUT /employees/1 - Privilege escalation dengan mengubah jabatan/role
- Data integrity compromise
Fix:
| ->withErrors(['foto' => 'Gagal mengunggah foto: ' . $e->getMessage()]); | |
| public function update($id, UpdateEmployeeRequest $request): RedirectResponse | |
| { | |
| $employee = $this->employeeRepository->find($id); | |
| if (empty($employee)) { | |
| flash('Employee not found')->error(); | |
| return redirect(route('employees.index')); | |
| } | |
| // ✅ Add authorization check | |
| $this->authorize('update', $employee); | |
| $input = $request->all(); | |
| // ... rest of code | |
| } |
Jangan lupa buat EmployeePolicy dengan method update() dan delete().
|
📍 [CRITICAL] Bug: Race Condition Causing Data Loss Masalah: File thumbnail lama dihapus SEBELUM Risiko/Dampak:
Fix: |
| { | ||
| /** | ||
| * Dangerous patterns that indicate malicious content | ||
| * (Subset dari SecureImageUploadService, disesuaikan untuk generic files) |
There was a problem hiding this comment.
[HIGH] Security: Insufficient Archive File Validation
Masalah: File archive (ZIP, RAR, 7z) diterima tanpa deep inspection. Attacker bisa upload ZIP berisi PHP webshell atau malicious scripts.
Risiko/Dampak:
- Webshell tersembunyi dalam archive bisa di-extract dan dieksekusi
- Zip bomb attack untuk DoS (decompression bomb)
- Bypass semua validasi dengan menyembunyikan malicious file dalam archive
Fix:
| * (Subset dari SecureImageUploadService, disesuaikan untuk generic files) | |
| private function validateArchiveContent(UploadedFile $file): void | |
| { | |
| $zip = new \ZipArchive(); | |
| if ($zip->open($file->getRealPath()) !== true) { | |
| throw new RuntimeException('Unable to open archive for inspection.'); | |
| } | |
| $dangerousExtensions = ['php', 'phtml', 'php3', 'sh', 'py', 'pl', 'exe', 'bat']; | |
| for ($i = 0; $i < $zip->numFiles; $i++) { | |
| $stat = $zip->statIndex($i); | |
| $ext = strtolower(pathinfo($stat['name'], PATHINFO_EXTENSION)); | |
| if (in_array($ext, $dangerousExtensions, true)) { | |
| $zip->close(); | |
| throw new RuntimeException('Archive contains dangerous file types.'); | |
| } | |
| // Check zip bomb (compression ratio < 1%) | |
| if ($stat['size'] > 0 && ($stat['comp_size'] / $stat['size']) < 0.01) { | |
| $zip->close(); | |
| throw new RuntimeException('Potential zip bomb detected.'); | |
| } | |
| } | |
| $zip->close(); | |
| } |
Panggil method ini di upload() sebelum save file.
|
📍 [HIGH] Performance: N+1 Query Problem Masalah: Relasi Risiko/Dampak:
Fix: Atau buat method |
| return $mimeType ?: 'application/octet-stream'; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[CRITICAL] Bug: Potential Null Dereference
Masalah: finfo_open() bisa return false jika fileinfo extension tidak tersedia atau gagal initialize. Jika ini terjadi, finfo_file() akan crash dengan fatal error.
Risiko/Dampak:
- Entire upload process crash dengan 500 error
- Semua user tidak bisa upload file
- Production downtime jika fileinfo extension bermasalah
Fix:
| /** | |
| $finfo = finfo_open(FILEINFO_MIME_TYPE); | |
| if ($finfo === false) { | |
| throw new RuntimeException('Failed to initialize file info detector. Please check fileinfo extension.'); | |
| } | |
| $mime = finfo_file($finfo, $file->getRealPath()); | |
| finfo_close($finfo); |
| $maxKb = $this->maxFileSize / 1024; | ||
| throw new RuntimeException("File size exceeds maximum allowed size of {$maxKb} KB"); | ||
| } | ||
|
|
There was a problem hiding this comment.
[CRITICAL] Bug: Empty File Bypass
Masalah: File dengan size 0 bytes lolos validasi dan masuk ke proses re-encoding, yang akan gagal dengan error tidak jelas.
Risiko/Dampak:
- Server resources terbuang untuk memproses file kosong
- Error message tidak informatif untuk user
- Potential crash di image processing step
Fix:
| private function validateFileSize(UploadedFile $file, int $maxSizeKb): void | |
| { | |
| if ($file->getSize() === 0) { | |
| throw new RuntimeException('File is empty (0 bytes). Please upload a valid file.'); | |
| } | |
| if ($file->getSize() / 1024 > $maxSizeKb) { | |
| throw new RuntimeException("File size exceeds maximum allowed size of {$maxSizeKb}KB"); | |
| } | |
| } |
|
📍 [CRITICAL] Bug: Unhandled Exception in Validator Masalah: Semua validation methods throw Risiko/Dampak:
Fix: |
|
📍 [HIGH] Performance: Missing Default Pagination Masalah: Method Risiko/Dampak:
Fix: Atau lebih baik, gunakan |
| } | ||
|
|
||
| /** | ||
| * Validate and process uploaded image securely |
There was a problem hiding this comment.
[CRITICAL] Security: Path Traversal Risk
Masalah: Parameter $destinationPath tidak divalidasi dengan whitelist. Jika ada bug di controller yang memungkinkan user control path, attacker bisa upload ke directory arbitrary.
Risiko/Dampak:
- Upload file ke
../../config/atau directory sensitif lainnya - Overwrite file sistem penting
- Bypass nginx security rules
Fix:
| * Validate and process uploaded image securely | |
| private function sanitizeDestinationPath(string $path): string | |
| { | |
| // Remove path traversal attempts | |
| $path = str_replace(['../', '..\\', './'], '', $path); | |
| // Whitelist allowed upload directories | |
| $allowedPaths = [ | |
| 'uploads/artikel', | |
| 'uploads/identitas', | |
| 'uploads/download', | |
| 'uploads/profile', | |
| 'uploads/slide', | |
| 'uploads/employee', | |
| ]; | |
| if (!in_array($path, $allowedPaths, true)) { | |
| throw new RuntimeException('Invalid upload destination path.'); | |
| } | |
| return $path; | |
| } | |
| public function upload(UploadedFile $file, string $destinationPath, string $outputFormat = 'jpg', int $maxSizeKb = 2048): string | |
| { | |
| // Validate path first | |
| $destinationPath = $this->sanitizeDestinationPath($destinationPath); | |
| // ... rest of validation | |
| } |
2 participants
Perbaikan issue #959
🎯 Executive Summary
Comprehensive file upload security has been implemented to prevent webshell uploads, arbitrary file execution, and other upload-based attacks. The implementation includes multi-layer validation, server-side image processing, centralized security services, and upload directory hardening.
Key Achievements
🚨 Security Issues Addressed
finfo_file()content detection🏗️ Architecture
Service Layer Architecture
Validation Flow
For Images (JPG/PNG/GIF)
For Generic Files (PDF/DOC/XLS/ZIP)
📁 Files Created
Core Services
app/Services/SecureImageUploadService.phpapp/Services/GenericFileUploadService.phpSecurity Configurations
storage/app/public/uploads/.htaccessnginx_uploads_security.confDocumentation
docs/SECURE_FILE_UPLOAD.mddocs/UPLOAD_SECURITY_AUDIT_REPORT.mddocs/SECURE_UPLOAD_QUICK_REFERENCE.mddocs/UPLOAD_SECURITY_REFACTORING.mddocs/UPLOAD_SERVICES_ARCHITECTURE.mdTests
tests/Feature/SecureUploadTest.phptests/Feature/DownloadControllerCmsTest.php📝 Files Modified
Controllers
app/Http/Controllers/Master/ArtikelUploadController.phpapp/Http/Controllers/Api/IdentitasController.phpapp/Http/Controllers/CMS/DownloadController.phpTraits
app/Traits/UploadedFile.phpFormRequests
app/Http/Requests/ArtikelImageRequest.phpapp/Http/Requests/UploadImageRequest.phpModels
app/Models/Employee.phpapp/Models/CMS/Download.phpProviders
app/Providers/AppServiceProvider.php🛡️ Security Features
SecureImageUploadService
Patterns Detected (40+):
<?php,<?<script,javascript:eval(),exec(),system(),shell_exec(), etc.<html,<body,<iframe,<formValidations:
finfo_file()getimagesize()GenericFileUploadService
Patterns Detected (9):
<?php,<?<script,javascript:eval(),system(),exec(),shell_exec(), etc.__halt_compilerValidations:
✅ Test Results
All Upload-Related Tests
Security Test Coverage
test_rejects_spoofed_file_extensiontest_get_real_mime_type_ignores_extensiontest_rejects_file_with_php_codetest_rejects_file_with_script_tagstest_rejects_webshell_signaturestest_rejects_dangerous_functionstest_rejects_null_byte_injectiontest_rejects_file_exceeding_max_sizetest_rejects_empty_filetest_rejects_corrupted_imagetest_reencoding_strips_metadata🚀 Deployment Instructions
1. Apache Servers
The
.htaccessfile is automatically applied:2. Nginx Servers
Add the security configuration:
3. Verify Installation
4. Clear Caches
📖 Usage Examples
Image Upload (via Trait)
Generic File Upload (via Trait)
Direct Service Usage
📊 Performance Impact
Note: Performance impact is acceptable given critical security improvements.
🔐 Security Recommendations
Immediate Actions
✅ Compliance
This implementation addresses:
🎯 Conclusion
The file upload security implementation provides comprehensive protection against:
Test upload pada artikel