Dateiupload

[Tom]

Hello,

Hallo Tom,

hier also der bereinigte Code
[code lang=php]
<?
@session_start();

/************************************************************************************************/
if (($_SESSION['myusername'] == "") OR ($_SESSION['mypassword'] == "")) {
include ("login.php");
}
/************************************************************************************************/

if (empty($_SESSION['myusername'] == "") OR empty($_SESSION['mypassword'])
{
    include ("login.php");
    exit;
}

include("./include/db_vars.php");
include("./include/db_connect.php");

$speicherdatum = date("d. M Y - H:i:s");
$unix_time = time();
$updir = "..";
$dir = "shop";
$subdir = "articles";

if(isset($_POST['submit']) && $_POST['submit']=="hochladen"){

hier zuerst auf Fehler prüfen:

achte auf den Identitätsvergleich mit ===

if ($_FILES['userfile']['error'] === UPLOAD_ERR_OK)
{

dann die weiteren Prüfungen

($_FILES['userfile']['tmp_name'])

tmp_name kann nicht mehr leer sein, wenn der Error-Wert === 0 (UPLOAD_ERR_OK) ist

http://de.php.net/manual/en/features.file-upload.errors.php

if($_POST['artikelName'] == ""){
header("Location: article_upload_antw.php?flag=artikelName");
exit;
}

if($_POST['Preis'] != ""){

  if(is\_numeric($\_POST['Preis'])){  
  	 $\_POST['preis'] = intval($\_POST['Preis']);  
  }  
  else {  
  header("Location: article\_upload\_antw.php?flag=nonumeric");  
  exit;  
  }  

}

if($_POST['Preis'] == ""){
header("Location: article_upload_antw.php?flag=preis");
exit;
}

if($_POST['Versandkosten'] != ""){

  if(is\_numeric($\_POST['Versandkosten'])){  
  	 $\_POST['Versandkosten'] = intval($\_POST['Versandkosten']);  
  }  
  else {  
  header("Location: article\_upload\_antw.php?flag=nonumeric");  
  exit;  
  }  

}

if($_POST['Versandkosten'] == ""){
header("Location: article_upload_antw.php?flag=versandkosten");
exit;
}

if($_POST['Breite'] == ""){
header("Location: article_upload_antw.php?flag=breite");
exit;
}
if($_POST['Hoehe'] == ""){
header("Location: article_upload_antw.php?flag=hoehe");
exit;
}
if($_POST['Tiefe'] == ""){
header("Location: article_upload_antw.php?flag=tiefe");
exit;
}

Ich würde die Übertragung der Postparameter von der Übertragung des Bildes trennen, also einen eigenen Zyklus dafür aufbauen. Es ist dabei ziemlich egal, ob Du zuerst das Bild hochladen lässt, ein Thumbnail daraus berechnest und dann erst die Beschreibungsdaten dazu hochladen lässt, oder umgekehrt. Das ist nur eine Frage deiner Geschäftslogik. Alle Bilder ohne vollständige Beschreibung würden in einem Zwischenverzeichnis liegen bleiben, bis der User mit der Arbeit fertig ist. Beschreibungen ohne Bilder könnten doch aber eventuell zulässig sein, oder? Das könnte man parametrisieren in einer INI-Datei. Dazu müssten die Vorgangszüge aber getrennt werden...

else {
$_FILES['userfile']['name'] = str_replace("ä","ae",$_FILES['userfile']['name']);
$_FILES['userfile']['name'] = str_replace("ö","oe",$_FILES['userfile']['name']);
$_FILES['userfile']['name'] = str_replace("ü","ue",$_FILES['userfile']['name']);

  if (file\_exists($updir."/".$dir."/".$subdir."/".mysql\_real\_escape\_string($\_FILES['userfile']['name']))) {  
  	header("Location: article\_upload\_antw.php?flag=already");  
  	exit;  
  }  

mysql_real_escape_string() hat hier nichts zu suchen. Das behandelt das Argument nur genau für die SQL-Textschnittstelle, da diese einen gemischten Daten- und Befehlsstrom verwendet.

Für das Dateisystem ist es viel wesentlicher, dass $_FILES['userfile']['name'] darauf geprüft wird, ob es nur einen Namen und auf gar keinen Fall einen (unerwünschten) Pfadanteil (auch '../../../' o.ä.) enthält. Dafür ist die Funktion basename() brauchbar.
http://de.php.net/manual/en/function.basename.php

  if (!@move\_uploaded\_file($\_FILES['userfile']['tmp\_name'], $updir."/".$dir."/".$subdir."/".mysql\_real\_escape\_string($\_FILES['userfile']['name']))){  
  	header("Location: article\_upload\_antw.php?flag=nosaved");  
  	exit;  
  }  

solange nicht geklärt ist, welchen Zusatznutzen move_uploaded_file() an dieser Stelle bietet, halte ich den Nachteil des TOCTTOU-Fehlers für überwiegend. Es wäre daher besser, das Kopieren der Daten generisch zu lösen und für das Eröffnen der neuen Datei im Zielverzeichnis

fopen($ziel, 'xb+')
    http://de.php.net/manual/en/function.fopen.php

Die notwendige Absicherung für das Quellfile liefert move_uploaded_file() mMn nicht. Bei Verwendung von $_FILES['tmp_name'] kann es sich nur um ein hochgeladenes File handeln. Dieses könnte aber immer noch durch andere User manipuliert worden sein, wenn man nicht daran gedacht hat, 'upload_tmp_dir' auf einen eigenen, geschützten Bereich umzuschalten. Eben diese Absicherung, die durch eine Hashbildung des DateiINHALTES beim Serverupload möglich wäre, wird aber (vermutlich) gerad nicht vorgenommen. Der Hash wird nur über den Dateinamen (lt. Martin) gebildet. Dieser ist aber durch das gesicherte Array-Element $_FILES['tmp_name'] schon abgesichert.

Hier habe ich erstmal aufgehört, weiterzulesen.

Die Übertragung der Daten aus der Session in die MySQL-Tabelle ist ein eigener Vorgangszug, der erst dann durchgeführt werden sollte, wenn die beiden anderen (Bildupload, Beschreibungsdaten) ordnungsgemäß durchgeführt worden sind. Erst dann sollte auch das Bild vom persistenten Zwischenspeicher des Users in den allgemeinen Bildspeicher des Systems überführt werden.

Wenn Du den Vorgang in drei Teile (Vorgangszüge) zerlegst (zuzüglich Bildverschiebung von USER nach ALLGEMEIN), dann sollte das eine sehr flexible und sichere Lösung werden.

Liebe Grüße aus dem schönen Oberharz

Tom vom Berg