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"


  • FoxDev

    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 οƒΉ



  • 😧 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.

    πŸ˜† that's in the list of things judged unlikely


  • BINNED

    @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 πŸ˜„


  • 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. πŸ˜„

    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 πŸ˜ƒ



  • @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`.


  • @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.


  • Discourse touched me in a no-no place

    @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.


  • Java Dev

    @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 );
    

  • BINNED

    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?


  • BINNED

    @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?


  • BINNED

    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?


  • BINNED

    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 πŸ˜†



  • @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;
    

  • BINNED

    @flabdablet said:

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

    FTFY


  • Trolleybus Mechanic

    @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:


  • 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.


  • BINNED

    @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


  • Java Dev

    @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.


  • BINNED

    @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.



  • @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:

    πŸ˜† 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.


  • @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 πŸ˜› 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>


  • πŸ˜†

    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);


  • BINNED

    @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".


  • Discourse touched me in a no-no place

    @Onyx said:

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

    Is he aiming to be a deputy under-blakey?



  • @thegoryone said:

    @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

    I do not know the original state of $Config['enabled'] and I have no control over how the widget reads it. It's real simple. The documentation says I should set it to a boolean so that's what I'm gonna do. Leaving an important config variable unset is a fucking no-go. You're ignoring the whole context and come up with your own reality of how things ought to be then attack me when I tell you they are not like that. Did you even read my first post in this thread?



  • @gleemonk said:

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

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

    They don't teach you that on the Zend Certification πŸ˜›


  • Discourse touched me in a no-no place

    @Arantor said:

    Zend Certification

    I thought the only PHP-related certification was:


  • FoxDev

    .... do you have a higher resolution version of that available?

    cause i want to frame that


  • Discourse touched me in a no-no place

    @accalia said:

    .... do you have a higher resolution version of that available?

    GIS does.



  • I do have the official embossed 'you can PHP' certificate with the Zend logo on it. It confers much street cred amongst my coworkers who have declared me, on multiple occasions, a 'Zend guru'.

    I also have this one:



  • @Arantor said:

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

    They don't teach you that on the Zend Certification πŸ˜›

    That's the bestest line for this situation. I don't like the double negation, but it's a common pattern so I don't trip over it much.


Log in to reply