Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.
Moderator: General Moderators
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 1:21 pm
I'm aware that absolutely nothing is encrypted, but besides that, is this a security flaw?
// My included security check function.
Code: Select all
// Return true if logged in.
function bverify_login($bIsAdmin = 0)
{
global $logins_table;
if ( (isset($_COOKIE["tj_email"])) && (isset($_COOKIE["tj_password"])) )
{
$input_email = $_COOKIE["tj_email"];
$input_password = $_COOKIE["tj_password"];
}
else if ( (isset($_POST["login"])) && (isset($_POST["password"])) )
{
$input_email = $_POST["login"];
$input_password = $_POST["password"];
}
else
return false;
$result = mysql_query(" SELECT * FROM $logins_table WHERE email = '$input_email' ");
$found = mysql_fetch_row($result);
if ( $found[12] != '') // is activated?
return false;
if ( $found[2] == $input_password )
{
if ( $found[10] >= $bIsAdmin )
return true;
return false;
}
return false;
}
// This is before the header include in every page that needs protection, this particular example checks for admin rights, checking for user rights is 0.
Code: Select all
if ( !bverify_login(1) )
header("Location: index.php?message=no_permissions");
Ambush Commander
DevNet Master
Posts: 3698 Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US
Post
by Ambush Commander » Mon Nov 07, 2005 2:24 pm
It's not secure. It stores the password in a cookie as cleartext. There are other issues, but we'll talk about this one first.
Weirdan
Moderator
Posts: 5978 Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine
Post
by Weirdan » Mon Nov 07, 2005 2:44 pm
Ambush Commander wrote: It's not secure. It stores the password in a cookie as cleartext. There are other issues, but we'll talk about this one first.
Personally, I would rate possible sql injection as much more severe flaw comparing to plain-text password in cookies.
Ambush Commander
DevNet Master
Posts: 3698 Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US
Post
by Ambush Commander » Mon Nov 07, 2005 2:47 pm
Ah, you're right. Missed that. Hmph... that's why you should always use bind sql. Clears a bunch of messy problems that occur when you forget to escape output.
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 3:20 pm
Ambush Commander wrote: It's not secure. It stores the password in a cookie as cleartext. There are other issues, but we'll talk about this one first.
Yes, are you refering to encryption? I stated in my post that I'm already aware of this. What are some of the other issues?
Ambush Commander
DevNet Master
Posts: 3698 Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US
Post
by Ambush Commander » Mon Nov 07, 2005 3:24 pm
As stated above, SQL injection. input_email isn't escaped, so an attacker can execute an arbitrary SQL statement.
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 3:29 pm
Ambush Commander wrote: As stated above, SQL injection. input_email isn't escaped, so an attacker can execute an arbitrary SQL statement.
how would i go about doing this? is it a php function?
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 3:38 pm
i good place to start is probably Strip_Tags(), I'm not sure how I can filter SQL statements however.
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 3:44 pm
for SQL it doesn't, but it will prevent some javascript on random html tags, or some XSS.
ahh, this will help alot. Is there any other serious and obvious issues i should be worried about?
Ambush Commander
DevNet Master
Posts: 3698 Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US
Post
by Ambush Commander » Mon Nov 07, 2005 3:48 pm
Well, it looks like you've got a fairly large application, so consider using a Database wrapper. It will help prevent future SQL injection attacks.
Once again, don't put passwords as cleartext in cookies. Instead, use remember me tokens (no encryption required).
Last edited by
Ambush Commander on Mon Nov 07, 2005 3:50 pm, edited 1 time in total.
foobar
Forum Regular
Posts: 613 Joined: Wed Sep 28, 2005 10:08 am
Post
by foobar » Mon Nov 07, 2005 3:49 pm
Dark[NSF] wrote:
for SQL it doesn't, but it will prevent some javascript on random html tags, or some XSS.
Which has nothing to do with your question. Ambush commander already gave you the answer.
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 3:54 pm
foobar wrote: Dark[NSF] wrote:
for SQL it doesn't, but it will prevent some javascript on random html tags, or some XSS.
Which has nothing to do with your question. Ambush commander already gave you the answer.
I was refering to security as a whole, strip_tags() is probably a good place to start. i'm not saying he's wrong.
Ambush Commander
DevNet Master
Posts: 3698 Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US
Post
by Ambush Commander » Mon Nov 07, 2005 4:08 pm
A login system also needs to manage sessions. Can you post that code for a security audit too?
Dark[NSF]
Forum Newbie
Posts: 18 Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:
Post
by Dark[NSF] » Mon Nov 07, 2005 4:13 pm
Ambush Commander wrote: A login system also needs to manage sessions. Can you post that code for a security audit too?
I was wondering if sessions were necessary, my current system doesn't use sessions. only cookies. i suppose that is a bad thing.
good thing this forum exists.