Sikkerheds check

Væsenligt nemmere at vudere det når man ser source-code til det, da man ellers bare skyder i blinde...

Security through obscurity er aldrig en smart måde at sikre sig på...

Morten

PS, meget fedt billede med skærmen / hovede - har du selv lavet det?

PPS, det ligner du mangler at style lidt tekst her: http://www.tackleprices.com/merchant/add.php

Det ville ihvertfald ikke skade, med lidt kryptering. Jeg sniffede data, imens jeg indtastede brugernavn: test og password: hest.
Følgende sniffede jeg: username=test&password=hest&submit=Login

Overvej at bruge noget https

"PS, meget fedt billede med skærmen / hovede - har du selv lavet det?"
- Nej, det er fra phpAdsNew (fik at vide det var okay at bruge, når det er open source)

"PPS, det ligner du mangler at style lidt tekst her: http://www.tackleprices.com/merchant/add.php"
- Den bliver lige rettet :P


Login_check.php :
-------------------------------------------------------------------------------------
<?
/* Check User Script */
session_start();  // Start Session
include("/home/tacklepr/public_html/db.php");

// Convert to simple variables
$username = $_POST['username'];
$password = $_POST['password'];

//Adding slashes to the username and password variables
function my_addslashes($streng){
  return get_magic_quotes_gpc()?$streng:addslashes($streng);
}

if((!$username) || (!$password)){
    //If one or both variables are empty, then the user get a error
    include 'login_failure.htm';
    exit();
}

// Convert password to md5 hash
$password = md5($password);

// check if the user info validates the db
$sql = mysql_query("SELECT * FROM merchants WHERE login_name='$username' AND password='$password' AND status_admin='1'");
$login_check = mysql_num_rows($sql);

if($login_check > 0){
    while($row = mysql_fetch_array($sql)){
    foreach( $row AS $key => $val ){
        $$key = stripslashes( $val );
    }
        // Register some session variables!
        session_register('id');
        $_SESSION['id'] = $id;
        session_register('login_name');
        $_SESSION['login_name'] = $login_name;
        session_register('merchant_name');
        $_SESSION['merchant_name'] = $merchant_name;
        session_register('password');
        $_SESSION['password'] = $password;
       
        mysql_query("UPDATE merchants SET last_login=now() WHERE id='$id'");
        header("Location: index.php");
    }
} else {
    include 'login_failure.htm';
}
?>


index.php :
-------------------------------------------------------------------------------------
<?
include "/home/tacklepr/public_html/merchant/include/header.php";
?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<title>Tackleprices.com | Merchant Area</title>
<link href="/merchant/styles/default.css" rel="stylesheet" type="text/css">
<?
include "/home/tacklepr/public_html/merchant/include/head.php";
?>
</head>
<body>
<?
include("/home/tacklepr/public_html/db.php");
include "/home/tacklepr/public_html/merchant/menu/menu.htm";
?>
<div id="container">
  <?
include "/home/tacklepr/public_html/merchant/include/menues.php";
?>
</div>
</body>
</html>


header.php (bliver inkluderet på alle sider, for at check om brugeren er logged ind) :
-------------------------------------------------------------------------------------
<?
session_start();

// check if the user info validates the db
include("/home/tacklepr/public_html/db.php");

$sql_doublecheck = mysql_query("SELECT * FROM merchants WHERE login_name='$login_name' AND password='$password' AND status_admin='1'");
$doublecheck = mysql_num_rows($sql_doublecheck);

if($doublecheck != 1) {
    include '/home/tacklepr/public_html/merchant/login.htm';
    exit();
}
?>

x-masman: Hvordan smider jeg https på mine sider? Er det noget der kan aktiveres automatisk, eller skal det integreres på alle mine sider? Har altid opfattet https som noget heavy...et-eller-andet :P

HTTPS er en server-ekstention - og som man normalt skal betale lidt ektra for hos webhoteller...

Du kan nok omgå det ved at når man trykker [Login], så md5-krypteres password med JS og sendes krypteret - så skal du bare lade være med at kryptere det med PHP da det jo allerede er krypteret...

---

Jeg ville lave
$username = $_POST['username'];
$password = $_POST['password'];

om til
$username = (isset($_POST['username'])) ? $_POST['username'] : '';
$password = (isset($_POST['password'])) ? $_POST['password'] : '';

Bare så den ikke laver notice's hvis der ikke er noget password...

ligeledes ville jeg bruge

empty() istedet for at tjekke om de er sande/falske...

Morten

Det er ikke mig der er php-programmør. :o)

fangel: Jeg arbejder på det JS md5 kryptering, god ide :)

Jeg ikke lige hvad du mener med resten

https er noget du sætter din webserver op til. Du skal bruge et modul der hedder mod_ssl så vidt jeg husker - så det er ikke rigtig noget med php at gøre :)

Der findes garanteret en tutorial på nettet et sted.

dennismp: det vil jeg kigge på, når resten er på plads :)

dette:

---
Jeg ville lave
$username = $_POST['username'];
$password = $_POST['password'];

om til
$username = (isset($_POST['username'])) ? $_POST['username'] : '';
$password = (isset($_POST['password'])) ? $_POST['password'] : '';

Bare så den ikke laver notice's hvis der ikke er noget password...
---

er ret forståeligt... hvis serveren står til error_reporting = E_ALL, så vil det give en notice: undefined variable, hvis variablen ikke findes... dette stykke kode tjekker om de findes, hvis de ikke gør det sættes de til '' (tom streng) istedet...

det andet var ang. dette:

if((!$username) || (!$password)){
    //If one or both variables are empty, then the user get a error
    include 'login_failure.htm';
    exit();
}

som jeg ville lave til

if(empty($username) || empty($password)){
    //If one or both variables are empty, then the user get a error
    include 'login_failure.htm';
    exit();
}

og se om de er tomme istedet for at tjekke om de er false... endnu bedre ville være

if(empty(trim($username)) || empty(trim($password))){
    //If one or both variables are empty, then the user get a error
    include 'login_failure.htm';
    exit();
}

der også tjekke om de kun indeholder mellemrum/tab/newline/whatever blanke tegn

Morten

Når du bruger $_SESSION['blah'] = 1; behøver du ikke kalde session_register('blah');

coderdk; Du henviser til denne part (ik?):

    }
        // Register some session variables!
        session_register('id');
        $_SESSION['id'] = $id;
        session_register('login_name');
        $_SESSION['login_name'] = $login_name;
        session_register('merchant_name');
        $_SESSION['merchant_name'] = $merchant_name;
        session_register('password');
        $_SESSION['password'] = $password;
       
        mysql_query("UPDATE merchants SET last_login=now() WHERE id='$id'");
        header("Location: index.php");
    }


- Skal jeg så bare slette:
        session_register('id');
        session_register('login_name');
        session_register('merchant_name');
        session_register('password');

x-masman : Så skulle det ikke være muligt at sniffe data mere :)

fangel: Rigtig gode forslag, de er hermed rettet (Forstod kun den sidste, den første er for kompliceret til mine evner :P)

Jeg får dog en fejl i linie 15:
if(empty(trim($username)) || empty(trim($password))){

Fejlen:
Parse error: parse error, expecting `T_VARIABLE' or `'$'' in /home/tacklepr/public_html/merchant/login_check.php on line 15

session_register må ikke bruges, hvis man bruger $_SESSION - men det har du fixet.

Prøv:
$username = trim($username);
$password = trim($password);

if( empty($username) || empty($password) )

.. den fejl du får, er een af PHPs særheder. Har aldrig helt forstået hvorfor.

Jeg kan stadig sniffe data, men bare ikke se hvad der står. ;)

dennismp: "session_register må ikke bruges, hvis man bruger $_SESSION - men det har du fixet."
- nej, ikke endnu. Skal jeg bare fjerne session_register de steder kan er anvendt?


parse error fejlen er væk, så det virkede :)

x-masman: Du kan kun se brugernavn og det krypterede password, ik?

btw.. der er noget med at hvis man kender et brugernavn, så kan passwordet omgåes med noget /'.. OR = /'.... et eller andet. Er der en der vil teste med brugernavnet "wide"?

Ja, session_register skal væk.

Fra manualen:
"If you are using $_SESSION (or $HTTP_SESSION_VARS), do not use session_register(), session_is_registered(), and session_unregister()."

Jeg forsøgte det med OR men kunne ikke komme ind - så enten er jeg er for dårlig eller også er der taget hånd om det :)

okay :)

Så ser det sådan ud

}
        // Register some session variables!
        $_SESSION['id'] = $id;
        $_SESSION['login_name'] = $login_name;
        $_SESSION['merchant_name'] = $merchant_name;
        $_SESSION['password'] = $password;
       
        mysql_query("UPDATE merchants SET last_login=now() WHERE id='$id'");
        header("Location: index.php");
    }

dennismp: Jeg har prøvet at løse det problem (fik hjælp til det problem tidligere), men er aldrig blevet testet i praktis.

function my_addslashes($streng){
  return get_magic_quotes_gpc()?$streng:addslashes($streng);
}

denne funktion kan hindre SQL-Injection Expliots - men du får aldrig brugt den... ;)

kør lige en

$username = my_addslashes($username);
$password = my_ad...

de-facto standarden er dog at `get_magic_quotes_gpc` er sat tuk true, så serveren plejer at hindre dette...

Morten

fangel: ved ikke lige hvad du mener

fangel: jeg tror han bruger magic_quotes_gpc så han behøver ikke. Jeg har forsøgt at provokere en fejl men det lykkedes ikke :-)

Du har set dennismp (jeg bruger magic_quotes_gpc):

function my_addslashes($streng){
  return get_magic_quotes_gpc()?$streng:addslashes($streng);
}

$username = trim($username);
$password = trim($password);

du har funktionen i koden, men du bruger aldrig funktionen... ;) dvs du skal kører tekststrengene igennem funktionen for at sikre sig at de er ordenligt escape'et...

har også forsøgt at provokere en fejl - heller ikke lykkedes her...

Morten

Fangel: Er det ikke rigtigt nu

//Adding slashes to the username and password variables
function my_addslashes($streng){
  return get_magic_quotes_gpc()?$streng:addslashes($streng);
}

$username = my_addslashes($username);
$password = my_addslashes($password);

det er bedre ;)

fino :)

Er der andre ting jeg kan gøre for at forbedre sikkerheden?

et lille input: en ny artikel omkring sikkerhed kan findes her: http://www.devshed.com/c/a/Security/Lock-Down-Your-Website/

God artikel avminarm.

Har ingen begrænsning på min password field, så jeg må nok hellere kigge på den del :)

Du skal lige vide, at det at køre md5 på passwordet i JS egentlig ikke giver nogen ekstra sikkkerhed. Det er korrekt, at en sniffer så ikke vil kunne se koden, men han nu er det jo bare en md5-kode, der sendes i stedet, og den er også ens hver gang (indtil der bliver skiftet password). Så snifferen kan stadig se, det han har brug for.

Et fix kunne være at lade javascriptet tage en md5 af en streng med et tidsstempel sat sammen med passwordet og sende brugernavn, tidsstempel og md5-kode. Så er det nemlig en ny md5-kode, der sendes hver gang, og en sniffer skal kende koden for at kunne lave en gyldig md5-kode.

Så skal du bare i dit php gemme tidsstemplet og kontrollere, at det er nyere end sidste gang, der blev logget ind, så en sniffer ikke bare kan genbruge en gammel kode.

Ang. md5 i javascript. Er det ikke lidt overflødigt?

Ja, folk kan ikke se ens "ægte" password (det de taster ind), men det er jo også ligemeget, for imellem browseren og serveren der bruges md5-hashen af passwordet som det egentlige password. Om du bruger det ægte password eller en md5-hash er jo i sidste ende det samme.

Hvis jeg bare får fat i det, så kan jeg logge ind. Og da du bruger javascript så skal du have det slået til for at kunne logge ind - ellers har du lukket de folk ude. Og det giver ikke rigtig nogen fordel. Hvis man er bange for at nogle opsnapper passwordet i transit, så bruge https istedet.

Ideen med at bruge md5 password er bedre at gøre serverside. Således at du modtager brugerens password, laver en md5-hash og så sammenligner med den md5-hash man har i databasen. Fordelen er her, at selvom nogen får adgang til databasen, så kan de ikke få fat i brugerens password. De kan ændre det ja, men så har du mulighed for at se at noget er galt. Hvis man kan aflæse passwordet i klar tekst - så kan man bare misbruge samtlige konti - og det ville være sværere at opfange.

Hvis I kan huske den sag med telia et par år tilbage, hvor en masse brugers password blev lagt på nettet. Dette kunne de havde undgået med md5-hashes. men md5-hashes giver ikke mere sikkerhed end at det skjuler folks ægte password. Det hindre jo ikke nogen i at erstatte en md5-hash med et andet. Jeg bruger altid md5-hash på denne måde. Jeg ser slet ikke nogen grund til at jeg skal kunne se folks password.

dennismp: Det, du skriver i dine tre første afsnit, er vel det samme, som jeg også skrev i første afsnit? Derudover er jeg også enig i dine betragtninger.

Mit forslag var ment som et fiks til at gøre JS-metoden mere sikker, hvis man ikke har mulighed for at bruge https. Her er lidt pseudo-kode, til at illustrere:

Klient:
kode_til_server=md5( tid . md5(password));

Server:
if( md5($POST[tid'].$pw_fra_db) == $_POST['password'] && $POST[tid']>$tid_fra_db){
  //login OK
}

Jeg har lige læst dit igen. Jo, vi er vist enige - skip de afsnit.

Jeg syntes bare det er spild af arbejde med javascript. Brug https :). At bruge timestamp løser heller ikke problemet - udover at man så skal have klient til at have indstillet sit ur så den matcher serveren.. Og selvom det virker, så giver det ikke mere sikkerhed. MrBadGuy kan jo bare gå ind på loginside og se hvordan javascriptet gør.. Så det gør det allerhøst en smule mere besværligt.

Jeg gentager vist bare migselv og andre nu :-)

Det hjælper ikke MrBadGuy at se, hvordan scriptet virker, hvis han ikke har kodeordet. De eneste værdier, formularen skal sende, er:

bruger="hugo";
tid="20040611235455";
kode=md5("20040611235455".md5($kodeord));

På serveren kontrolleres så, at den sendte tid er større end den sidst brugte tid, så det stiller ikke store krav til hvordan brugerens ur er indstillet. Ja, det vil være et problem, hvis brugeren en gang logger ind fra et sted med et ur langt inde i fremtiden, for så vil han ikke kunne logge ind igen, før hans normale computer indhenter tidsforskellen, men hvis bare uret er nogenlunde rigtigt, er der ikke et problem.

I stedet for et tidsstempel kunne man bruge en unik kode, genereret af serveren. Så gemmer man den i $_SESSION samtidig med at man laver login-formularen. Denne unikke kode kunne sagtens også bare være et tidsstempel.

Det, der er vigtigt, er, at der benyttes en eller anden stump tekst, der ikke kan genbruges, og at den bliver md5-kodet sammen med den egentlige kode. Dermed kan MrBadGuy ikke sniffe sig frem til noget, han kan bruge til selv at logge ind senere.

Men i modsætning til https, så giver denne metode ikke nogensomhelst sikkerhed mod at MrBadGuy kan læse hele indholdet af de passwordbeskyttede sider. Det eneste metoden sikrer, er at man kun kan logge ind ved brug af koden, og at koden ikke bliver opsnappet undervejs. Derfor skal dette kun ses som en nødløsning, hvis https ikke er en mulighed.

Ah, nu tror jeg at jeg begynder at forstå ideen. Det er jo egentlig meget snedigt, hvis man er sikker på at ens brugergruppe har javascript slået til.

"Ja, det vil være et problem, hvis brugeren en gang logger ind fra et sted med et ur langt inde i fremtiden"

Det er jo en antagelse man kan leve med. Men kræver vel så at "sidst brugte tid" kun bliver justeret når brugeren er godkendt (ellers kan det vel misbruges af den famøse MrBadGuy - hvis han forsøger sig med tider i fremtiden, så skal det jo ikke gå ud over brugeren).

Men det er da interessant - selvom det er en nødløsning.  Er det noget du selv har fundet på, eller læst dig dig til (i så fald, hvor) ? Jeg må indrømme at jeg har vist været ret hurtig til at bedømme javascript løsninger som ubrugelige, men så lærte jeg lidt idag :)

Det er min egen ide - muligvis inspireret af lidt summen i baghovedet efter at have læst om den metode Jyske Bank bruger til deres netbank. Deres er dog udvidet en tand mere, da der også benyttes et unikt password, som findes på en lap papir med en hel masse koder.

Det vil sige, at de sender en kode, som kunden bruger til at finde en anden kode, og den sidste kode md5es så sammen med passwordet. Så kræver det, at man både kender password og har en gyldig lap papir for at komme ind.

(Og ja, serveren skal naturligvis ignorere tidsstemplet fra ugyldige login-forsøg.)

sandbox ->

hvis disse info bliver godtaget:
bruger="hugo";
tid="20040611235455";
kode=md5("20040611235455".md5($kodeord));

en gang, bliver de også godtaget næste gang... det giver bare brugeren en ting mere og sniffe, nemli 'tid'-variablen...

Man kunne kombinere det med en PGP-key - men ved ikke hvordan det implementeres i JS.. (så bruger oplyser public-PGP-key ved oprettelse, og så sender en md5 af PGP-keyword og password, således at der sendes en, for brugere, individuel tekst hashet sammen med passwordet...

Morten

Så vidt jeg forstod, så skal det "næste" timestamp være højere end det forrige for at undgå at man kan "genbruge" den hash-værdi man får.

hmm - det er jo en god ide... ja...

Tror det er okay som jeg har det nu. Hvis der bliver brug for yderligere bestyttelse, så betyder det nok at sitet er blevet populært, hvorefter jeg nok alligevel er nødsaget til at anvende https.

Jeg takker for hjælpen.
Hvis i alle lige ligger et svar, så bliver der uddelt point :D

Det var et spændende spørgsmål du fik stillet - sikkerhed er altid interessant :).

Morten, hvis du ud fra en ikke-hemmelig kode og en md5 af en sammensætning af den med en hemmelig kode kan gætte den hemmelige kode, så vil jeg godtage dit argument. (Hint: Det kan du ikke.) Jeg er dog sikker på, om den kan brute-forces, man skal jo bare finde en tekst, der giver samme md5 som den hemmelige kode, og der er kun ca 340282366900000000000000000000000000000 muligheder. ;-) For at det kan blive interessant at prøve, må man nok automatisere indbrudsforsøget. Og det kan besværliggøres en hel del af noget i denne stil:

if( md5($POST[tid'].$pw_fra_db) == $_POST['password'] && $POST[tid']>$tid_fra_db){
  //login OK
} else {
  sleep(10);
}

Så vil det tage ti sekunder pr. forsøg, og ca 107902830708060141889705291549901,1325 år at prøve alle muligheder.

Hvis brugeren selv har valgt den hemmelige kode, er det dog ofte noget lettere - brugere har jo en tendens til at vælge simple koder, der ofte kan være ganske lette at gætte, hvis man kender brugeren.

.. ikke for at vade rundt i det, men ..

Hvis jeg laver bruteforce, så automatisere jeg det.. Hvis jeg ikke får svar med det samme (inden for en tidsramme - man kan jo sammenligne med en hvor lang tid den er om at sende login-siden.. og så forvente at den er ligeså hurtig til at svare på selve login plus en tolerance), så ved jeg at den staller.. Jeg kan bare lukke forbindelse og sende en ny request..

Og jeg kan jo sende flere requests parallelt.. Fra flere maskiner.. osv.

Men ok. Jeg gider nok ikke at gå igang med at lave en brute-force :). Det er nok nemmere at komme ind andre veje, hvis man endelig vil.

Dennismp: "Det var et spændende spørgsmål du fik stillet - sikkerhed er altid interessant :)"

- ja ik :)
Er altid rart at forsætte med "arbejdet", når man er lidt mere tryg ved sikkerheden :)