Groot probleem ontdekt

Overzicht Reageren

Sponsored by: Vacatures door Monsterboard

Mario Onbekend

Mario Onbekend

06/10/2010 01:10:19
Quote Anchor link
Hallo,

Ik was zojuist wat aan het uitproberen, en toen kwam ik erachter dat er een probleem in mijn ledenscript zit.
Als iemand zich wil registreren, en hij voert daarbij de gegevens (gebruikersnaam en wachtwoord) in van een lid dat al bestaat, dan wordt hij ingelogd (als het lid dat hij invoert).
Ik heb gecontroleerd of dit niet aan de sessie lag, maar dit is niet het geval. Ik heb namelijk ter controle meerdere leden geregistreerd, en bij het inschrijven met dezelfde gegevens werden al deze leden ingelogd.

Kunnen jullie mij helpen? Ik weet niet waar het probleem zit.

index.php
* Registreren is ?pagina=inschrijven en inloggen is ?pagina=aanmelden
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
<?php

include("config.php");

session_start();

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    if (!$_POST['aanmelden']) {
        $gebruikersnaam = $_POST['gebruikersnaam'];
        $gebruikersnaam = stripslashes($gebruikersnaam);
        $gebruikersnaam = mysql_real_escape_string($gebruikersnaam);
        $wachtwoord = $_POST['wachtwoord'];
        $wachtwoord = stripslashes($wachtwoord);
        $wachtwoord = mysql_real_escape_string($wachtwoord);
        $veiligwachtwoord = md5($wachtwoord);
        $veiligwachtwoord = md5($veiligwachtwoord);
        $veiligwachtwoord = sha1($veiligwachtwoord);
        $query = "SELECT * FROM gebruikers WHERE gebruikersnaam='".$gebruikersnaam."' AND wachtwoord='".$veiligwachtwoord."' AND actief='1'";
        $query2 = mysql_query($query);
        $query3 = mysql_num_rows($query2);
        if ($query3 == 1) {
            session_register("gebruikersnaam");
            session_register("veiligwachtwoord");
            header("Location: welkom.php");
        }
    }

    if (!$_POST['inschrijven']) {
        $gebruikersnaam = $_POST['gebruikersnaam'];
        $wachtwoord = $_POST['wachtwoord'];
        $wachtwoord2 = $_POST['wachtwoord2'];
        $emailadres = $_POST['emailadres'];
        $voornaam = $_POST['voornaam'];
        $achternaam = "";
        $rang = 1;
        $saldo = 10;
        $actief = 1;
        if (!empty($voornaam) and !empty($gebruikersnaam) and !empty($wachtwoord) and !empty($wachtwoord2) and $wachtwoord2 == $wachtwoord) {
            $query = "SELECT * FROM gebruikers WHERE gebruikersnaam='".$gebruikersnaam."'";
            $query2 = mysql_query($query) or die ("We kunnen geen verbinding maken.");
            $query3 = mysql_num_rows($query2);
            if ($query3 == 0) {
                ob_start();
                    header("Location: http://www..eu/id/index.php?pagina=inschrijven&id=ingeschreven");
                ob_flush();
                $veiligwachtwoord = md5($wachtwoord);
                $veiligwachtwoord = md5($veiligwachtwoord);
                $veiligwachtwoord = sha1($veiligwachtwoord);
                $inschrijven = "INSERT INTO gebruikers (gebruikersnaam, wachtwoord, emailadres, voornaam, rang, saldo, actief) VALUES ('".$gebruikersnaam."', '".$veiligwachtwoord."', '".$emailadres."', '".$voornaam."', '".$rang."', '".$saldo."', '".$actief."')";
                mail("".$emailadres."", "Welkom bij ", "Hallo ".$voornaam.",\n\nWelkom bij ! Uw gegevens zijn:\n\nGebruikersnaam: ".$gebruikersnaam."\nWachtwoord: ".$wachtwoord."\n\nVeel plezier!\n\n\n\n\nwww..eu", "From: ");
                mysql_query($inschrijven) or die ("We kunnen geen verbinding maken.");
            }
else {
                ob_start();
                    header("Location: http://www..eu//index.php?pagina=inschrijven&id=gebruikersnaam");
                ob_flush();
            }
        }
    }
}


?>


config.php
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
<?php

$dbhost
= "localhost";
$dbgebruikersnaam = "";
$dbwachtwoord = "";

$verbinden = mysql_connect($dbhost, $dbgebruikersnaam, $dbwachtwoord) or die ("<b>Code D001:</b><br />We kunnen geen verbinding maken.");

$db = "";

mysql_select_db($db) or die ("<b>Code D002:</b><br />We kunnen geen verbinding maken.");

// Functies

function isnum($value) {
    if (!is_array($value)) {
        return (preg_match("/^[0-9]+$/", $value));
    }
else {
        return false;
    }
}

function
redirect($location, $script = false) {
    if (!$script) {
        header("Location: ".str_replace("&amp;", "&", $location));
        exit;
    }
else {
        echo "<script type='text/javascript'>document.location.href='".str_replace("&amp;", "&", $location)."'</script>";
        exit;
    }
}


?>
Gewijzigd op 13/07/2011 14:05:45 door Bas Kreleger
 
PHP hulp

PHP hulp

19/09/2019 09:38:43
 
- SanThe -

- SanThe -

06/10/2010 01:45:51
Quote Anchor link
Het zal wel aan de header() op regel 43 liggen denk ik. Verder is je script lek => sql-injection.
 
Mario Onbekend

Mario Onbekend

06/10/2010 02:05:35
Quote Anchor link
Hoe kan het aan de header liggen dan? Het enige wat deze header doet is de pagina doorsturen naar ?pagina=inschrijven&id=ingeschreven, waarop de $_GET['ingeschreven'] een mededeling weergeeft dat je bent geregistreerd.
En hoe kan ik ervoor zorgen dat het script goed beveiligd is?
 
Noppes Homeland

Noppes Homeland

06/10/2010 06:26:02
Quote Anchor link
Er is wel meer mis dan alleen SQL injectie:

Geen correcte controles en validaties op de $_POST
Het maken van overbodige variabelen
Gebruik van de functie empty
Het bufferen van data als het om headers gaat
session_register is een prehistorisch gebruik $_SESSION
waarom een functie isnum als je ctype functies hebt, welke vaak ook nog eens sneller zijn dan een preg.
totaal geen foutafhandeling op afgevuurde queries
dit soort zaken: "".$emailadres."" is de grootstmogelijk onzin er zijn nu 3 zaken die door de php-parser geevalueerd moeten worden.
het te pas en te onpas gebruiken van exit, duid er vaak op dat het ontbreekt aan script logica
 
Robert Deiman

Robert Deiman

06/10/2010 07:07:56
Quote Anchor link
@Noppes
Probeer ook eens aan te geven hoe het wel moet volgens jou, een script afschieten en fouten aanwijzen is vaak niet zo lastig. Het is voor iedereen prettiger als je ze probeert te helpen, in plaats van bijna bang te maken en ze probeert van PHP af te krijgen. (Zo komen je reacties nog al eens over)
En ik zie weer de functie empty, die vind jij niet goed, maar waarom weet niemand. En toch wil je ook niet aangeven waarom. (zie topic: short if)

Als iemand andere functies gebruikt die eenzelfde controle doen, prima toch, hij is op zoek naar wat er fout gaat. Jij geeft eigenlijk alleen maar aan:
- Je doet alles fout

Probeer feedback te geven ipv commentaar, dat hout het hier wel zo gezellig..


Zo nu verder met de eigenlijke reden voor posten:
Wat ik vermoed is dat je controle op deze 2 variabelen niet goed gaat:
$_POST['aanmelden']
$_POST['inschrijven']

Als je wordt ingelogd, betekend dit eigenlijk dat het script al in de eerste IF (binnen de controle of er überhaupt wat is gepost) komt, waar de user wordt ingelogd.
Als het goed is wordt dus de nieuwe user ook niet aangemaakt.

Als een nieuwe gebruiker wordt aangemaakt wil je niet dat hij/ zij ingelogd wordt. Dat is 1 fout, want nu gebeurt dat wel. Je kan bij posten eens met bijvoorbeeld print_r (of var_dump, it's your call) en dan een exit (dus binnen if($_SERVER['REQUEST_METHOD'] == 'POST) ) eens kijken wat er in de gehele geposte array staat. Het gaat dan met name om de eerder door mij genoemde variabelen.

Daarnaast gaat het fout doordat je 2 maal alleen een if gebruikt voor deze genoemde variabelen, maar de logica zou moeten zijn:
- Als iemand zich aanmeld (bij jou aanmelden == inloggen, is wel verwarrend) moet alleen ingelogd worden.
- Als iemand zich inschrijft moet die niet ingelogd worden.

Codetechnisch (zonder de headers mee te rekenen, dat kan ook fout gaan) moet je het dan al zo opbouwen dat als de 1e actie waar is, dat de volgende actie niet waar is/ niet gecontroleerd wordt. if/ else if, zijn hiervoor een betere constructie.
Daarnaast is het aan te raden om een exit; te plaatsen na een header redirect. Als de header fout gaat, wil je niet dat dan alsnog de rest van het script wordt uitgevoerd.
 
Obelix Idefix

Obelix Idefix

06/10/2010 09:20:51
Quote Anchor link
Mario Braam op 06/10/2010 01:10:19:

Als iemand zich wil registreren, en hij voert daarbij de gegevens (gebruikersnaam en wachtwoord) in van een lid dat al bestaat, dan wordt hij ingelogd (als het lid dat hij invoert).
Ik heb namelijk ter controle meerdere leden geregistreerd, en bij het inschrijven met dezelfde gegevens werden al deze leden ingelogd.


Kan het zijn dat je niet controleert of gebruikersnaam al bestaat?
De kans dat iemand dezelfde gebruikersnaam _en_ wachtwoord kiest lijkt me overigens niet zo groot.
 
Karl Karl

Karl Karl

06/10/2010 09:23:59
Quote Anchor link
Kan je geen betere topic titel verzinnen?
Dit is niks zeggend.
 
Robert Deiman

Robert Deiman

06/10/2010 09:38:42
Quote Anchor link
Obelix en Idefix op 06/10/2010 09:20:51:
Mario Braam op 06/10/2010 01:10:19:

Als iemand zich wil registreren, en hij voert daarbij de gegevens (gebruikersnaam en wachtwoord) in van een lid dat al bestaat, dan wordt hij ingelogd (als het lid dat hij invoert).
Ik heb namelijk ter controle meerdere leden geregistreerd, en bij het inschrijven met dezelfde gegevens werden al deze leden ingelogd.


Kan het zijn dat je niet controleert of gebruikersnaam al bestaat?
De kans dat iemand dezelfde gebruikersnaam _en_ wachtwoord kiest lijkt me overigens niet zo groot.


Het grappige in zijn code is, dat de controle voor inloggen al voor het registreren staat. Als er ingelogd wordt tijdens het registreren, betekend het dat de controle niet goed is en dat de ingevoerde gebruikersnaam/ wachtwoord combinatie wordt meegegeven aan de inlogfunctie, in plaats van het registreren van de gebruiker.
Gezien het feit dat het systeem inlogt, worden de gebruikers helemaal niet aangemaakt.
 
Elio vp

Elio vp

06/10/2010 11:38:30
Quote Anchor link
Ik zou persoonlijk beginnen om register & login op te splitsen in 2 bestanden..

Dan gaat het misschien voor jezelf duidelijker zijn :-)

Je kan ze altijd includen he..

en dan heb je eveneens ook het probleem niet dat je eerst gaat controleren op login voor iemand registreert..


Succes!
 
Mario Onbekend

Mario Onbekend

06/10/2010 12:10:46
Quote Anchor link
@Noppes:
Bedankt voor je commentaar, alleen ik ben het ook eens met Robert. Je geeft commentaar, maar geen mogelijke oplossing. En dat is voor een beginner (want dat ben ik) nog best lastig. Mijn vraag staat niet voor niks in het forum.

@Robert:
Misschien is het een idee om 2 bestanden aan te maken (aanmelden.php en inschrijven.php), en dan bij de form action deze bestanden op te geven. Dan blijven meteen beide acties gescheiden denk ik.
Je geeft aan dat ik in de post moet kijken wat er in de array staat. Maar hoe moet ik dit dan doen? Ik weet namelijk niet hoe print_r, var_dump of exit werken...
En hoef ik achter de header dan alleen exit(); te plaatsen?

@Obelix en Idefix:
De kans is inderdaad heel klein. Maar de kans moet er gewoon niet zijn. Ik kan niet voorspellen wat voor wachtwoorden mijn leden kiezen, maar niet iedereen gebruikt zo'n veilig wachtwoord als ik doe.

@Karl Karl:
Geen commentaar. Reageer dan niet.

@Robert:
Dat klopt, het lid wordt dan niet aangemaakt. Eigenlijk functioneert het registratiescript een beetje als loginscript, haha. Wel handig, maar niet mijn bedoeling!

@Elio:
Dankjewel, dat ga ik doen.
 
Karl Karl

Karl Karl

06/10/2010 12:16:29
Quote Anchor link
Mario Braam op 06/10/2010 12:10:46:
(...)
@Karl Karl:
Geen commentaar. Reageer dan niet.
(...)


De topic titel hoort de samenvatting te zijn van het topic in één regel.
Dat is mijn commentaar. En daarom reageer ik ook.
 
Mario Onbekend

Mario Onbekend

06/10/2010 12:19:16
Quote Anchor link
Ga liever inhoudelijk in op mijn vraag zonder je te ergeren aan mijn topic titel. Dit ledenscript heeft voor mij veel waarde aangezien ik het dit keer helemaal zelf heb gemaakt! En dan is het jammer als je zulke problemen tegen komt.
 
Nicoow Unknown

Nicoow Unknown

06/10/2010 12:24:20
Quote Anchor link
Als jij word aangehouden door oom agent, dan zeg je ook, ga liever inhoudelijk in op mijn auto, in plaats van te zeuren over die 60km te hard, deze auto betekent echt veel voor me, ik heb hem helemaal zelf gekocht.

Je overtreed gewoon een regel door het gebruik van een slechte topic titel, dus daar zal je een oplossing voor moeten vinden.
En nee, vluchten is geen optie.
 
John D

John D

06/10/2010 12:29:24
Quote Anchor link
@Mario: Het is gebruikelijk dat je Topic Titel enigszins iets weergeeft dat betrekking heeft op de functionaliteit van je script en dan met name datgene dat niet werkt. Gewoon even aanpassen en niet zo arrogant over doen.
 
Karl Karl

Karl Karl

06/10/2010 12:57:09
Quote Anchor link
Mario Braam op 06/10/2010 12:19:16:
Ga liever inhoudelijk in op mijn vraag zonder je te ergeren aan mijn topic titel. Dit ledenscript heeft voor mij veel waarde aangezien ik het dit keer helemaal zelf heb gemaakt! En dan is het jammer als je zulke problemen tegen komt.


Oh, het is een ledenscript. Dat had ik nog niet gezien...
Zag wel dat er wat sql rommel in zat. Maar dat het ledenscript was.
Dus misschien had je als titel iets kunnen doen als 'SQL problemen ledenscript'.
Of 'Ledenscript dubbele leden'.
 
Niek s

niek s

06/10/2010 13:11:19
Quote Anchor link
if (!$_POST['aanmelden']) {

Geeft bijna altijd true. Besef je dat?
 
Robert Deiman

Robert Deiman

06/10/2010 14:09:09
Quote Anchor link
Mario Braam op 06/10/2010 12:10:46:
@Robert:
Misschien is het een idee om 2 bestanden aan te maken (aanmelden.php en inschrijven.php), en dan bij de form action deze bestanden op te geven. Dan blijven meteen beide acties gescheiden denk ik.
Je geeft aan dat ik in de post moet kijken wat er in de array staat. Maar hoe moet ik dit dan doen? Ik weet namelijk niet hoe print_r, var_dump of exit werken...
En hoef ik achter de header dan alleen exit(); te plaatsen?
Quote:
Wellicht dat http://nl.php.net/print_r of http://nl.php.net/var_dump je helpen. Maar het enige wat je hoeft te doen is dit toevoegen op de genoemde plek:
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
<?php
//de echo en <pre> zijn voor een leesbare weergave
echo '<pre>' . print_r($_POST, true) . '</pre>';
exit;
?>


Het maken van 2 bestanden is een optie, maar geen moeten. Het kan prima, maar dan moet je goed weten (nagaan) wat er gebeurt, en waar je op controleert. De print_r kan je daarbij helpen.

Mario Braam op 06/10/2010 12:10:46:
@Obelix en Idefix:
De kans is inderdaad heel klein. Maar de kans moet er gewoon niet zijn. Ik kan niet voorspellen wat voor wachtwoorden mijn leden kiezen, maar niet iedereen gebruikt zo'n veilig wachtwoord als ik doe.
Quote:
In principe moet je afdwingen (unique kolom in MySQL) dat een gebruikersnaam maar 1 maal kan voorkomen. Dat het wachtwoord bij sommige mensen overeenkomt, is dan weer geen probleem.

Mario Braam op 06/10/2010 12:10:46:
@Robert:
Dat klopt, het lid wordt dan niet aangemaakt. Eigenlijk functioneert het registratiescript een beetje als loginscript, haha. Wel handig, maar niet mijn bedoeling!

Nee, dat klopt. Daarom moet je ook goed weten wat je controleert. 2 bestanden is leuk, maar niet meteen nodig. De controle kan prima in 1 bestand, als je maar weet wat je doet.
 



Overzicht Reageren

 
 

Om de gebruiksvriendelijkheid van onze website en diensten te optimaliseren maken wij gebruik van cookies. Deze cookies gebruiken wij voor functionaliteiten, analytische gegevens en marketing doeleinden. U vindt meer informatie in onze privacy statement.