[PHP]Onverwachte output.

[Verwijderd]

Hee tweakers,

Na toch al een paar uur verspild te hebben aan dit probleem wil ik graag jullie hulp inroepen. Ik heb het volgende PHP script:

PHP:

<?php

/* Script to retrieve the page content from a text file.*/

$page = $_GET["page"];

function secure_string($string)  { /*Function to check for possible SQL-Injection, and other vulnerabilities. */
$string = htmlentities($string, ENT_QUOTES);
$string = stripslashes($string);

    if(strpos($string,"<") && strpos($string,";") && strpos($string,"@") && strpos($string,"!") && strpos($string,">") && strpos($string,"?") && strpos($string, ".") == false ) {
    return $string;
}
        else {
        /*header('Location: TheDudeIsTryingToHackUs.on.nimp.org'); Send possible attackers to the hyperlink */
        echo $string;
        return "page_not_found"; 
} }

/*Open the file to read content */
$filename = "./content/" . secure_string($page) .".txt";
$handle = fopen($filename , "r");
$content = fread($handle, filesize($filename));

echo $content; 

?>

De functie van het script lijkt me vrij duidelijk, een user gaat naar www.mijnsite.nl/getpage.php?page=about en als alles goed gaat wordt about.txt gelezen en weergegeven op de site.

Ik heb een aantal checks ingebouwd om te zorgen dat users niet ?page=../al_mijn_wachtwoorden.txt intypen om maar een voorbeeld te noemen. De checks gaan goed daarmee bedoel ik dan: htmlentities en stripslashes maar met de laatste beveiligingsmaatregel gaat was mis:

PHP:

    if(strpos($string,"<") && strpos($string,";") && strpos($string,"@") && strpos($string,"!") && strpos($string,">") && strpos($string,"?") && strpos($string, ".") == false ) {
    return $string;
}

De PHP.NET manual voor strpos zegt het volgende:

Returns the position as an integer. If needle is not found, strpos() will return boolean FALSE.

Als het goed is en geen van deze characters zijn aanwezig moet de if statement false returnen. Vul ik echter ?page=about> in dan wordt het else statement ge-execute. Maar het meest rare is: vul ik "?page=about" in, waar dus geen characters in zitten die niet voor mogen komen. Wordt nog steeds het else statement ge-execute.

Kan iemand mij op weg helpen?

[spleethoven]

PHP:

<?php
    if(strpos($string,"<") && strpos($string,";") && strpos($string,"@") && strpos($string,"!") && strpos($string,">") && strpos($string,"?") && strpos($string, ".") == false ) {
    return $string;
}
?>

Op deze manier ga je alleen maar controleren of "strpos($string, ".") == false" false is, de rest moet volgens jouw script true zijn.

je kan simepweg haakjes rond de hele structuur zetten en dan zal het wel werken:

PHP:

<?php
    if((strpos($string,"<") && strpos($string,";") && strpos($string,"@") && strpos($string,"!") && strpos($string,">") && strpos($string,"?") && strpos($string, ".")) == false ) {
    return $string;
}
?>

[Low-Tech]

Afgezien van jou aparte manier van opzetten, is dit jou probleem:
Je vergelijkt nu alleen de laatste met == false.
Tip: vergelijk met === false ivm de eerste positie (0)

Over jou opzet:
Je controleert nu de string i.v.m. mogelijke SQL injecties en vervolgens gebruik je het als bestandsnaam. Deze twee gaan niet samen, je kan beter de string strippen met een preg_replace zodat je alleen maar a-z en A-Z overhoud en dan met is_file controleren of die bestaat. Of je stopt de inhoud werkelijk in een database en je sanitized de input.

[Creepy]

Vergeet niet dat 0 gelijk is aan false. Dus je zult met === moeten vergelijken.

Los daarvan is het niet zo handig om zelf dit soort checks te bouwen e.d. omdat je je parameter page direct voor de filenaam gaat gebruiken. Handiger is het om een lijst bij te houden van bestanden met een naam in een array. Zo hoef je alleen maar te controleren of in je array de naam voorkomt en die page te tonen. Dan zijn alle andere checks ineens niet meer nodig en heb je geen beveiliginslek als je ineens wat vergeten bent.

[Verwijderd]

Wow 3 reacties in minder dan 10 minuten! Super top!

Bedankt voor jullie verheldering spleethoven en Low-Tech het klonk zo logisch toen ik het opschreef maar achteraf is dit best wel dom

Low-Tech jouw suggesties klinken erg goed Ik ga ze dan ook er in proberen te verwerken. @Creepy ik wil graag veel pagina's makkelijk toe kunnen voegen en als ik jouw manier gebruik moet ik voor elke pagina die ik aanmaak een database-entry maken en moeten pagina's die normaliter geen SQL-verbinding nodig hebben toch een SQL-Verbinding maken om de array van paginas te laden wat volgensmij voor een groot performance loss zorgt. Aan de andere kant is wat jij zegt natuurlijk wel waar, tekst laden a.d.v een bestandsnaam is niet zo slim. Maar als ik d.m.v van regex en preg_replace zoals Low-Tech aangaf alleen a-z en A-Z toe laat wat kan er dan gebeuren?

[Verwijderd]

Om het topic toch zo volledig mogelijk te maken. Na wat aanpassingen doorgevoerd te hebben ziet mijn code er als volgt uit:

PHP:

<?php

/* Script to retrieve the page content from a text file.*/

if(isset($_GET["page"]) == 1) {
$page = $_GET["page"]; }
    else {
    $page = "index"; }

function secure_string($string)  { /*Function to check for possible SQL-Injection, and other vulnerabilities. */
$string = htmlentities($string, ENT_QUOTES);
$string = stripslashes($string);

    if(ctype_alnum($string) == 1) 
    return $string;

        else 
        return "page_not_found"; 
 }

/*Open the file to read content */
$filename = "./content/" . secure_string($page) .".txt";

if(is_file($filename) == 1) {
$handle = fopen($filename , "r");
$content = fread($handle, filesize($filename));
echo $content;  }
    else {
    $handle = fopen("./content/page_not_found.txt", "r");
    $content = fread($handle, filesize("./content/page_not_found.txt"));
    echo $content; }

?>

[TJHeuvel]

Als ik een beetje mag nitpicken zou ik de volgende wijziging maken bij je check of de pagina uberhaupt bestaat:

PHP:

<?php

/*Open the file to read content */
$filename = "./content/" . secure_string($page) .".txt";

 if(!is_file($filename))
  $filename = "./content/page_not_found.txt";

$handle = fopen($filename , "r");
$content = fread($handle, filesize($filename));
echo $content;
?>

Je zag dat je in principe 2 maal dezelfde code had staan, dit valt natuurlijk te optimaliseren
Ook zou je eens moeten kijken naar file_get_contents.

[Low-Tech]

PHP:

if(isset($_GET["page"]) == 1) {

Kan je noteren als:

PHP:

if(isset($_GET["page"])) {

De functie isset geeft alleen een true of false terug, zie link.

En vergeet je functie securestring niet aan te passen, je kan kijken naar bijvoorbeeld filter_var. Maar persoonlijk heb ik liever een preg_replace.

[Verwijderd]

Hee, bedankt voor jullie tips. Wat TJheuvel zei is inderdaad erg goed, ik heb nu 2x hetzelfde en dat kan voorkomen worden. Thanks

@Low-Tech ook handige tips, het ziet er een stuk beter uit zonder de == 1), ipv preg_replace gebruik ik ctype_alnum dan hoef ik niet ingewikkeld te doen met regex.

[crisp]

Als je filenaam ueberhaupt alleen maar alfanumerieke karakters mag bevatten dan zijn de htmlentities en stripslashes ook overbodig.

Sowieso is htmlentities geen sanitation functie maar een output encoding functie (en meestal ben je beter en sneller af met htmlspecialchars), en is stripslashes alleen maar nodig als je nog PHP's oude magic-quotes feature gebruikt (die tegenwoordig standaard uit staat nmw).

[IceM]

Ik snap niet zo goed waarom je al die stripslashes etc gebruikt terwijl de bestandsnaam blijkbaar enkel uit alfanumerieke karakters mag bestaan en al het andere ongeldig is. Als ik snel even naar je code kijk voldoet het volgende ook?

PHP:

$page = "index";
if (isset($_GET["page"]) && !empty($_GET["page"])) {
    $page = $_GET["page"];
}

$fileName = "./content/" . $page . ".txt";

if (ctype_alnum($page) && is_file($fileName)) {
    $fileName = "./content/page_not_found.txt";
}

echo  file_get_contents($fileName);

Edit: zoals crisp zei dus ...

[ Voor 14% gewijzigd door IceM op 30-07-2011 11:27 ]

[YopY]

Verwijderd schreef op woensdag 27 juli 2011 @ 17:02:
[...] wat volgensmij voor een groot performance loss zorgt.

Heb je dat nagemeten? Hoe weet je dat? $prematureOptimization === Math.sqrt(EVIL); enzo .

Als het veilig moet en toch makkelijk te onderhouden is het ongeveer 372,234388 maal eenvoudiger om een CMS of Wordpress of wat maar van toepassing is te installeren en op te zetten.

En het kan nog korterder.

PHP:

//niks

Zet gewoon een paar .html bestanden in je webroot. Die kun je dan met een programmaatje genaamd een 'webserver' zo ophalen door een speciale URL in te vullen in je webbrowser - zo'n URL ziet er dan ongeveer zo uit:

http://www.example.com/mijn_pagina.html

En je kunt ook nog subpagina's en een hele structuur opzetten:

http://www.example.com/categorie/dingetjes/pietje/puk.html

Wat een amazing discovery! En zoiets als http://example.com/../ding.html zal ook niet werken. SQL injection? Niet mogelijk.

* YopY steekt natuurlijk den draak aan, verveelt zich, etc.

[ Voor 3% gewijzigd door YopY op 30-07-2011 20:56 ]

[Verwijderd]

Ik doe het gewoon zo:

PHP:

<?php
                    if(!empty($_GET['cat']) && !empty($_GET['page'])){
                        $path = '/admin/' . basename($_GET['cat']) . '/' . basename($_GET['page'], 'php') . '.php';
                        if(file_exists($_SERVER{'DOCUMENT_ROOT'} . $path)){
                            include $_SERVER{'DOCUMENT_ROOT'} . $path;
                        }
                    }
?>

Met basename() zorg ik ervoor dat er enkel bestanden uit een bepaalde directory kunnen worden geraadpleegd. In dit voorbeeld worden enkel php bestanden geopend (ik gebruik ze met een include). Ik hoop dat dit is wat je zoekt ...

offtopic:
Sorry YopY, maar ik zie niet echt in wat jouw reactie bijdraagt ... Ben ik niet echt van jou gewoon. Jammer!

[Verwijderd]

Verwijderd schreef op zondag 31 juli 2011 @ 23:26:
Met basename() zorg ik ervoor dat er enkel bestanden uit een bepaalde directory kunnen worden geraadpleegd.

Ik vind dat een beetje fout. Als iemand een URL probeert te vervalsen stuur je hem door naar een pagina die er nog enigszins op lijkt. Dat hoort niet. Een URL verwijst naar iets unieks, en als er meerdere dingen naar datzelfde verwijzen is er iets mis. Mensen die URL's proberen te forgen moeten een 404 voorgeschoteld krijgen, of een 403 als je de potentiele aanvaller wil waarschuwen.