• (cs) in reply to Still A Lurker
    Anonymous:
    <font color="#000099">Quote:
    ---------
    function</font> terminate() {
      <font color="#000099">exec</font>(<font color="#990000">"rm -r /var/public_www/$_SESSION['Tempfile']"</font>);
    }
    ---------


    I'm pretty sure this won't work..."rm -r" SHOULD prompt for confirmation...now "rm -rf" on the other hand...

    captcha = (<


    It only asks for confirmation when you are root, and that's because most shells set the -i (interactive) option as an alias for rm in the default profile rc file.  This is why you need to use -f while root to delete an entire directory tree.  The idea is that when you are root, you have the power to delete the entire filesystem, so better set interactive mode as default.

        -dZ.

  • (cs)

    I'll probably get some heat for this, but I actually like PHP. Unfortunately it's morons like this who propogate the stereotype that PHP is for newbies and shouldn't be used for "real" sites. Of course you can screw up in any language, I guess PHP just makes it easier. I've done a lot of different sites and never saw the need to use exec()...I'm sure there is one I just can't think of it. I feel sorry for Edward having to read 10 hours of that crap, I guarantee there was plenty of global variables as well...(blech)

  • (cs) in reply to Henrique
    Anonymous:
    I believe rm special-cases / and always asks for confirmation in that case.  I haven't tested, though.


    nope.  check your profile rc file and probably rm is aliased with -i for interactive mode.  This is usually on the root profile only, though.

       -dZ.
  • Deparment of Redundancy Department (unregistered) in reply to snoofle
    "I'm not a web person - can someone please explain why, upon the session timing out, this would delete all the files as opposed to just the temp files?"

    It's not a web thing -- it's an Enterprisey thing.
  • (cs) in reply to OneFactor

    I hate to be pedantic here, but...

    John Connor: Did you terminate all those files?
    Terminator: Esta la Vista, baby.

    "Esta la Vista" doesn't really mean anything, unless, badly translated, you interpret it to mean "this is the view."

    You were thinking "Hasta la vista," which is "goodbye," or "until I see you" (or you see me, or we see each other--it's a colloquialism that doesn't really have a literal translation).

    Actually, I don't hate to be pedantic.

  • tq (unregistered) in reply to Still A Lurker
    Anonymous:
    <font color="#000099">Quote:
    ---------
    function</font> terminate() {
      <font color="#000099">exec</font>(<font color="#990000">"rm -r /var/public_www/$_SESSION['Tempfile']"</font>);
    }
    ---------


    I'm pretty sure this won't work..."rm -r" SHOULD prompt for confirmation...now "rm -rf" on the other hand...

    captcha = (<

    :-O  Um, the fact that we're reading about the result pretty much proves it does work.  BTW, you're thinking of "rm -i ..."

    This really isn't a colossally huge, blindingly stupid WTF.  It's just your average, everyday, stupid code-monkey sort of WTF.  Not bothering to sanity check before relying on a variable's contents?  Go back to school kid.

  • (cs) in reply to Otto
    Otto:

    The real WTF here is the "exec (rm..." in the first place. Use the normal file deletion functions, you shouldn't go to the shell for trivial shit like this. Going to the shell is a really easy way to accidentally introduce security risks. There should be an IQ test before allowing programmers to use functions like exec.

    I have seen this in actual production Java code:

    public void copyFile(String filename1, String filename2)
    {
        try {
            if (System.getProperty("os.name").indexOf("Windows") >= 0)
                Runtime.getRuntime().exec("copy " + filename1 + " " + filename2);
            else
                Runtime.getRuntime().exec("/bin/cp " + filename1 + " " + filename2);
        } catch (IOException e) { }
    }
  • DP (unregistered)

    How did edward have access to the source code of the application?

  • (cs)

    <FONT face=Tahoma>They should have placed this message in the site.

    "This page will self destruct in 5 minutes (or until your session times out)"



    </FONT>

  • (cs)

    Software that selflessly sacrifices itself to make the work a better place... nice!

  • Boner (unregistered) in reply to merreborn
    merreborn:


    "$_SESSION['Tempfile']" ALWAYS evaluates to "", while "$_SESSION[Tempfile]" can actually return data -- IIRC, PHP doesn't like it when you single quote array indices in a double quoted string.


    Correct:
    "$_SESSION['Tempfile']" # throws an error
    "$_SESSION[Tempfile]" # throws a notice
    "{$_SESSION['Tempfile']}" # correct

    This wouldn't actually work at all:
    exec("rm -r /var/public_www/$_SESSION['Tempfile']");

    This would be correct:
    exec("rm -r /var/public_www/{$_SESSION['Tempfile']}");
  • weg (unregistered) in reply to Volmarias
    Volmarias:
    Whoops. Really stupid to not have actually checked for this, but I wouldn't call it an absolute WTF.

    Yeah, I always check websites to see if they will delete themselves if I leave them alone for half an hour. Can't be too careful.
  • (cs) in reply to tster
    tster:
    That would introduce a race condition.  if it tested that at 24 minutes 59.99 seconds then the session expired and interupt was sent and session became undefined the same thing is going to happen.  granted there is a very small chance for it to happen.  but remember, if something can go wrong, it will.

    Not quite as the sessions are expired based upon the file timestamp, when ever the file is read it's timestamp is updated, and the files are read when you tell php to load the session (might be able to set it to auto load the session when the page loads, havn't used php in a while).

    I thought php allowed you to set code that would run when a session expired? As this would be the place to do this sort of clean up code, although this wouldn't negate the argument checking.

    On a side note this reminds me of some code produced by a co-worker to allow clients to browse files that we had uploaded to the site. Only the path to browse was passed in the query string, and wasn't validated. The server would quite happily allow users to browse c:\ to their hearts content. Surfice it to say, after seeing this I steered him away from all security related code and just let him design the layouts whilst I wrote all the code to make the page work.

  • Someone else (unregistered)
    Alex Papadimoulis:

    Finding this application wasn't easy, but after months of searching and evaluating, they found three strong vendors, each with a solid product, a good reputation, and a strong support plan. There was only one problem: they all were expensive (like, $50,000 expensive).

     

    The WTF starts here. I don't know what these guys make per month/hour, but spending months evaluating gets you through a pretty large chuck of $50,000. In the end when they went with the cheaper product, they probably spend 2-3 times as much evaluating the products as they spend on the software.

  • verisimilidude (unregistered) in reply to DP
    Anonymous:
    How did edward have access to the source code of the application?


    The same way the PHP interperteter gets access to it. 
  • Brad (unregistered)

    The real wtf is that this code exists at all.  It happens every time a session times out and a user tries another action.  I could see this slipping by shoddy testing, but this company must be the very first real user of the system.

    Captcha = genius.  I feel better already.

  • Wes (unregistered)

    The obvious fix...

    <font color="#000099">function</font> terminate() {
      <font color="#000099">exec</font>(<font color="#990000">"rm -r /var/public_www/$_SESSION['Tempfile']"</font>);
      
      //added to fix weird bug

      if(isEmpty(<font><font color="#990000">$_SESSION['Tempfile']</font></font>)) {
        exec("cp -R /var/backup_public_www /var/public_www");
      }
    }

  • (cs) in reply to Wes

    Well I know that the problem arises whenever the string is empty, but technically you should rather use "isset()" rather than "!empty()" to check if the session variable expired. It's probably not the case here but empty(0) returns true.

    I suspect that missing curly brackets are the clue leading to something more sophisticated in the original code. Maybe they have had a whole temp directories for each user/session  (bad variable naming, though) - that would explain the origin of " -r" (you should use rmdir() for that, though - unfortunately the directory must be empty [but empty in the other meaning than presented in this very WTF], which often pisses people off, as it requires a full scan and deleting files one-by-one [or so it seems]).

    Finally I must admit that this is not so obvious for me after the first reading that there is a thread of variable expiration - it's a bit counter intuitive. However, it is obvious  that the variable could be never set at all, so why nobody checks for that? I mean: if the code assumes that this field is always set, how did it manage to run at the first connection, when there is no session? I'd guess there must be some place in the script that checks if( there is no session ) {build some default session info}, so there must be something screwed up with this test and I'd like to see it!:)

    Also, as mentioned earlier, most often the script fully controls the content of the session variables, so it is not-so-insecure to use them inside exec(), but why risk?

  • Nick (unregistered) in reply to Colin

    So what exactly is the WTF here?  Is it
    a) That the code deleted the entire /var/public_www directory,
    b) The vendor refused to consider the possibility that there could be something wrong with the program (and claimed that it was 'hack-proof')
    c) That PHP lets that line execute (shouldn't it throw an error or something if you try to use a variable that doesn't exist?)
    or d) That no one (at least as far I know, its possible they didn't include this in the submission) has driven down to the vendors office and beaten the crap out of him?

  • Suzy Underpants (unregistered) in reply to Henrique

    I dunno if it's different in a chroot, but I just tried it and rm happily wiped my chroot out.

  • JarFil (unregistered)

    So deliciously horrible... scary, amazing.... speechless.

  • Hikaru (unregistered)

    OMG!!!!! i am speachless..

  • (cs) in reply to VGR

    Is there any other (moderately simple, at least) way of copying a file with all its security attributes and metadata in Java?

     

  • (cs) in reply to VGR
    VGR:
    Otto:

    The real WTF here is the "exec (rm..." in the first place. Use the normal file deletion functions, you shouldn't go to the shell for trivial shit like this. Going to the shell is a really easy way to accidentally introduce security risks. There should be an IQ test before allowing programmers to use functions like exec.

    I have seen this in actual production Java code:

    public void copyFile(String filename1, String filename2)
    {
        try {
            if (System.getProperty("os.name").indexOf("Windows") >= 0)
                Runtime.getRuntime().exec("copy " + filename1 + " " + filename2);
            else
                Runtime.getRuntime().exec("/bin/cp " + filename1 + " " + filename2);
        } catch (IOException e) { }
    }


    Talk about write-once run anywhere!

    Wait, it doesn't work on Mac OS9?
  • Cheong (unregistered) in reply to tster

    Regarding race-condition, I don't think it'll really happen.

    I'm not a PHP programmer, but in general the session-expire time would be renewed whenever the client post something to the server, wouldn't it?

  • monster (unregistered)

    IMHO the true WTF here is the 'exec'. Why would I ever want to 'exec' something that can be done in like 10 lines or pure code? Duh!

    captcha = awesomeness - what a stupid word

  • Your Name (unregistered) in reply to Cheong

    This is, like, the WTF of the month at least.

  • (cs) in reply to DrPizza
    DrPizza:

    Is there any other (moderately simple, at least) way of copying a file with all its security attributes and metadata in Java?

     



    Runtime.getRuntime().exec("/bin/cp " + filename1 + " " + filename2);
    wouldn't copy metadata or security attributes as cp is defined in the Single UNIX Specification.  You need the -p switch for that.
  • Andrey (unregistered) in reply to snoofle
    snoofle:

    Satanicpuppy:
    Anyone who is dumb enough to call "rm -r" without checking to make sure they're in the right spot is a complete idiot. You're just begging for this sort of trouble.

    Even php, which is sloppy about file and directory hooks, still has enough methods that you can use...nice safe code methods, without having to resort to exec(), which has vast potential for these sorts of problems.

    Also I wonder at the set file permissions...what user is php running as, and why does it have permission to delete the whole application? All you need is read and execute, except in whatever directory you're storing your session information in, and if they'd done that, the worst thign that could have happened is that when the code borked, it would have knocked out everyones sessions and not the whole application.

    As above, I'm not a web person, but (sadly) most of the applications that I've inherited in the Unix world ran as root whether there was a good reason or not. If I tried to explain that this wasn't prudent, I'd invariably get back "it's working, leave it alone". Seems like the folks who design these things up front are too lazy to deal with privilege issues and setting up non-root users.

    edit: imagine if they'd done something like this:

    function terminate() {
  exec("rm -r $_SESSION['VarPublicWwwDir']/$_SESSION['Tempfile']");
}


    The result would depend on the OS and the particular version of rm being used.  Some versions of rm will just refuse to do a recursive delete on root.  Others will all too happily trust the judgement of the user.
  • (cs) in reply to mrprogguy
    mrprogguy:

    I hate to be pedantic here, but...

    John Connor: Did you terminate all those files?
    Terminator: Esta la Vista, baby.

    "Esta la Vista" doesn't really mean anything, unless, badly translated, you interpret it to mean "this is the view."

    You were thinking "Hasta la vista," which is "goodbye," or "until I see you" (or you see me, or we see each other--it's a colloquialism that doesn't really have a literal translation).

    Actually, I don't hate to be pedantic.


    In spanish, H's are usually muted (have style="display: none;" set on them), hence "Hasta" is often heard as "'asta" (or in the case of a really bad accent, "'esta").
    I was always under the impression (thanks to my secondary school spanish teacher) that "Hasta la vista" meant "I'll see you when I see you", and that you should really use "Hasta Luego" or "Hasta Mañana", meaning "I'll see you later" and "I'll see you tomorrow" respectively. Though Mañana is a slippery one that can mean (in it's colloquial terminology) "Tomorrow, or sometime after that".

    And I've got my +5 Mantle of Pedantry on. :)
  • Dan (unregistered) in reply to Cheong

    From time to time, PHP will check what sessions should expire, delete their tempfiles and clear their variables.  So when the user comes back half an hour later PHP opens a new empty session which you are right will not time out.  So checking out if what you thought you put in $_SESSION is still there, you'll know if the session expired or not.

    As people pointed before, you can save pretty much anything you want in your session and PHP takes care of the cleaning.

  • Esben (unregistered) in reply to Boner

       In which setup does "$_SESSION[Tempfile]" throw a notice?

    It is perfectly legal php code. Since the key Tempfile is already qouted once(In the outer string) it doesn't need to be qouted again.

    And to the guy that suggested that

    if(exists($_SESSION['Tempfile'])) /Use $_SESSION['Tempfile'] here/

    should be a race condition is wrong. $_SESSION[] is copied into the program so even if another thread or process should get cpu time the data will still be there when control returns.


  • (cs) in reply to qbolec
    qbolec:

    Also, as mentioned earlier, most often the script fully controls the content of the session variables, so it is not-so-insecure to use them inside exec(), but why risk?

    Ideally that would be true, but beware of injection bugs.  As you say, it's best not to risk it.

  • [ICR] (unregistered) in reply to tster

    "That would introduce a race condition.  if it tested that at 24 minutes 59.99 seconds then the session expired and interupt was sent and session became undefined the same thing is going to happen.  granted there is a very small chance for it to happen.  but remember, if something can go wrong, it will."

    As already mentioned, because of the way PHP handles it there wouldn't be a race condition. However, it's also very easy even if it didn't handle them like that to avoid the race condition:
    <font face="Lucida Console" size="2">$tempFile = $_SESSION['Tempfile']);
    <font color="#000000">if (!empty($tempFile))
       </font></font><font color="#000000">exec(</font><font color="#000000">"rm -r /var/public_www/</font><font color="#000000" face="Lucida Console" size="2">$tempFile</font><font color="#000000">");
    Which effectivly mimicks what PHP already does :)
    </font>

  • (cs) in reply to [ICR]

    No, PHP will not treat undefined variables as NULL. They're just undefined. Infact if you try to get the value of an undefined variable a properly configured PHP installation will issue a Notice. I'll just assume that this installation wasn't properly configured.

  • Matt (unregistered) in reply to Lumpio-

    Instead of checking if the session is expired, why not use:
    If (strlen($_SESSION['tempfile']) > 0) ...


    because you never know when some jackass will store  "" or worse in $_SESSION['tempfile'].

  • nikolas (unregistered) in reply to Lumpio-
    Lumpio-:
    No, PHP will not treat undefined variables as NULL. They're just undefined. Infact if you try to get the value of an undefined variable a properly configured PHP installation will issue a Notice. I'll just assume that this installation wasn't properly configured.


    a _real_ php-coder turns all that annoying notices and warnings off. in the end that's the reason he chose to use php: don't care for error - add weird code until it seems to do what it should...

    p.s.: my CAPTCHA test string for this post was: billgates
  • belef (unregistered) in reply to rob_squared
    rob_squared:
    See, this is an enabling technology for that on-the-go hacker who doesn't have time to guess passwords or scan for vulnerabilities.

    Of course the reall WTF is the "in the family" mentality.  Its happened to me.  At a job I worked they made a custodian an assistant system tech because he was a friend of someone in management.
    ----------
    "There is no such thing as good luck. There is only misfortune and its occasional absence."




    There is no such thing as good luck. There is only misfortune and its occasional nisfortune.
  • (cs) in reply to merreborn
    merreborn:
    As stupid as this function and directory layout are to begin with, the addition of one line probably would have completely prevented this.


    function terminate() {
      if(!empty($_SESSION['Tempfile'])) 
        exec("rm -r /var/public_www/$_SESSION['Tempfile']");
    }


    So what happens when when a black hat submits a query with the session variable Tempfile set to '*'?

    You need a lot more checks than just empty().

  • Esben (unregistered) in reply to [ICR]
    Anonymous:
    "That would introduce a race condition.  if it tested that at 24 minutes 59.99 seconds then the session expired and interupt was sent and session became undefined the same thing is going to happen.  granted there is a very small chance for it to happen.  but remember, if something can go wrong, it will."

    As already mentioned, because of the way PHP handles it there wouldn't be a race condition. However, it's also very easy even if it didn't handle them like that to avoid the race condition:
    <font face="Lucida Console" size="2">$tempFile = $_SESSION['Tempfile']);
    <font color="#000000">if (!empty($tempFile))
       </font></font><font color="#000000">exec(</font><font color="#000000">"rm -r /var/public_www/</font><font color="#000000" face="Lucida Console" size="2">$tempFile</font><font color="#000000">");
    Which effectivly mimicks what PHP already does :)
    </font>


    In order to have a race condition you must have shared memory. $_SESSION is not shared. It is copied into the process before any PHP code is executed. So here a race condition is impossible.
  • arashi (unregistered) in reply to Boner
    Anonymous:
    merreborn:


    "$_SESSION['Tempfile']" ALWAYS evaluates to "", while "$_SESSION[Tempfile]" can actually return data -- IIRC, PHP doesn't like it when you single quote array indices in a double quoted string.


    Correct:
    "$_SESSION['Tempfile']" # throws an error
    "$_SESSION[Tempfile]" # throws a notice
    "{$_SESSION['Tempfile']}" # correct

    This wouldn't actually work at all:
    exec("rm -r /var/public_www/$_SESSION['Tempfile']");

    This would be correct:
    exec("rm -r /var/public_www/{$_SESSION['Tempfile']}");


    or simply

    exec("rm -r /var/public_www/".$_SESSION['Tempfile'])
  • (cs) in reply to EvanED
    EvanED:
    merreborn:
    As stupid as this function and directory layout are to begin with, the addition of one line probably would have completely prevented this.


    function terminate() {
      if(!empty($_SESSION['Tempfile'])) 
        exec("rm -r /var/public_www/$_SESSION['Tempfile']");
    }


    So what happens when when a black hat submits a query with the session variable Tempfile set to '*'?

    You need a lot more checks than just empty().



    Well, I'm nitpicking here, but the data inside the session is controlled by the script, and not by the end user. The user will have a cookie with a session ID, which may be stored in the cookie. Modifying that value however will just make it invalid, and not alter any of the data associated with the session.

    The tempfile would hopefully be set with
    $_SESSION['tempfile'] = mktmp();

    In order for the black-hat to modify it, it would have to be actually provided by the browser, i.e.
    $_SESSION['tempfile'] = $_POST['tmpfile'];
    and a <input type="hidden" name="tmpfile" value="blahblah231432.tmp">

    which is unlikely.. (but I would not be surprised if they really did that.)

  • cm (unregistered) in reply to Nandurius

    Well, I'm nitpicking here, but the data inside the session is controlled by the script, and not by the end user.

    That's not nitpicking, it's simply the correct answer.

  • (cs) in reply to Volmarias
    Volmarias:
    Whoops. Really stupid to not have actually checked for this, but I wouldn't call it an absolute WTF.

    You're joking, right? Even by dodgy php code standards, this is very, very bad.

  • (cs) in reply to snoofle

    Actually, it seems odd to have the web directory write-able by the server, anyway.

  • (cs) in reply to powerlord
    powerlord:
    DrPizza:

    Is there any other (moderately simple, at least) way of copying a file with all its security attributes and metadata in Java?

     



    Runtime.getRuntime().exec("/bin/cp " + filename1 + " " + filename2);
    wouldn't copy metadata or security attributes as cp is defined in the Single UNIX Specification.  You need the -p switch for that.

    None of it?  Oh well.  In Windows you'd need to "copy" to for example easily copy over multiple file streams (in which some metadata resides).  Though actually to copy things like timestamps and permissions does require something other than plain "copy".

     

  • ShareaWeb (unregistered)

    The real WTF here is someone paid $5K and never reviewed the code they bought. How many of you would gladly accept $500 to rifle through some php code looking for sick crap like that.

    The secondary 'holy crap' is that their php.ini is surely a WTF case as well, for exec() access on a production server is a freakin' no-no to begin with (Ever hear of Cron Jobs or Scheduled Tasks??)

    Third, rudimentary php is that any superglobals (Session/Cookie/Posts/Gets) should always be checked for definition ("undefined index") before calls to them.

    Lastly, this one made me really chuckle:

    This would have been solved if you just called it:
    <font color="#000099">function</font> terminate(DO_NOT_PUT_ANYTHING_IN_HERE_EVER)
    								</div>
								</div>

    P.S. I proved I was human by typing 'batman' in the captcha. :)

  • Mark (unregistered)
    General Announcement: If you used the contact form to submit anything to me in the past week or so, I never received it due to spam filters. Feel free to resend.

    WTF!!

    :-)
  • (cs)

    <sarcasm> Nothing wrong here, the terminate() function acts as intended: it totally [ex]terminates the application. </sarcam>

  • Loren Pechtel (unregistered) in reply to WIldpeaks

    Note that there is probably another bug lurking here.

    The session variable was blank because it had been timeout terminated--but was the temp stuff cleaned out when this happened?  I suspect this code builds up garbage until you get a disk full.  It's probably slow enough that nobody would have noticed it, though.

Leave a comment on “The Terminate()or”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article