SECURITY: WARNING: Too long didn't read



  • So I'm investigating this malware upload. Site's been running undisturbed since 2009. Logs suggest that the javascript richtext-editor could be the culprit, or specifically, its upload feature. Behold the PHP config of the upload feature:

    // SECURITY: You must explicitly enable this "connector". (Set it to "true").
    // WARNING: don't just set "$Config['Enabled'] = true ;", you must be sure that only
    //		authenticated users can access this file or use some kind of session checking.
    $Config['Enabled'] = (bool)$_SESSION['user'] ;
    $Config['Enabled'] = true;
    


  • Wow. I guess that's absolutely what I (should) have come to expect from anything on TDWTF. How are people smart enough to understand this stuff but dumb enough to completely ignore the huge warning one line above it?


    Filed under: Inb4 "TRWTF is PHP"


  • sockdevs

    Use this on the dev responsible:

    Just make sure you clean it before you return it ;)



  • Who added the :wtf: "$Config['Enabled'] = true;" line?

    1. I did that myself to test whether it would work that way, then I forgot (I get the benefit of doubt)
    2. Somebody else did it to get it running (they don't get the benefit of doubt)
    3. All our installations are like this (fuckfuckfuck)

    Which one will it be?



  • Ooh, a game!

    My guess is both 2 and 3.



  • Fuck you might be right. Because the mtime of that config file is not newer than the mtime of all the other uploaded files :fa_ambulance:



  • :anguished: at least one other site has that config-version so it was probably in mainline for a while. But not in any version past 2012. That means only a few stragglers will be affected, which is a relief.



  • You forgot this one:

    1. Some other attack vector changed it.


  • @powerlord said:

    4) Some other attack vector changed it.

    :laughing: that's in the list of things judged unlikely


  • Discourse touched me in a no-no place

    @rc4 said:

    How are people smart enough to understand this stuff but dumb enough to completely ignore the huge warning one line above it?

    They stopped reading after the first line?

    // SECURITY: You must explicitly enable this "connector". (Set it to "true").



  • It looks like the attackers were unable to execute the code they uploaded. The uploader-script insisted on clamping to a benign filename extension. Security in depth saves the day.

    Good thing they didn't try harder :smile:


  • Discourse touched me in a no-no place

    @gleemonk said:

    1. All our installations are like this

    My guess is this one. Probably turned on to support some contractor way back. We've had ever so many angry contractors because our production websites run on systems with read-only filesystems. :smile:

    Filed under: Of course we log to off-host systems…



  • In all fairness, it took me a few reads to understand that line.

    "You must set it to "true" [in the requests where you want to give the user permission to use it]"



  • @dkf said:

    systems with read-only filesystems

    Defense in depth, the natural opponent of 'easy'.

    @dkf said:

    Of course we log to off-host systems…

    You know I had to read through some awstats logfiles because I didn't have access to the apache-log? The site sees almost no traffic, so this worked well :smiley:



  • @gleemonk said:

    // SECURITY: You must explicitly enable this "connector". (Set it to "true").
    // WARNING: don't just set "$Config['Enabled'] = true ;", you must be sure that only
    // authenticated users can access this file or use some kind of session checking.
    $Config['Enabled'] = (bool)$_SESSION['user'] ;
    $Config['Enabled'] = true;

    Nice. Really nice. What's wrong with

    if (isset($_SESSION['user'])) {
    $Config['Enabled'] = true;
    }


  • @thegoryone said:

    Nice. Really nice. What's wrong with

        if (isset($_SESSION['user'])) {
        $Config['Enabled'] = true;
        }
    ```</blockquote>
    
    The indendation is wrong!  Also the widget is enabled even if `$_SESSION['user'] === false`.


  • @gleemonk said:

    The indendation is wrong! Also the widget is enabled even if $_SESSION['user'] === false.

    What is this, school?

    $_SESSION['user'] === false.

    That's because it says $Config['enabled'] = true right after the session check.

    isset() returns boolean true if the variable is set but empty if memory serves, but $_SESSION[user] === false will return false correctly on empty or null, true. Perhaps you have a faulty session unset? Maybe the system does $_SESSION['user'] == ""; instead of unset($_SESSION['user'])?



  • @thegoryone said:

    What is this, school?

    Welcome to TDWTF.

    @thegoryone said:

    That's because it says $Config['enabled'] = true right after the session check.

    Congratulations, you found the :wtf:

    @thegoryone said:

    isset() returns boolean true if the variable is set but empty if memory serves, but $_SESSION[user] === false will return false correctly on empty or null, true.

    I'm sorry I don't understand what your angle is. To me

    $Config['Enabled'] = (bool)$_SESSION['user'] ;
    

    is a lot more robust compared to

    if (isset($_SESSION['user'])) {
        $Config['Enabled'] = true;
    }
    

    I prefer the first version because

    1. It always sets the config var, so we don't have to care how it could have been set before.
    2. It looks for truthiness, not whether it's set. That's stricter. (Still not verystrict)
    3. Oh and it's easier to read and understand

    @thegoryone said:

    Perhaps you have a faulty session unset? Maybe the system does $_SESSION['user'] == ""; instead of unset($_SESSION['user'])?

    It doesn't matter. If you want to propose improvements, maybe propose making the check stricter, not to loosen it further. In a security context you shouldn't rely on subtle differences between null and false.



  • @gleemonk said:

    $Config['Enabled'] = (bool)$_SESSION['user'] ;

    Is type casting (which isn't ideal because PHP), and isn't robust. You shouldn't be type casting for security.

    Ideally what you would have is a random hashed CSRF token entered on user login with an expiry set to the same as your session timeout, which matches a database entry for the username and if($Config['enabled'] === $_SESSION['user']), or make sure that the session is being unset or destroyed correctly and you can just use if(isset($_SESSION['user']))which is perfectly robust if the session management is actually done right. That's if you're talking about security. As an added benefit the CSRF token helps prevent (in part, at least) cross site scripting attacks. So yeah, security.



  • @thegoryone said:

    Ideally what you would have is

    ... not PHP.



  • @thegoryone said:

    isset($_SESSION['user'])

    Passing a variable to a function to see if the variable exists always struck me as odd.



  • @ben_lubar said:

    @thegoryone said:
    isset($_SESSION['user'])

    Passing a variable to a function to see if the variable exists always struck me as odd.

    Yeah, it's cleaner to write

    array_key_exists( 'user', $_SESSION );
    

  • :belt_onion:

    Am I the only one annoyed by potential warnings this would produce if the variable isn't actually set?

    // Just in case there is a situation in which $_SESSION['user'] is falsy
    $Config['Enabled'] = (isset($_SESSION['user']) && $_SESSION['user']); 
    

    Or does anyone have a problem with that one?



  • Why do you think I mentioned isset too? You could just ternary it though.

    $Config['enabled'] = (isset($_SESSION['user']) AND !empty($_SESSION['user'])) ? true : false;
    

    ITT: A lot of people who've never actually put access control into PHP scripts (correctly). isset() is the commonly accepted way to check if a session variable actually exists.

    array_key_exists( 'user', $_SESSION );
    

    Generally no, see this link as to why.. It's not that it doesn't work (It does) but isset() is able to handle error cases that array_key_exists() does not. Both are functionally secure.

    Also most PHP frameworks and publications tend to use isset() as a standard

    @ben_lubar said:

    Passing a variable to a function to see if the variable exists always struck me as odd.

    PHP is great, isn't it?



  • @gleemonk said:

    I'm sorry I don't understand what your angle is. To me

    $Config['Enabled'] = (bool)$_SESSION['user'] ;

    Do this without checking the array key* exists first and what happens?

    Notice: Undefined index: user in C:\xampp\htdocs\check.php on line 3
    

  • :belt_onion:

    @thegoryone said:

    You could just ternary it though.

    Both isset and empty return a boolean TRUE or FALSE, what do you even need the ternary for?



  • @Onyx said:

    @thegoryone said:
    You could just ternary it though.

    Both isset and empty return a boolean TRUE or FALSE, what do you even need the ternary for?

    Empty returns false on an empty string, isset returns true on an empty string if the variable exists (i.e. $_SESSION['user'] = "". Security.

    Edit: Also because it looks like that script he's validating is a massive clusterfuck that isn't handling session variables correctly so it can't hurt.


  • :belt_onion:

    That's not what I asked. I asked what the point of

    (bool && bool) : true : false

    is, when the && operator will do the same anyway without any extra checks?



  • This post is deleted!


  • Bad habits. It's PHP after all ;) You're right after all

    Edit: I think we can both agree though that the OP is at fault here


  • :belt_onion:

    No sweat. With the amount of ternaries I have in my code just do deal with this kind of shit I probably have at least one of those somewhere, waiting for someone to find it and post it to the sidebar :laughing:



  • @Onyx said:

    Both isset and empty return a boolean TRUE or FALSE

    To be on the safe side you should clearly use

    $Config['enabled'] = ((isset($_SESSION['user']) ? true : false) AND (empty($_SESSION['user']) ? false : true)) == true ? true : false;
    

  • :belt_onion:

    @flabdablet said:

    To be on the safe side you should clearly not use PHP

    FTFY



  • @flabdablet said:

    @Onyx said:
    Both isset and empty return a boolean TRUE or FALSE

    To be on the safe side you should clearly use

    $Config['enabled'] = ((isset($_SESSION['user']) ? true : false) AND (empty($_SESSION['user']) ? false : true)) == true ? true : false;
    ```</blockquote>
    
    I'll create a function for that:
    
    ```myphp_check_variable_is_set_or_empty```
    
    Wait, fucked it up, but it's in core now.  Okay, NEW new function:
    
    ```myphp_real_check_variable_is_set_or_empty```
    
    SHIT fucked it up again, but it's in core.  Okay, let's just fork it and rename it to handle the fucked up case I accidentally introduced:
    
    ```myphp_real_check_variable_is_set_or_empty_or_not_null```
    
    Can we put in the docs though-- this will return true or false or null, so be sure people do type checking:
    
    ```if (false !== myphp_real_check_variable_is_set_or_empty_or_not_null(variable)) ... ```


  • @thegoryone said:

    @gleemonk said:
    I'm sorry I don't understand what your angle is. To me

    $Config['Enabled'] = (bool)$_SESSION['user'] ;

    Do this without checking the array key* exists first and what happens?

    Notice: Undefined index: user in C:\xampp\htdocs\check.php on line 3
    ```</blockquote>
    
    :laughing: That is the least of my problems. You know that systems in production are supposed not to show these warnings? I don't actually have a problem with sticking a call to `isset()` in front of it to silence the warning. Let's see whether in this case I can muster a fuck.... ah no.
    
    @thegoryone <a href="/t/via-quote/51796/19">said</a>:<blockquote>Ideally what you would have is a random hashed CSRF token entered on user login with an expiry set to the same as your session timeout, which matches a database entry for the username and `if($Config['enabled'] === $_SESSION['user'])`</blockquote>
    
    :wtf: you realize this is for a third-party component and I wouldn't dare sticking anything which is not a boolean into `$Config['enabled']`? Somehow the part about having to ensure `$Config['enabled']` is set to something sensible (a boolean!) seems to escape you completely.
    
    @thegoryone <a href="/t/via-quote/51796/19">said</a>:<blockquote>or make sure that the session is being unset or destroyed correctly and you can just use if(isset($_SESSION['user']))which is perfectly robust if the session management is actually done right.</blockquote>
    
    You continue to argue that I could change the semantics of this code to just use `isset()`. Why do you insist that unsetting is the only 'correct' way? In this case the variable happens to be a numeric id and setting it to zero may well indicate no logged-in user. I might actually want to fail loudly when that value is not set to a number because that means some earlier check was not performed. I don't think that's how this system works but why would that be 'not done right'?
    
    
    @thegoryone <a href="/t/via-quote/51796/24">said</a>:<blockquote>`? true : false;`</blockquote>
    
    Please tell me more about this feature :smiley:
    
    @thegoryone <a href="/t/via-quote/51796/30">said</a>:<blockquote>Edit: I think we can both agree though that the OP is at fault here</blockquote>
    
    :sadface:

  • sockdevs

    That's not the only reason people do false !== thing. It's a habit that generally prevents people from doing crap like if (false = expression) and here just taken to the extreme.

    That and some functions return 0 as a legitimate positive result and actual false to mean negative result, e.g. strpos.



  • @gleemonk said:

    :laughing: That is the least of my problems. You know that systems in production are supposed not to show these warnings? I don't actually have a problem with sticking a call to isset() in front of it to silence the warning. Let's see whether in this case I can muster a fuck.... ah no.

    One of us is a terrible PHP developer. It's not me.The only reason to disable these errors is for portability with older PHP installs, otherwise do the job right and the errors won't be there in the first place. Hiding errors and saying everything is ok is not a valid approach for any developer, it's utterly TRWTF.

    @gleemonk said:

    :wtf: you realize this is for a third-party component and I wouldn't dare sticking anything which is not a boolean into $Config['enabled']? Somehow the part about having to ensure $Config['enabled'] is set to something sensible (a boolean!) seems to escape you completely.

    That's not what I actually said, which shows you have no idea what I'm talking about, strike 2. if($Config['enabled'] === $_SESSION['user']) will return true or false, but isn't exactly best practice; It's still better than what you have. Ideally you'd use if(isset($Config['enabled']) && $Config['enabled']).

    @gleemonk said:

    You continue to argue that I could change the semantics of this code to just use isset(). Why do you insist that unsetting is the only 'correct' way? In this case the variable happens to be a numeric id and setting it to zero may well indicate no logged-in user. I might actually want to fail loudly when that value is not set to a number because that means some earlier check was not performed. I don't think that's how this system works but why would that be 'not done right'?

    Unsetting/destroying via session_unset() is the correct way, that's why it exists. You're arguing in favor of an anti-pattern. Why would you set a session variable used for security to 0? Why? What possible reason do you have for that? That creates more work validating whether or not it's a valid value. Both ways technically leave you open to session hijacking which is exactly why I mentioned CSRF tokens since you kept babbling on about security. Your argument is full-blown :wtf:.

    @gleemonk said:

    Please tell me more about this feature :smiley:

    Can't tell if sarcastic...

    $var = (isset($var)) ? true : false;

    is the same as

    if (isset($var)) { $var = true } else { $var = false; }

    @gleemonk said:

    :sadface:

    @Arantor, where are you?


  • :belt_onion:

    @thegoryone said:

    One of us is a terrible PHP developer. It's not me.The only reason to disable these errors is for portability with older PHP installs, otherwise do the job right and the errors won't be there in the first place. Hiding errors and saying everything is ok is not a valid approach for any developer, it's utterly TRWTF.

    QFFT

    @thegoryone said:

    Unsetting/destroying via session_unset() is the correct way, that's why it exists. You're arguing in favor of an anti-pattern. Why would you set a session variable used for security to 0? Why? What possible reason do you have for that? That creates more work validating whether or not it's a valid value. Both ways technically leave you open to session hijacking which is exactly why I mentioned CSRF tokens since you kept babbling on about security. Your argument is full-blown :wtf:.

    See above.



  • @thegoryone said:

    One of us is a terrible PHP developer. It's not me.The only reason to disable these errors is for portability with older PHP installs, otherwise do the job right and the errors won't be there in the first place. Hiding errors and saying everything is ok is not a valid approach for any developer, it's utterly TRWTF.

    Maybe you didn't read that part but I don't have access to the Apache logs on this system. So what good would enabling notices do? They might be turned on for all I know.

    Note how I continue not to give fuck? I'm not going to barge in and change code just to silence some notices I will never see.

    @thegoryone said:

    if($Config['enabled'] === $_SESSION['user']) will return true or false, but isn't exactly best practice

    Would you be kind enough to explain in what scenario writing if($Config['enabled'] === $_SESSION['user']) would be an option? Because $Config['enabled'] is supposed to hold a boolean and $_SESSION['user'] should not ever contain a boolean in your scenario. What's the purpose of comparing the two?

    @thegoryone said:

    Ideally you'd use if(isset($Config['enabled']) && $Config['enabled'])

    No I would not do that for the simple reason that I never read $Config['enabled']. I'm setting it. What are you talkling about?

    @thegoryone said:

    Unsetting/destroying via session_unset() is the correct way, that's why it exists. You're arguing in favor of an anti-pattern. Why would you set a session variable used for security to 0? Why? What possible reason do you have for that?

    Maybe I like the invariant of always having a number there. I don't, but I'm asking why this should be in any way less secure than unsetting the variable in the session. You still haven't explained that. Of course unsetting the whole session is more secure because it's simpler, but we weren't talking about that.

    @thegoryone said:

    Can't tell if sarcastic...

    I was being sarcastic. I've cleaned a lot of unnecessary logic out of PHP code.

    @thegoryone said:

    $var = (isset($var)) ? true : false;

    is the same as

    if (isset($var)) { $var = true } else { $var = false; }
    $var = isset($var);


    There, you triggered my compulsive cleanup reflex.
    [1]: http://php.net/manual/en/errorfunc.configuration.php#ini.display-errors



  • @thegoryone said:

    The only reason to disable these errors is for portability with older PHP installs, otherwise do the job right and the errors won't be there in the first place.

    Other reasons not to print them in the output include preventing corruption of non-HTML output such as AJAX responses, and preventing information leaks to potential attackers if a notice slips through.

    These errors belong in one of your server logs, where you can review them.


  • :belt_onion:

    @PleegWat said:

    Other reasons not to print them in the output include preventing corruption of non-HTML output such as AJAX responses, and preventing information leaks to potential attackers if a notice slips through.

    Which is why I turn them off as well in production, just in case one slips by all the testing.

    The point was that @gleemonk's original statement sounded more like "I don't give a shit about that, not in dev, not in test, not in prod. Ever."

    If that's not the case then it was poorly phrased.



  • @gleemonk said:

    Maybe you didn't read that part but I don't have access to the Apache logs on this system. So what good would enabling notices do? They might be turned on for all I know.

    Quite right, I didn't see you mention that. Fair enough. Still not a valid excuse for not doing it right, however.

    @thegoryone said:

    if($Config['enabled'] === $_SESSION['user']) will return true or false, but isn't exactly best practice

    Perfectly valid way to check the user key after you typecast it to bool and already have your config variable set, like was done in the initial code provided.

    @gleemonk said:

    Would you be kind enough to explain in what scenario writing if($Config['enabled'] === $_SESSION['user']) would be an option? Because $Config['enabled'] is supposed to hold a boolean and $_SESSION['user'] should not ever contain a boolean in your scenario. What's the purpose of comparing the two?

    See above. That was simply a "Well, you could do this after to make sure it's valid".

    @gleemonk said:

    No I would not do that for the simple reason that I never read $Config['enabled']. I'm setting it. What are you talkling about?

    Shit. Well, got me there. That's without a doubt my own stupidity. Should be

    if(isset($_SESSION['user']) && $_SESSION['user']) {
      $Config['enabled'] = true;
    }
    

    @gleemonk said:

    Maybe I like the invariant of always having a number there. I don't, but I'm asking why this should be in any way less secure than unsetting the variable in the session. You still haven't explained that. Of course unsetting the whole session is more secure because it's simpler, but we weren't talking about that.

    [b]Of course unsetting the whole session is more secure because it's simpler, but we weren't talking about that.[/b]

    I said the key, not the entire session, but either way you clearly know why

    @gleemonk said:

    if (isset($var)) { $var = true } else { $var = false; }
    $var = isset($var);
    There, you triggered my compulsive cleanup reflex.
    [1]: http://php.net/manual/en/errorfunc.configuration.php#ini.display-errors

    Code example. Not to be taken seriously as production code. Also nobody likes people who don't indent if statements, be it tabs or spaces (that's been done to death really)



  • @Onyx said:

    The point was that @gleemonk's original statement sounded more like "I don't give a shit about that, not in dev, not in test, not in prod. Ever."

    If that's not the case then it was poorly phrased.

    @gleemonk said:

    :laughing: That is the least of my problems. You know that systems in production are supposed not to show these warnings? I don't actually have a problem with sticking a call to isset() in front of it to silence the warning. Let's see whether in this case I can muster a fuck.... ah no.

    Translation: I'm not going to change production code just to placate somebody I don't know.



  • @thegoryone said:

    Still not a valid excuse for not doing it right, however.

    In this context doing it right means not touching it. And I'm sloppy when it comes to separating development and production. If ignoring the possibility of a PHP notice on a 5 year old production system makes me a terrible developer in your eyes, what the fuck are you? A cowboy developer?

    @thegoryone said:

    @thegoryone said:
    if($Config['enabled'] === $_SESSION['user']) will return true or false, but isn't exactly best practice

    Perfectly valid way to check the user key after you typecast it to bool and already have your config variable set, like was done in the initial code provided.

    :wtf: You want me to change the widget's code? Why would I do that?

    @thegoryone said:

    ```php
    if(isset($_SESSION['user']) && $_SESSION['user']) {
    $Config['enabled'] = true;
    }

    
    You're still not addressing the fact that this would not set `$Config['enabled']` when no user is logged in. Why not
    
    

    $Config['enabled'] = isset($_SESSION['user']) && $_SESSION['user'];

    ?
    
    @thegoryone <a href="/t/via-quote/51796/42">said</a>:<blockquote>I said the key, not the entire session, but either way you clearly know why</blockquote>
    
    Then why bring up `session_unset()`? And no I don't know why because you haven't said anything of substance. You just insisted that using `unset($_SESSION['user'])` was better than `$_SESSION['user'] = 0` because you also insisted that doing `isset($_SESSION['user'])` should be enough to check whether a user was logged in.

  • sockdevs

    @thegoryone said:

    Can't tell if sarcastic...

    $var = (isset($var)) ? true : false;

    @Arantor, where are you?

    $var = isset($var);

    This is still less :wtf: y than the crap I deal with :stuck_out_tongue: Oh wait, :hanzo:'d

    Though we still have a shit-ton of if ($_POST['sentSomething'] == 'true') checks to fix.

    I also habitually use empty() rather than isset just to deal with various fuckery that might exist around me.



  • empty() is the dirty brother of the inbreed isset().



  • @thegoryone said:

    @gleemonk said:
    I'm sorry I don't understand what your angle is. To me

    $Config['Enabled'] = (bool)$_SESSION['user'] ;

    Do this without checking the array key* exists first and what happens?

    Notice: Undefined index: user in C:\xampp\htdocs\check.php on line 3
    ```</blockquote>
    
    

    @$Config['Enabled'] = (bool)$_SESSION['user'] ;

    
    <abbr title="PHP'd That For You">PTFY</abbr>


  • :laughing:

    Me I just disable error_reporting when I know some code might throw notices :trollface:

    $err = error_reporting(0);
    $Config['Enabled'] = (bool)$_SESSION['user'] ;
    error_reporting($err);
    

    This avoids nasty warnings when ini_set('scream.enabled', true);



  • @gleemonk said:

    You're still not addressing the fact that this would not set $Config['enabled'] when no user is logged in. Why not

    If it's not set, it'll return false on the isset() check. Are you for real? This whole conversation thread addresses this at least half a dozen times


  • :belt_onion:

    @Onyx said:

    Am I the only one annoyed by potential warnings this would produce if the variable isn't actually set?

    // Just in case there is a situation in which $_SESSION['user'] is falsy
    $Config['Enabled'] = (isset($_SESSION['user']) && $_SESSION['user']);
    

    Or does anyone have a problem with that one?

    @gleemonk said:

    I don't actually have a problem with sticking a call to isset() in front of it to silence the warning. Let's see whether in this case I can muster a fuck.... ah no.

    Confirming @gleemonk's current location is "under a bridge".


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.