0

I have developed a php code given below. I would like to know what are the security vulnerabilities present within this php code?

<?php
        if(isset($_SESSION['id'])){
        // get data from mysql table where id = $_SESSION['id'];
        // shows data

        }else{
        echo "pls login";
        }

     ?>
1
  • You code is incomplete, don't know if your using prepared statements or not. Always use prepare statements, otherwise you could get burned.
    – cybernard
    Jun 15, 2017 at 16:56

2 Answers 2

8

That snippet in itself is not really a security check, it just checks that a value exists before using it (good practice, but not security related). The meat of your code will be in:

from mysql table where id = $_SESSION['id'];

And it all depends on how the data got to $_SESSION['id']. Either before inserting it there or before using it, you need to do the proper security checks, based on what are you going to use the value for. In your case, check for SQL injection AND you are better off using parametrised queries.

If your SQL actually looks a bit like what you wrote, that is, a concatenation of strings with the $_SESSION['id'] unqouted as a numeric value, then, yes, this is VERY vulnerable.

1
  • +1, but don't forget session based attacks - such as session hijacking, session fixation, etc etc. Of course there is not enough context in the code to be sure, but the existance of this pattern makes those flaws to be likely.
    – AviD
    Jan 6, 2013 at 7:31
0

As long as you properly clear the session variables after the user has logged out, this should be fine. Whilst this particular session variable isn't injectable, do make sure you're using parameterised queries in all of your code.

3
  • 4
    From this code snippet you don't know if $_SESSION['id'] is or is not injectable. You are right - it's better to use parametrised queries in all your code, because you never know if given variable may be controlled by attacker.
    – pgolen
    Jan 3, 2013 at 7:08
  • @pgolen I agree that the snippet alone doesn't show it as injectable or not, but the standard pattern is to set $_SESSION['id'] to an internal ID, based on the username given as a login. Allowing the user ID to be passed manually wouldn't be very secure, since someone could just change it to a different ID and authenticate as them.
    – Polynomial
    Jan 3, 2013 at 8:24
  • I've seen quite a few application in which I was able to set any $_SESSION variable to value of my choice. Of course it was vulnerability by itself, but could also allow to exploit this kind of SQL queries. I think that it's better to be safe than sorry.
    – pgolen
    Feb 1, 2013 at 15:09

Not the answer you're looking for? Browse other questions tagged .