[PHP] Is deze switch functie wel veilig?

[Flaterik]

Voor een website opdracht gebruik ik onderstaande code om content aan te roepen binnen een pagina. Op deze manier heb ik voor diverse onderdelen niet steeds een nieuwe pagina nodig maar zit alles in 1 .php pagina.

Ik heb echter in het verleden ook gebruik gemaakt van deze PHP functie en kan me herinneren dat deze wel eens gehackt werd. Ik wil dit nu voorkomen echter ben ik meer een designer dan van het programmeren.

code:

<?php
switch($item) {
    case "waarom":
        echo 
        ?> HTML code
<?php
        ;
        break;
    case "instructeurs":
        echo 
        ?> HTML code
<?php
        ;
        default:
        echo "";
        break;
}
?>

Ik roep dus via de url de 'case' aan en deze toon ik op de site. Na de echo sluit ik de php code zodat ik daar verder kan werken. Nu werkt dit allemaal prima echter zit ik dus met de beveiliging. Kan iemand aangeven of dit een veilige manier van werken is? En zo nee hoe ik het goed kan beveiligen?

[mithras]

Je veiligheid komt in gevaar wanneer user input ongewenst doorgevoerd kan worden (dus naar database, output, email etc). Met de switch baken jij zelf de mogelijkheden af: je kan kiezen voor a, b, c of de standaard waarde. In die zin is een switch veilig.

Een andere tip: include lekker je content bestandjes. Dat is een stuk gemakkelijker dan een groot bestand waar al je pagina's in zitten

[jbdeiman]

De Case is op zich niet echt te hacken hoor, dat is wel veilig. Het hacken is waarschijnlijk niet vanuit die switch geweest, maar wat anders.

Overigens:

De echo hoort daar niet (voor de afsluitende php tag) en je mist de break voor "default:"

[Verwijderd]

Op zich is switch gewoon een soort if / else dingetje. In plaats van alle code in een pagina te stoppen, lijkt het me lekkerder werken om zoals mithras zegt de bups te include.

Dan zou je zoiets kunnen doen.

PHP:

if(!isset($_GET['item']) || !file_exists("includemap/".$_GET['item'].".html")) include ("includemap/404.html");
else include ("includemap/".$_GET['item'].".html");

Dat is ook voor het onderhoud van je code enzo een stuk overzichtelijker.

[ Voor 5% gewijzigd door Verwijderd op 25-09-2009 11:05 . Reden: moeilijk he, quootjes ]

[--MeAngry--]

Ja, maar het voorbeeld van jou is meteen ook weer onveilig en haal je het hele nut van een switch() onderuit.

Daarnaast hoef je dus inderdaad geen echo te gebruiken voor het sluiten van de PHP tag. Als je gewoon HTML wilt weergeven tenminste...

[ Voor 4% gewijzigd door --MeAngry-- op 25-09-2009 11:08 ]

[DexterDee]

Verwijderd schreef op vrijdag 25 september 2009 @ 11:02:
Op zich is switch gewoon een soort if / else dingetje. In plaats van alle code in een pagina te stoppen, lijkt het me lekkerder werken om zoals mithras zegt de bups te include.

Dan zou je zoiets kunnen doen.

PHP:

if(!isset($_GET['item']) || !file_exists(includemap/".$_GET['item'].".html)) include ("includemap/404.html");
else include ("includemap/".$_GET['item'].".html");

Dat is ook voor het onderhoud van je code enzo een stuk overzichtelijker.

Dit moet je dus NOOIT doen. Met dit script kan ik elk bestand op het hele filesystem includen. Een enorm securityhole dus.
Je denk dat je veilig zit met het plakken van .html achter die querystring parameter, omdat dat de schijn geeft dat je hiermee alleen bestanden met die extensie kan includen. Niets is echter minder waar. PHP is geschreven in C en maakt gebruik van C-Strings. Door eenvoudigweg een CHR(0) aan het einde van de $_GET['item'] mee te geven, sluit PHP dus de string af en zal die .html erna negeren.

Voorbeeldje: www.server.com/scriptje.php?item=../../../etc/passwd%00

[Flaterik]

Oei... ik was net overgestapt op het script van Spijtbetuiging maar ik lees dat dat juist niet de veilig manier is..

[Verwijderd]

Ja, inderdaad. Dit was ff zo uit de losse pols. Je moet het wel ff veilig maken. Ik had ook niet verwacht dat Flaterik deze suggestie 1op1 zou overnemen, waarvoor excuus.

Ik vraag me trouwens wel even af hoe je hierom heen zou kunnen werken. Een arraytje maken met toegestane webpagina's waar die ff in moet kijken oid? Lijkt me nog steeds niet echt lekker.

[HuHu]

Flaterik schreef op vrijdag 25 september 2009 @ 11:13:
Oei... ik was net overgestapt op het script van Spijtbetuiging maar ik lees dat dat juist niet de veilig manier is..

Die is nogal slecht inderdaad. Beter voorbeeldje:

PHP:

$toegestaan = array('home', 'informatie', 'contact');
$item = isset($_GET['item']) ? $_GET['item'] : '';

if (in_array($item, $toegestaan)) {
  include 'bestanden/'. $item .'.html';
} else {
  echo 'De opgevraagde pagina bestaat niet.';
}

Je controleert dus eerst of de pagina die je wilt hebben wel is toegestaan. Zo ja, dan vraag je hem op in je pagina. Zo niet, dan toon je standaard-pagina (of een foutmelding).

[jbdeiman]

@huhu
Die van jou gaat ook niet goed. Want als er geen pagina is meegegeven (de home kom je op binnen, doorsturen naar index.php?item=home is ook niet handig)
Dus als jij gewoon "index.php" aanroept krijg je de melding dat de pagina niet bestaat. Handiger is om de default (die bij jou op een lege string staat) de waarde home bijvoorbeeld mee te geven.
Dan krijg je wanneer $_GET['item'] niet wordt meegegeven de home te zien.

[HuHu]

jbdeiman schreef op vrijdag 25 september 2009 @ 12:10:
@huhu
Die van jou gaat ook niet goed. Want als er geen pagina is meegegeven (de home kom je op binnen, doorsturen naar index.php?item=home is ook niet handig)
Dus als jij gewoon "index.php" aanroept krijg je de melding dat de pagina niet bestaat. Handiger is om de default (die bij jou op een lege string staat) de waarde home bijvoorbeeld mee te geven.
Dan krijg je wanneer $_GET['item'] niet wordt meegegeven de home te zien.

Waar zit de fout dan, als 'ie niet goed is? Hij is wel goed, alleen kan het misschien handiger door standaard iets anders dan een lege string mee te geven op regel 2. Maar dat maakt het nog niet fout of onveilig.

Standaard de home tonen is ook niet handig, want dan kun je geen onderscheid maken tussen een verzoek voor de home-pagina, of een foutieve pagina. Dus dan zou je uiteindelijk zoiets willen:

PHP:

$toegestaan = array('home', 'informatie', 'contact'); 
$item = isset($_GET['item']) ? $_GET['item'] : ''; 

if ($item === '') {
  header('Location: http://www.example.org/?item=home');
  exit;
}

if (in_array($item, $toegestaan)) { 
  include 'bestanden/'. $item .'.html'; 
} else { 
  header('HTTP/1.1 404 Not Found');
  echo 'De opgevraagde pagina bestaat niet.'; 
}

Maargoed, het is maar een voorbeeld. Ik ga er vanuit dat Flaterik zelf zoiets ook wel kan verzinnen.

[.oisyn]

jbdeiman heeft het erover dat je de '' op regel 2 vervangt door 'home'. Dan heb je jouw regel 4 t/m 7 ook niet meer nodig. Hij zegt niet dat als de pagina niet bestaat dat je dan gewoon 'home' moet kiezen.

[ Voor 25% gewijzigd door .oisyn op 25-09-2009 13:03 ]

[dwilmer]

Dan zou deze hem dus moeten worden:

PHP:

<?php
$toegestaan = array('home', 'informatie', 'contact');
$item = isset($_GET['item']) ? $_GET['item'] : 'home'; // Als $_GET['item'] niet is gegeven, naar home gaan

if (in_array($item, $toegestaan)) {
    include 'bestanden/'. $item .'.html';
} else {
    echo 'De opgevraagde pagina bestaat niet.';
}
?>

Note: Flaterik geeft zelf aan meer een designer te zijn dan een programmeur.

[Verwijderd]

Of met een hele kleine aanpassing:

PHP:

<?php 
$toegestaan = array('home', 'informatie', 'contact'); 
$item = isset($_GET['item']) ? $_GET['item'] : $toegestaan[0]; // Als $_GET['item'] niet is gegeven, naar de eerste pagina in de array gaan 

if (in_array($item, $toegestaan)) { 
    include 'bestanden/'. $item .'.html'; 
} else { 
    echo 'De opgevraagde pagina bestaat niet.'; 
} 
?>

[Voutloos]

Anders gaat men nog 5 posts door nadat er een oplossing gegeven is welke nou niet bepaald rocket science is om aan te passen voor een zinnige default.