Page 1 of 1

Login security

Posted: Mon May 28, 2007 4:31 am
by ddragas
Hi all

I'm worried about login security of my script, so I'm asking if somebody could check code and give me some advices on security

Here is code:

Code: Select all

<?
session_start();
function filter($string)
        {
				if (!$conn)
				{
					include("../con_db.php");
				}				
$forbiden=array("SELECT", "DELETE", "UPDATE", "INSERT", "insert", " or ", " OR ", "select", "delete", "update", "{", "}", "[", "]", "(", ")", "&", "#", "$", "!", "=", "%");
$change = "";
                $filtered  = str_replace($forbiden, $change, $string);
                return mysql_real_escape_string($filtered);
        } 
?>
<?


if(isset($_POST['prihvati']))
{

if(isset($_SESSION['nastavnik']))
	{
		$nastavnik = $_SESSION['nastavnik'] ;
	}


if((isset($_POST['nastavnik'])) and (isset($_POST['lozinka'])))
	{
		$kor_ime = filter($_POST['nastavnik']);
		$loz = filter($_POST['lozinka']);
		$lozinka = md5($loz);

		include("../admin/con_db.php");
		
		
		$sel = "SELECT * FROM nastavnici where email = '$kor_ime' and lozinka_md5 = '$lozinka'"; 
		$quer = mysql_query($sel); 
		$row = mysql_fetch_array($quer); 
		$nastavnik = $_SESSION['nastavnik'] = $row["id"];
		$nivo = $_SESSION['nivo'] = $row["nivo"];
		$Ime =  utf8_decode(stripslashes($row["ime"]));
		$Prezime = utf8_decode(stripslashes($row["prezime"]));
		

	}		

if(isset($nastavnik))
	{
		include("../admin/con_db.php");
		
		
		$sel = "SELECT * FROM nastavnici where id='$nastavnik'"; 
		$quer = mysql_query($sel); 
		$row = mysql_fetch_array($quer); 
		$nastavnik = $_SESSION['nastavnik'] = $row["id"];
		$nivo = $_SESSION['nivo'] = $row["nivo"];
		$Ime =  utf8_decode(stripslashes($row["ime"]));
		$Prezime = utf8_decode(stripslashes($row["prezime"]));
		

	}

if(empty($nastavnik))
	{
		echo "<br>";
		echo "Do&#313;&#711;lo je do gre&#313;&#711;ke u autorizaciji korisnika."; 
		echo "<br>";
		echo "Postoji mogućnost nekoliko gre&#313;&#711;aka.";
		echo "<br><br>";
		
		echo "<li>korisniÄ&#356;ko ime ili lozinka koju ste upisali ne nalazi se u bazi podataka, ili je pogre&#313;&#711;no upisana.</li>"; 
		echo "<br><br>";
		echo "<li>razlika je u velikim/malim slovima u lozinci.</li></ul>";
		echo "<br>";
		echo "<br>";
		print  "Molim probajte <a href='index.php'>ponovo</a>"; 
		session_destroy();
	}
		else
	{
		
		echo "Dobrodo&#313;&#711;li " . $Ime . " " . $Prezime;
		echo "<br>\n" . "<br>\n" . "PriÄ&#356;ekajte trenutak ..........";
		echo "<meta http-equiv=\"refresh\" content=\"1;URL='main.php'\">\n";
	}
	
}

?>
regards

Posted: Mon May 28, 2007 9:42 am
by Mordred
This "smells" in several ways:

1. The conditional include (and also include("../admin/con_db.php"); below) -- you maybe mean require_once() ?
2. The str_replace is wrong in two ways - it is not needed if you do mysql_real_escape_string(), and its place is not in a generic filter function. What about a user that wants "insert" for a password?
3. stripslashes in database code is wrong. Maybe you have magic_quotes on? If so, either turn it off in your config, or strip post/get/request etc from the slashes in the beginning of your script.

4. Just as a "style improvement" -- kor_ime, nastavnik, email -- pick one and stick to it throughout ;). Not to mention that $_SESSION['nastavnik'] is the id, whereas $_POST['nastavnik'] is the email.

5. The second query, $sel = "SELECT * FROM nastavnici where id='$nastavnik'"; is superfluous. You might as well keep these in the session.

Otherwise it looks secure.

Posted: Mon May 28, 2007 9:56 am
by superdezign
Also, you'll easily run include("../admin/con_db.php"); twice without using else-if.

And filter() has simply gotta go. It's useless. mysql_real_escape_string exists so that we don't waste time writing our own filter functions. SQL injection can't get through it.

Posted: Mon May 28, 2007 10:24 am
by Mordred
superdezign wrote:SQL injection can't get through it.
Most SQL injections can't get through it. I'm in the process of writing an article on that.

Btw, a possible (good) use of your own filter function is if you want to have an abstraction over the underlying database. So your filter will call mysql_real_escape_string() if you're using mysql, pg_escape_string() for postgre, etc.

Posted: Mon May 28, 2007 10:26 am
by superdezign
Mordred wrote:
superdezign wrote:SQL injection can't get through it.
Most SQL injections can't get through it. I'm in the process of writing an article on that.
Security buff. :-p

I was under the impression that the function was written as <b>the</b> prevention method. They missed some stuff, huh?

Posted: Mon May 28, 2007 2:11 pm
by ddragas
thank you both for reply and advice. I'll move away second query.
Another question.
How can I protect script (site) from all sql injections and other hack stuff (if there are any?)?

regards
ddragas

Posted: Mon May 28, 2007 2:29 pm
by John Cartwright
If memory serves, you'll need to apply trim() on all incoming data in combination with mysql_real_escape_string() to prevent sql injection

Posted: Mon May 28, 2007 2:51 pm
by ddragas
and that is all?
are there some other HACK stuff that I should avoid in some way and protect my site ?

Posted: Mon May 28, 2007 2:56 pm
by s.dot
What happens if someones email validly contains one of your filtered words?

Posted: Tue May 29, 2007 1:25 am
by ddragas
Yes - you're right, but only by this way (I think) sql injections will not be executed. That's the reason why I've maked it in this way.