Drupal



  • Security risk: 25/25 ( Highly Critical) AC:None/A:None/CI:All/II:All/E:Exploit/TD:All

    Description
    This Public Service Announcement is a follow up to SA-CORE-2014-005 - Drupal core - SQL injection. This is not an announcement of a new vulnerability in Drupal.

    Automated attacks began compromising Drupal 7 websites that were not patched or updated to Drupal 7.32 within hours of the announcement of SA-CORE-2014-005 - Drupal core - SQL injection. You should proceed under the assumption that every Drupal 7 website was compromised unless updated or patched before Oct 15th, 11pm UTC, that is 7 hours after the announcement.

    Simply updating to Drupal 7.32 will not remove backdoors.

    If you have not updated or applied this patch, do so immediately, then continue reading this announcement; updating to version 7.32 or applying the patch fixes the vulnerability but does not fix an already compromised website. If you find that your site is already patched but you didn’t do it, that can be a symptom that the site was compromised - some attacks have applied the patch as a way to guarantee they are the only attacker in control of the site.

    Data and damage control

    Attackers may have copied all data out of your site and could use it maliciously. There may be no trace of the attack.

    Take a look at our help documentation, ”Your Drupal site got hacked, now what”

    Recovery

    Attackers may have created access points for themselves (sometimes called “backdoors”) in the database, code, files directory and other locations. Attackers could compromise other services on the server or escalate their access.

    Removing a compromised website’s backdoors is difficult because it is not possible to be certain all backdoors have been found.

    The Drupal security team recommends that you consult with your hosting provider. If they did not patch Drupal for you or otherwise block the SQL injection attacks within hours of the announcement of Oct 15th, 4pm UTC, restore your website to a backup from before 15 October 2014:

    Take the website offline by replacing it with a static HTML page
    Notify the server’s administrator emphasizing that other sites or applications hosted on the same server might have been compromised via a backdoor installed by the initial attack
    Consider obtaining a new server, or otherwise remove all the website’s files and database from the server. (Keep a copy safe for later analysis.)
    Restore the website (Drupal files, uploaded files and database) from backups from before 15 October 2014
    Update or patch the restored Drupal core code
    Put the restored and patched/updated website back online
    Manually redo any desired changes made to the website since the date of the restored backup
    Audit anything merged from the compromised website, such as custom code, configuration, files or other artifacts, to confirm they are correct and have not been tampered with.
    While recovery without restoring from backup may be possible, this is not advised because backdoors can be extremely difficult to find. The recommendation is to restore from backup or rebuild from scratch.

    For more information, please see our FAQ on SA-CORE-2014-005.



  • @Matches said:

    Security risk: 25/25 ( Highly Critical) AC:None

    I read this as "armor class," which is interesting because it has been at least a couple of years since I last played a game in which that would be an applicable statistic.



  • I know some people who are huge Drupal advocates. There WILL be teasing involved.



  • Some more on why this a problem ...



  • Am I the only one who finds security patch diffs for PHP software incomprehensible? Most of them seem to be about adding an extra = to == or !=, making === or !==. And this one?

    
        diff --git a/includes/database/database.inc b/includes/database/database.inc
        index f78098b..01b6385 100644
        --- a/includes/database/database.inc
        +++ b/includes/database/database.inc
        @@ -736,7 +736,7 @@ abstract class DatabaseConnection extends PDO {
             // to expand it out into a comma-delimited set of placeholders.
             foreach (array_filter($args, 'is_array') as $key => $data) {
               $new_keys = array();
        -      foreach ($data as $i => $value) {
        +      foreach (array_values($data) as $i => $value) {
                 // This assumes that there are no other placeholders that use the same
                 // name.  For example, if the array placeholder is defined as :example
                 // and there is already an :example_2 placeholder, this will generate
    
    

    So...

    array_values() returns all the values from the array and indexes the array numerically. 
    

    ...that totally sounds like a no-op to me. Takes an array, returns an array. But numerically indexed! (Why wouldn't an array be numerically indexed in the first place?)



  • @hhaamu said:

    totally sounds like a no-op to me. Takes an array, returns an array. But numerically indexed! (Why wouldn't an array be numerically indexed in the first place?)

    PHP arrays are actually hashmaps.



  • SQL Injection in this day and age seriously?



  • @hhaamu said:

    ...that totally sounds like a no-op to me. Takes an array, returns an array. But numerically indexed! (Why wouldn't an array be numerically indexed in the first place?)

    PHP uses the word "array" to refer to hashmaps. But it's possible to have a hashmap that's identical to an array.

    This function takes an array with no indexing (meaning, a hashmap) and converts it into a normal array with indexes. #phpexpert

    I can't for the life of me imagine how this exploit works based on that patch, though. Of course we don't see how $i is actually used.



  • @blakeyrat said:

    I can't for the life of me imagine how this exploit works based on that patch, though. Of course we don't see how $i is actually used.

      /**
       * Expands out shorthand placeholders.
       *
       * Drupal supports an alternate syntax for doing arrays of values. We
       * therefore need to expand them out into a full, executable query string.
       *
       * @param $query
       *   The query string to modify.
       * @param $args
       *   The arguments for the query.
       *
       * @return
       *   TRUE if the query was modified, FALSE otherwise.
       */
      protected function expandArguments(&$query, &$args) {
        $modified = FALSE;
    
        // If the placeholder value to insert is an array, assume that we need
        // to expand it out into a comma-delimited set of placeholders.
        foreach (array_filter($args, 'is_array') as $key => $data) {
          $new_keys = array();
          foreach ($data as $i => $value) {
            // This assumes that there are no other placeholders that use the same
            // name.  For example, if the array placeholder is defined as :example
            // and there is already an :example_2 placeholder, this will generate
            // a duplicate key.  We do not account for that as the calling code
            // is already broken if that happens.
            $new_keys[$key . '_' . $i] = $value;
          }
    
          // Update the query with the new placeholders.
          // preg_replace is necessary to ensure the replacement does not affect
          // placeholders that start with the same exact text. For example, if the
          // query contains the placeholders :foo and :foobar, and :foo has an
          // array of values, using str_replace would affect both placeholders,
          // but using the following preg_replace would only affect :foo because
          // it is followed by a non-word character.
          $query = preg_replace('#' . $key . '\b#', implode(', ', array_keys($new_keys)), $query);
    
          // Update the args array with the new placeholders.
          unset($args[$key]);
          $args += $new_keys;
    
          $modified = TRUE;
        }
    
        return $modified;
      }
    

    That's a bit of a comments overload for me. Twenty-nine lines of comments for eleven lines of code.

    Anyway, I take it that placeholders in an sql string are being replaced with "${key}_${i}", and if $i (the not-quite-numeric hash key) is naughty, it's injection time.



  • It's amazing there's no PHP bashing here like everywhere else because "PHP is bad" is a meme just like "gotos are evil" meanwhile the actual bug is no fault of PHP.Drupal should ditch their shitty database abstraction and use the prepared statements for MySQL and Postgres.



  • It's a waste of effort to preach to the choir.



  • @delfinom said:

    It's amazing there's no PHP bashing here like everywhere else

    How long have you been here?

    We kind of eased up on it a bit lately, since the forum now actually has a PHP developer on it. Not sure we ever really did before.

    @delfinom said:

    Drupal should ditch their shitty database abstraction and use the prepared statements for MySQL and Postgres.

    That is undoubtedly true ++.


  • Discourse touched me in a no-no place

    @blakeyrat said:

    since the forum now actually has a PHP developer on it. Not sure we ever really did before.

    I've used it in anger and created a site from scratch as a hobby. As a result I'm now (oh fuck) the go-to guy for the few times it rears it's head at work. (Frontend guys are, I understand, python. I'm largely backend stuff.)

    That not count?


  • Discourse touched me in a no-no place

    @blakeyrat said:

    We kind of eased up on it a bit lately, since the forum now actually has a PHP developer on it. Not sure we ever really did before.

    I think it's more that we've been busy bashing on other things recently. There's just not enough hours in the day to give everything the lambasting it merits.



  • I was referring to Arentor, but whatever.



  • @blakeyrat said:

    I was referring to @Arantor

    FTFY
    Putting credit where credit is due



  • Spelling's for sucks and neeerds.



  • Naming things and counting are three things that are hard in IT



  • You could create a similar vulnerability in any language if you are building queries like that.


  • BINNED

    @Arantor is much more open about his dabblings


  • :belt_onion:

    @blakeyrat said:

    We kind of eased up on it a bit lately, since the forum now actually has a PHP developer on it. Not sure we ever really did before.

    Well, not one crazy enough to admit it at least.

    @delfinom said:

    Drupal should ditch their shitty database abstraction and use the prepared statements for MySQL and Postgres.

    You can have both. I actually have a layer that constructs queries in a similar way, but then they get fed through PDO's prepare(). I really can't fathom why Drupal didn't do that as well.



  • @Onyx said:

    I really can't fathom why Drupal didn't do that as well.

    We could say "Because PHP" but that's not entirely fair. We could say "Because PHP history" and that might be more accurate. We could say "Because PHP culture" and then I think we're getting pretty close. I just stick with "Because PHP" because it's shorter and the extra accuracy is rarely important.


  • sockdevs

    I was quite amazed at this, at the levels of compounding required to make this fail.

    Bonus fact: only some of the issues are inherent to the language, much more of the issues are poor use of that language.

    Don't let up on bashing PHP on my account, guys. It has its issues - but of the level of failure here, PHP is not really the root cause :stuck_out_tongue:



  • @hhaamu said:

    That's a bit of a comments overload for me. Twenty-nine lines of comments for eleven lines of code.

    To be fair, those are really good comments IMO. Those are the kinds of things that (1) are not self-evident by reading the code (in in the case of the first longer comment, aren't evident at all from reading the code for just that function unless you start with the assumption that it is correct) and (2) would be potentially relevant to future maintainers.

    My one complaint is some of the first long comment could be turned into an assertion.



  • @EvanED said:

    those are really good comments IMO

    The comments are indication that the code is too good for it's own good as evidenced by the fact that it had such a subtle but dangerous bug.

    @Arantor said:

    but of the level of failure here, PHP is not really the root cause

    The weird nature of it's arrays did certainly contribute though.


  • Discourse touched me in a no-no place

    @EvanED said:

    My one complaint is some of the first long comment could be turned into an assertion.

    Trust, but verify.


    One of my favourite comments (from the Tcl sources) is this very long one:

    /*
     * I tried a zillion different hash functions and asked many other people
     * for advice. Many people had their own favorite functions, all
     * different, but no-one had much idea why they were good ones. I chose
     * the one below (multiply by 9 and add new character) because of the
     * following reasons:
     *
     * 1. Multiplying by 10 is perfect for keys that are decimal strings, and
     *    multiplying by 9 is just about as good.
     * 2. Times-9 is (shift-left-3) plus (old). This means that each
     *    character's bits hang around in the low-order bits of the hash value
     *    for ever, plus they spread fairly rapidly up to the high-order bits
     *    to fill out the hash value. This seems works well both for decimal
     *    and non-decimal strings, but isn't strong against maliciously-chosen
     *    keys.
     *
     * Note that this function is very weak against malicious strings; it's
     * very easy to generate multiple keys that have the same hashcode. On the
     * other hand, that hardly ever actually occurs and this function *is*
     * very cheap, even by comparison with industry-standard hashes like FNV.
     * If real strength of hash is required though, use a custom hash based on
     * Bob Jenkins's lookup3(), but be aware that it's significantly slower.
     * Since Tcl command and namespace names are usually reasonably-named (the
     * main use for string hashes in modern Tcl) speed is far more important
     * than strength.
     *
     * See also HashString in tclLiteral.c.
     * See also TclObjHashKey in tclObj.c.
     *
     * See [tcl-Feature Request #2958832]
     */
    

    It's that long because it represents a lot of experience, and because anyone thinking about messing with it needs to STOP and think very hard about what they're doing. (It's from code that has been accused of being a very fast hash function, and yet which looks really simple and simple-minded. Hash functions are really non-obvious, especially in terms of performance on normal data…)


  • sockdevs

    The 'weird' nature of its arrays are both a strength and weakness.

    Yes, there are both indexed and hashmap arrays but most PHP devs do not normally need to care. You can just shove any old stuff in and expect it to not blow up. This was a bit special however.



  • @EvanED said:

    those are really good comments IMO

    Those comments are really good, but not for the reason you may be thinking. They're really good because as soon as I read:

    We therefore need to expand them out into a full, executable query string.

    I knew exactly what was coming. PHP (culture) does it again.


  • sockdevs

    Yes, the culture is full of stupid brokenness, written by people who don't know enough not to do this.

    Not all of the PHP ecosystem is this bad, though. Just most of it.



  • @another_sam said:

    I just stick with "Because PHP" because it's shorter and the extra accuracy is rarely important.

    Shortened that for you.


  • sockdevs

    Someone showed me this group on Facebook. My response, and I quote literally:

    my god the stupid

    You know I tend to punctuate and stuff - even on chat and so on but DAYMN. It hurts to be associated with this level of stupid.



  • obviously, Java is TRWTF here.

    also, it's very obvious from that forum that most developers don't understand its/it's, why would they understand the nuances of a programming language?


    Filed under post 25 proves my point



  • For those who don't have a [TRADEMARKED COMMON NOUN REDACTED][TRADEMARKED COMMON NOUN REDACTED] account, what is that page?



  • International PHP Developers Group



  • Is there supposed to be something funny about that?



  • I assume the content, which doesn't help you.



  • I can't wait until they invent the technology to show text on a website without requiring authentication.



  • #Why choosing PHP? PHP compared to other programming languages.
    1 min read; updated: september 29, 2014

    Why would you choose PHP compared to other languages?

    [end of article]


  • Grade A Premium Asshole

    @ben_lubar said:

    I can't wait until they invent the technology to show text on a website without requiring authentication.

    One of these days technology will catch up with needs on this front and then there will be a utopia of free information to be had. I do not see it on the horizon though, such a thing cannot yet be made.

    Or, more correctly:

    @ben_lubar said:

    We are long since past the day when people do not try to monetize make money off of every single thing by forcing you to sign up and inflate their user count, even if you hate the platform.

    FTFY


Log in to reply
 

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