Take the 2-minute tour ×
Information Security Stack Exchange is a question and answer site for Information security professionals. It's 100% free, no registration required.

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";
        }

     ?>
share|improve this question
    
I assume you mean isset rather than isser? –  Polynomial Jan 3 '13 at 6:51

2 Answers 2

That snippet in itself is not really a safety 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 how the data got to $_SESSION['id']. Either before inserting it there or before using it, you need to do the proper safety checks, based on what are you going to use the value for. In your case, check for SQL injection AND you better use parametrized 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.

share|improve this answer
    
+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 '13 at 7:31

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.

share|improve this answer
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 '13 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 '13 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 '13 at 15:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.