Sicherer Datei-Upload nur für PDF
Ich versuche, mit PHP 7+ einen sicheren Datei-Upload zu erstellen, bei dem nur PDF-Dateien zulässig sind . Ich habe viele Beiträge zu diesem Thema auf verschiedenen Websites gefunden, konnte jedoch keine vollständige Lösung finden, die sicherstellt, dass auf diese Weise keine schädlichen Dateien hochgeladen werden können.
Bisher habe ich folgenden Code. Kann mir jemand sagen, ob mir hier wichtige Schritte fehlen oder ob etwas in meinem Code geändert oder entfernt werden sollte?
( Hinweis: Die alten x-pdf-Dateitypen interessieren mich nicht.)
<?php
include 'session.php';
include 'header.php';
if (empty($_FILES['files'])) { echo json_encode(['error'=>'No files found for upload.']); return; } if(!empty($_POST['csrfToken'])) {
if(hash_equals($_SESSION['csrfToken'], $_POST['csrfToken'])) {
$postData = $_POST;
$files = $_FILES['files'];
$uploadRef = preg_replace('/[^A-Za-z0-9]/', '', $_GET['uploadRef']);
$categoryId = preg_replace('/[^A-Za-z0-9]/', '', $_GET['categoryId']);
$tags = preg_replace('/[^A-Za-z0-9,]/', '', $_GET['tagsList']);
$success = null; $paths= [];
$filenames = $files['name'];
for($i=0; $i < count($filenames); $i++){
if($_FILES['file']['error'] !== UPLOAD_ERR_OK) { die('Upload failed with error ' . $_FILES['file']['error']);
}
$fileTitle = $files['name'][$i]; $fileTitle = substr($fileTitle, 0 , (strrpos($fileTitle, ".")));
$fileExtensions = explode('.', basename($filenames[$i])); $fileExtension = strtolower(array_pop($fileExtensions)); $ok = false;
switch($fileExtension) { case 'pdf': $ok = true;
default:
die('Unknown/not permitted file type');
}
$finfo = finfo_open(FILEINFO_MIME_TYPE); $mime = finfo_file($finfo, $_FILES['file']['tmp_name']);
$ok = false; switch($mime) {
case 'application/pdf':
$ok = true; default: die('Unknown/not permitted file type'); } $uploadId = md5(uniqid()) . '_' . $i; $target = 'uploads' . DIRECTORY_SEPARATOR . $uploadId . '.' . $fileExtension;
if(move_uploaded_file($files['tmp_name'][$i], $target)) { $success = true;
$paths[] = $target;
$conn = new mysqli($dbHost, $dbUser, $dbPw, $dbName); if($conn->connect_error) {
exit($trans['errorConnectionFailedTxt'][$lang]);
}
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$conn->set_charset('utf8mb4'); $stmt = $conn->prepare("INSERT INTO uploads (uploadId, uploadRef, categoryId, tags, fileTitle, fileExtension) VALUES (?, ?, ?, ?, ?, ?)"); $stmt->bind_param("ssssss", $uploadId, $uploadRef, $categoryId, $tags, $fileTitle, $fileExtension);
$stmt->execute(); $stmt->close();
$conn->close(); } else { $success = false;
break;
}
}
if ($success === true) { $output = [];
} elseif ($success === false) { $output = ['error'=>'Error while uploading images. Contact the system administrator'];
foreach ($paths as $file) {
unlink($file); } } else { $output = ['error'=>'No files were processed.'];
}
unset($postData); echo json_encode($output);
} else {
echo json_encode('invalid CSRF token');
}
} else {
echo json_encode('no CSRF token');
}
?>
Antworten
Wachaussagen
if(!empty($_POST['csrfToken'])) { if(hash_equals($_SESSION['csrfToken'], $_POST['csrfToken'])) {
Ich denke, dass dies in einer separaten Methode durchgeführt werden sollte, die Token validiert. Darüber hinaus denke ich, dass dies in einem HTTP-Fehler enden sollte (wie in der anderen Antwort angegeben).
Oben wird eine "Guard-Anweisung" genannt, die prüft, ob der Status oder Parameter gültig ist, bevor die Methode ausgeführt wird. Wenn es nicht gültig ist, sollte es beendet werden. Die Beendigung kann lokal durchgeführt werden. Sie möchten definitiv nicht durch die Methode scrollen müssen, um Folgendes zu finden:
} else {
echo json_encode('invalid CSRF token');
}
} else {
echo json_encode('no CSRF token');
}
Am Ende. Wenn Sie die Methode einfach beenden, entfernen Sie außerdem eine Einrückungsebene, wodurch das Lesen der Methode weniger komplex wird.
Unklare reguläre Ausdrücke
$uploadRef = preg_replace('/[^A-Za-z0-9]/', '', $_GET['uploadRef']);
Dies sollte auch eine Methode sein, nicht so sehr, weil der reguläre Ausdruck schwer zu verstehen ist (das "Was"), aber es ist nicht klar, warum er ausgeführt wird.
Erfolg?
$success = null;
Erfolg ist ein Boolescher Wert, er sollte nicht als Variable mit drei Werten verwendet werden. Verwenden Sie entweder zwei Variablen oder eine Aufzählung. Darüber hinaus $successist ein schrecklicher schlechter Name, versuchen filesUploadedoder ähnliches.
Wenn wir vorher prüfen würden, ob count($filenames)Null ist, können wir einfach die Ausgabe vorher einstellen und den Rest der Ausführung überspringen (denken Sie an die Guard-Anweisungen). Bei der Programmierung geht es darum, die Komplexität zu begrenzen.
C-Programmierer wiederherstellen
$ok = false;
Nicht erforderlich, wenn Sie diesowieso, oder?
Nicht so einzigartig
uniqid()
"Diese Funktion garantiert keine Eindeutigkeit des Rückgabewerts." 1 Ähm, richtig. Dies wartet nur darauf, schrecklich zu scheitern, was auch immer Sie damit machen. Mit md5()an wird es nicht erreichen nichts . Eine Erweiterung mit einem Zähler hilft, es sei denn, derselbe Ordner wird von parallelen Prozessen verwendet (ist das möglich?).
Die Fortsetzung
Die SQL-Anweisungen sollten in einer separaten Methode vorliegen, z createFileUploadReport. Wenn Sie eine separate Klasse erstellen, können Sie sogar verschiedene Arten der Berichterstellung verwenden, z. B. die Berichterstellung an die Konsole oder die Protokolldatei, sodass Sie Ihre Methode ohne vorhandenen SQL Server testen können.
Fehler / Logikfehler
Die beiden switchAnweisungen haben einen Fall plus den Standardfall. In beiden switch-Anweisungen wird der defaultFall immer ausgeführt. Vielleicht wäre ein besseres Verständnis der Funktionsweise von switchAussagen hilfreich. Für einen Einzelfall ist es auch nicht sinnvoll, eine switchAnweisung zu verwenden - eine einfache ifAnweisung würde ausreichen.
Inkonsistente Rückgabetypen
In einigen Fällen wird ein Array an json_encode()eine echoAnweisung übergeben und mit dieser verwendet , in anderen Fällen die()wird sie aufgerufen. Die json-codierten Arrays lassen mich glauben, dass dieses Skript in Verbindung mit einem asynchronen Lademechanismus (z. B. AJAX) verwendet wird, aber das würde wahrscheinlich verworfen, wenn die()oder wenn exit()es verwendet wird, es sei denn, es sucht sowohl nach Arrays als auch nach einfachen Zeichenfolgen.
Dann, anscheinend, wenn der Upload erfolgreich ist, gibt es diesen Code:
if ($success === true) { $output = [];
Das scheint für den Front-End-Code nicht sehr hilfreich zu sein.
Es wäre auch ratsam, HTTP-Antwortcodes zu verwenden - z. B. 200, um den Erfolg anzuzeigen, 422 für ungültige Eingaben usw.
früh zurückkehren
Wenn die erste Bedingung, dh if (empty($_FILES['files'])) {bis zu , ausgewertet wird, truewird die JSON-Antwort mit der entsprechenden Fehlermeldung zurückgegeben. Die anderen Bedingungen, die zu Fehlermeldungen führen, sollten auf die gleiche Weise erfolgen, z. B. leerer Wert in $_POST['csrfToken']usw. Dadurch wird die Einrückung für den Rest des Codes verringert. Weitere Informationen zu diesem Thema finden Sie in dieser Präsentation zum Bereinigen von Code, in der Rafael Dohms über viele Möglichkeiten spricht, Code schlank zu halten - beispielsweise das Vermeiden des elseSchlüsselworts ( siehe die Folien hier ).
Da dies nicht von der Schleifeniteratorvariablen abhängt, kann dieser Block über die forSchleife verschoben werden :
if($_FILES['file']['error'] !== UPLOAD_ERR_OK) { die('Upload failed with error ' . $_FILES['file']['error']); }
Überprüfen des MIME-Typs
Anstatt finfo_open(FILEINFO_MIME_TYPE)nur aufzurufen , um den MIME-Typ zu erhalten, könnte die mime-content-type()Funktion verwendet werden.
$mime = mime-content-type($_FILES['file']['tmp_name'])
Darüber hinaus kann der MIME-Typ vom Browser bereitgestellt werden, $_FILES['files']['type'][$i]obwohl er " auf der PHP-Seite nicht aktiviert ist und daher seinen Wert nicht als selbstverständlich ansieht ". 1
variabel $postData- wirklich gebraucht?
Nachdem postDataes zugewiesen wurde, scheint es nur an einer Stelle verwendet zu werden, an die es übergeben wurde unset(). Es scheint kaum notwendig ...
Variadische Feldnamenprüfungen
Obwohl das an dieses Skript gesendete Formular nicht enthalten ist, kann man nur die Felder erraten. Das Skript prüft sowohl $_FILES['files']als auch $_FILES['file']- während das erstere höchstwahrscheinlich das Hochladen mehrerer Dateien ermöglicht, gibt es wirklich zwei verschiedene Dateieingabefelder?
Datenbankabfrage in Schleife
Der Code fügt derzeit bei jeder Iteration der Schleife Datensätze in die Datenbank ein. Verwenden Sie nur eine Anweisung, um alle Datensätze einzufügen. Dies minimiert Datenbankverbindungen (was die Ausführungszeit verlängern kann) und verhindert, dass ungültige Daten eingefügt werden (z. B. wenn die erste Datei gültig war, nachfolgende Dateien jedoch nicht).
reguläre Ausdrücke zum Ersetzen von Zeichen
Die regulären Ausdrücke könnten möglicherweise unter Verwendung des Zeichentyps vereinfacht werden, \wobwohl dies die Unterstriche enthält, dh _müssen diese entfernt werden? Auch nicht, dass preg_replace()Multi-Byte-Zeichenfolgen nicht sicher bereinigt werden. Denken Sie also daran, wenn Unicode-Zeichenfolgen unterstützt werden müssen.