1.0.2 Important security patches

Discussion in 'General Support' started by renlok, Jun 22, 2011.

  1. renlok

    renlok Administrator Staff Member

    Joined:
    Oct 20, 2008
    Messages:
    2,858
    Likes Received:
    330
    in includes/functions_global.php
    replace
    PHP:
    return $str;
    with
    PHP:
    return floatval($str);
    before
    PHP:
    ?>
    add
    PHP:
    // strip none alpha-numeric characters
    function strip_non_an_chars($str)
    {
        
    $str preg_replace("/[^a-zA-Z0-9\s]/"''$str);
        return 
    $str;
    }
    in includes/messages.inc.php
    replace
    PHP:
    $language $_GET['lan'];
    with
    PHP:
    $language preg_replace("/[^a-zA-Z\s]/"''$_GET['lan']);
    replace
    PHP:
    $language $_COOKIE['USERLANGUAGE'];
    with
    PHP:
    $language preg_replace("/[^a-zA-Z\s]/"''$_COOKIE['USERLANGUAGE']);
    in user_login.php replace
    PHP:
    $query "DELETE from " $DBPrefix "online WHERE SESSION = '" $_COOKIE['WEBID_ONLINE'] . "'";
    with
    PHP:
    $query "DELETE from " $DBPrefix "online WHERE SESSION = '" strip_non_an_chars($_COOKIE['WEBID_ONLINE']) . "'";
    in logout.php
    replace
    PHP:
    $query "DELETE FROM " $DBPrefix "rememberme WHERE hashkey = '" $_COOKIE['WEBID_RM_ID'] . "'";
    with
    PHP:
    $query "DELETE FROM " $DBPrefix "rememberme WHERE hashkey = '" strip_non_an_chars($_COOKIE['WEBID_RM_ID']) . "'";
    in includes/converter.inc.php replace
    PHP:
    $output .= "\t" "array('from' => '" $newaarray[$i]['from'] . "', 'to' => '" $newaarray[$i]['to'] . "', 'rate' => '" $newaarray[$i]['rate'] . "')";
    with
    PHP:
    $output .= "\t" "array('from' => '" floatval($newaarray[$i]['from']) . "', 'to' => '" floatval($newaarray[$i]['to']) . "', 'rate' => '" floatval($newaarray[$i]['rate']) . "')";
    and heres edits to feedback.php to make it more secure http://simpleauction.svn.sourceforge.net/viewvc/simpleauction/WeBid/trunk/feedback.php?r1=289&r2=297

    Also many thanks to EgiX for pointing these issues out :)
     
    Last edited: Jun 23, 2011
    infin8 and Lunkwill like this.
  2. Box Lot

    Box Lot Super Moderator Staff Member Developer

    Joined:
    Dec 18, 2008
    Messages:
    2,617
    Likes Received:
    164
    I don't see a change to includes/messages.inc.php after r.293 (which I'm still on) and line 29 does read
    Possibly covering 1.0.1?

    Thanks EgiX :)
    Thanks Renlok! :):)
     
    Last edited: Jun 23, 2011
  3. EgiX

    EgiX New Member

    Joined:
    Jun 22, 2011
    Messages:
    3
    Likes Received:
    1
    You've forgotten to validate also $_COOKIE['USERLANGUAGE'], you should add this line before calling include() at line 46:

    PHP:
    $language preg_replace("/[^a-zA-Z\s]/"''$language);
     
  4. infin8

    infin8 New Member

    Joined:
    Aug 4, 2010
    Messages:
    2
    Likes Received:
    0
    Thanks renlok for the patches, greatly
    appreciated!
     
    Last edited: Jun 23, 2011
  5. Box Lot

    Box Lot Super Moderator Staff Member Developer

    Joined:
    Dec 18, 2008
    Messages:
    2,617
    Likes Received:
    164
    Huh, what?

    Lol, can we (Renlok) redo the original includes/messages.inc.php section to reflect what it should look like.

    Couldn't tell if you were responding to me or something Renlok missed. Either way best to have the original posting right.

    EDIT: Regarding my question, I see from looking at the SVN, the replacement is reversed as written?
     
    Last edited: Jun 23, 2011
  6. EgiX

    EgiX New Member

    Joined:
    Jun 22, 2011
    Messages:
    3
    Likes Received:
    1
    1.0.2 Important security patches

    PHP:
    I was responding to a renlok missing, this is the actual code of /includes/messages.inc.php:

    PHP:
    17    // Language management
    18    if (isset($_GET['lan']) && !empty($_GET['lan']))
    19    {
    20            if ($user->logged_in)
    21            {
    22                    $query "UPDATE " $DBPrefix "users SET language = '" mysql_real_escape_string($_GET['lan']) . "' WHERE id = " $user->user_data['id'];
    23            }
    24            else
    25            {
    26                    // Set language cookie
    27                    setcookie('USERLANGUAGE'$_GET['lan'], time() + 31536000'/');
    28            }
    29            $language preg_replace("/[^a-zA-Z\s]/"''$_GET['lan']);
    30    }
    31    elseif ($user->logged_in)
    32    {
    33            $language $user->user_data['language'];
    34    }
    35    elseif (isset($_COOKIE['USERLANGUAGE']))
    36    {
    37            $language $_COOKIE['USERLANGUAGE'];
    38    }
    39    else
    40    {
    41            $language $system->SETTINGS['defaultlanguage'];
    42    }
    43    
    44    
    if (!isset($language) || empty($language)) $language $system->SETTINGS['defaultlanguage'];
    45    
    46    
    include $main_path 'language/' $language '/messages.inc.php';
    Only input passed through $_GET['lan'] is sanitised, but $_COOKIE['USERLANGUAGE'] is still vulnerable to a local file inclusion. So you should add this line before line 46:

    PHP:
    $language preg_replace("/[^a-zA-Z\s]/"''$language);
    and replace line 29 with this:

    PHP:
    $language $_GET['lan'];
    Finally, you should to patch also this two 0day vulnerabilities:

    http://www.allinfosec.com/2011/06/17/webapps-0day-webid-1-0-2-presistent-xss-via-sql-injection/
    http://www.allinfosec.com/2011/06/19/webapps-0day-webid-v1-0-2-multiple-remote-csrf-vulnerabilities/
     
    Last edited: Jun 23, 2011
  7. Box Lot

    Box Lot Super Moderator Staff Member Developer

    Joined:
    Dec 18, 2008
    Messages:
    2,617
    Likes Received:
    164
    Thanks. I get it but frankly a lot of people are now going to be confused, original posting should be corrected. Level of technical expertise is not balanced here and tends towards the lower level in general. Admittedly when it comes to the security aspect I'm at the lower level myself and don't "see" the weaknesses. :eek:

    That noted site is F'd up! Less then one day notice and they make it public! Is it supposed to be a helpful site or malicious one?

    Anyway, do you have the fixes for those to provide to Renlok?

    Have to say you showed up HARD to the site! Thanks.
     
    Last edited: Jun 23, 2011
  8. jdcanada

    jdcanada New Member

    Joined:
    Jun 18, 2011
    Messages:
    12
    Likes Received:
    1
    a real newbie here ... less than a week old ...

    followed all those steps in the first post that i could ... then followed the feedback additional link ...

    was able to download that file ... ( all good practise ! )

    but then also found that SVN 297 is available and I could have downloaded the files rather than having to have spent the time doing the individual line changes myself.

    ... again, all good practise for me in getting used to these file updates


    Is there more to come based on Egix's last post ?
     
    1 person likes this.
  9. Lunkwill

    Lunkwill New Member

    Joined:
    Nov 14, 2010
    Messages:
    65
    Likes Received:
    5
    Thanks EgiX and renlok!

    This query on line 72 in feedback.php smells bad too:
    PHP:
    $sql "UPDATE " $DBPrefix "users SET rate_sum = rate_sum + " $_POST['TPL_rate'] . ", rate_num = rate_num + 1 WHERE id = " intval($uid);
    Renlok, is there any reason why you're not using the recommended (as by some PHP reference at least) sprintf method? That would guard against SQL injections at least in the case of numeric arguments, plus the queries are much easier to read. This is how I replaced the whole assign-query/execute/check_mysql dance:
    PHP:
    $system->run_mysql(sprintf(
        
    "UPDATE ${DBPrefix}users SET rate_sum = rate_sum + %d, rate_num = rate_num + 1 WHERE id = %d"
        
    intval($_POST['TPL_rate']), intval($uid)),
    __LINE____FILE__);
    $system->run_mysql is:
    PHP:
    function run_mysql($query$line$file) {
        
    $res mysql_query($query);     
        
    $this->check_mysql($res$query$line$file);
        return 
    $res;
    }
     
  10. Lunkwill

    Lunkwill New Member

    Joined:
    Nov 14, 2010
    Messages:
    65
    Likes Received:
    5
    Regarding the two "0days", has anyone gotten them to work? All I get is a login page and unless I've overlooked something in the code there's no way around that.

    Edit: got it now, it's a CSRF, I overlooked that among all the ASCII art and only got it upon reading EgiX's last post. Nasty indeed but not that hard to fix IMHO.
     
    Last edited: Jun 23, 2011
  11. EgiX

    EgiX New Member

    Joined:
    Jun 22, 2011
    Messages:
    3
    Likes Received:
    1
    I think that the patch to /includes/functions_global.php given by renlok should works to prevent the SQL injection bug in adsearch.php.
    For the CSRF vulnerability patching code is a little more complex, I recommend using one of these methods:

    https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet

    Furthermore, to prevent information leakage, you should be add an .htaccess file with "deny from all" rule into /logs directory, cause anyone can read cron.log and error.log
     
    Last edited: Jun 23, 2011
    Lunkwill likes this.
  12. Roscko Coblat

    Roscko Coblat New Member

    Joined:
    May 13, 2011
    Messages:
    8
    Likes Received:
    1
    Thanks For The Updates

    This just proves how important it is that people should visit here often and be a part of the community. Two Thumbs UP for the updates, Thank you very much :)
     
  13. renlok

    renlok Administrator Staff Member

    Joined:
    Oct 20, 2008
    Messages:
    2,858
    Likes Received:
    330
    I'm not sure about the CSRF issue I dont understand how they get passed the logged in check but i dont know that much about php security maybe its worth creating a session token with something like
    PHP:
    $token md5(uniqid(rand(), true));
    $_SESSION['token'] = $token;
    and checking it on submission good plan no?
     
  14. Lunkwill

    Lunkwill New Member

    Joined:
    Nov 14, 2010
    Messages:
    65
    Likes Received:
    5
    The CSRF thing works like this: someone who knows you're a user (or better an admin) on a WeBid site puts a form on their site and makes it look like a feedback form or whatever that you'd want to use (if($user=='renlok') print("Mate, your password has expired, please choose a new one"), something along these lines) but actually posts to your site using a bunch of hidden parameters. Because WeBid doesn't check where the request actually came from, it will happily perform whatever the POST asks it to, like creating a new admin, changing your password etc. Using JavaScript they can actually make your browser post to your WeBid site whenever you click anything on their site. For that to work, you have to be authenticated on your site, but as many admins spend most of their day logged in to their sites, it's actually pretty dangerous. Not as bad as a classic remote exploit because the attacker has to control a site you visit while logged on to yours but still.

    Fortunately, security is pretty much the only thing I know about PHP (I learned it when working at an ISP and occasionally auditing other people's scripts to analyze hacking incidents) so I quickly hacked something that I hope can be made usable without to much pain :)

    Implementing the token as you started it is indeed what we should do. The major part is checking it on submission though:
    - Generate the token on login and store it in the session
    - Add a hidden field to every form that gets posted back on submission
    - If the session contains a token (i.e. a user is authenticated), check that every POST contains the same token

    To get the token into every form, I hacked assign_vars() in includes/template.php so it adds a "csrftoken" parameter if a correponding key is foundin $_SESSION. The user class got an is_logged_in() accessor and I replaced all accesses to the public logged_in attribute with it. The accessor checks whether POST or GET parameters other than the token are present and if so, verifies the token. If the verification fails, I output a simple error message and quit; this could be done better (invalidate the session etc.) but works for now.

    There are three files contained in the fix archive:
    - csrf-fix-patch.txt is a diff to be applied with "patch" that contains the new hand-written code
    - csrf-fix-patch2.txt is the same for the autogenerated changes - separate in case someone wants to tweak the changes
    - csrf-fix-patchscript is a Bash/Perl script that does all the automatic editing, i.e. adding the hidden token field to all templates and rewriting accesses to $user->logged_in as $user->is_logged_in(). You only need this if you want to fix stuff in patch2.

    Would be cool if you guys could try it out.
     
  15. Lunkwill

    Lunkwill New Member

    Joined:
    Nov 14, 2010
    Messages:
    65
    Likes Received:
    5
    Well, early in the morning as it was I of course fuct up some basic things and tested only the cases that were supposed to be blocked (i.e. exploits) but not what was supposed to continue working :rolleyes: As it is, the patch doesn't rewrite any links containing GET parameters so it breaks much of the navigation. As it turns out, doing that is the nastiest part as they're pieced together in lots of places including the messages file, using at least three different methods---generating template code, printf-style arguments and direct concatenation.
    I should have something working by tomorrow night though.
     
  16. renlok

    renlok Administrator Staff Member

    Joined:
    Oct 20, 2008
    Messages:
    2,858
    Likes Received:
    330
    it may be worth just not check the GET variables as no valuable information gets send via it just page info and the simple search uses it but that about it
     
  17. Lunkwill

    Lunkwill New Member

    Joined:
    Nov 14, 2010
    Messages:
    65
    Likes Received:
    5
    Hum ... yeah, makes sense, if you're sure. I mean, there are loads of links on the admin pages like https://site.foo/admin/deleteauction.php?id=ID&offset=BAR but looking around for a while I haven't found any that wouldn't require confirmation in form of a POST so that should be fine. That shortens the patch a bit and makes it a lot less ugly :)
     
  18. Lunkwill

    Lunkwill New Member

    Joined:
    Nov 14, 2010
    Messages:
    65
    Likes Received:
    5
    OK, so I made the modifications again with a clean r297 checkout as opposed to my own hacked-up version and diffed. This should apply fine now and not check any GET requests.
     
  19. luke

    luke New Member

    Joined:
    May 6, 2011
    Messages:
    24
    Likes Received:
    1
    Dear Renlok,

    I'm not that good in english so i don't understand parts of your discussion over here. Can I just put in your code?

    Thanks!
     
  20. renlok

    renlok Administrator Staff Member

    Joined:
    Oct 20, 2008
    Messages:
    2,858
    Likes Received:
    330
    yes you can
     

Share This Page