Is this Secure?

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:

Is this Secure?

Post by Dark[NSF] »

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");
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

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.
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

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. :twisted:
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

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] »

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?
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

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] »

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] »

i good place to start is probably Strip_Tags(), I'm not sure how I can filter SQL statements however.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

strip_tags doesn't have anything to do with it.

Try... mysql_real_escape_string()
Dark[NSF]
Forum Newbie
Posts: 18
Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:

Post by Dark[NSF] »

Ambush Commander wrote:strip_tags doesn't have anything to do with it.

Try... mysql_real_escape_string()

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?
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

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 »

Dark[NSF] wrote:
Ambush Commander wrote:strip_tags doesn't have anything to do with it.

Try... mysql_real_escape_string()

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] »

foobar wrote:
Dark[NSF] wrote:
Ambush Commander wrote:strip_tags doesn't have anything to do with it.

Try... mysql_real_escape_string()

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.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

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] »

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.
Post Reply