DragonPrime - LoGD Resource Community

Core Code Development Discussions => RAID! (Core Code Bug Reports and Patches) => Topic started by: Nightborn on April 26, 2019, 10:10:10 AM



Title: SQL injection bug in motd.php
Post by: Nightborn on April 26, 2019, 10:10:10 AM
I got a rather interesting sql injection attack on my server, which is (as I just downloaded the bugfixed 2017 version) also present in lotgd core.

motd.php has the following code:

Code:
$m = httpget("month");
if ($m > ""){
$sql = "SELECT " . db_prefix("motd") . ".*,name AS motdauthorname FROM " . db_prefix("motd") . " LEFT JOIN " . db_prefix("accounts") . " ON " . db_prefix("accounts") . ".acctid = " . db_prefix("motd") . ".motdauthor WHERE motddate >= '{$m}-01' AND motddate <= '{$m}-31' ORDER BY motddate DESC";
$result = db_query_cached($sql,"motd-$m");
}else{
$sql = "SELECT " . db_prefix("motd") . ".*,name AS motdauthorname FROM " . db_prefix("motd") . " LEFT JOIN " . db_prefix("accounts") . " ON " . db_prefix("accounts") . ".acctid = " . db_prefix("motd") . ".motdauthor ORDER BY motddate DESC limit $newcount,".($newcount+$count);
if ($newcount=0) //cache only the last x items
$result = db_query_cached($sql,"motd");
else
$result = db_query($sql);
}

As you can see, $m is entered by httpget() and patched without escape right into the query.

Which leads to somebody actively trying to get the db connection (and therefore access to user data) producing this error:

Code:
fopen(/var/datacache/datacache_motd-cast2828SELECT20dblink-connect28chr28104297C7Cchr28111297C7Cchr28115297C7Cchr28116297C7Cchr2861297C7Cchr2854297C7Cchr28121297C7Cchr28112297C7Cchr28116297C7Cchr28115297C7Cchr2851297C7Cchr28107297C7Cchr28100297C7Cchr28115297C7Cchr28110297C7Cchr28110297C7Cchr2849297C7Cchr28108297C7Cchr28106297C7Cchr28100297C7Cchr28114297C7Cchr2899297C7Cchr28120297C7Cchr28120297C7Cchr28107297C7Cchr2899297C7Cchr28115297C7Cchr28117297C7Cchr28106297C7Cchr28101297C7Cchr2897297C7Cchr28103297C7Cchr28105297C7Cchr2898297C7Cchr28102297C7Cchr2897297C7Cchr28111297C7Cchr2853297C7Cchr28118297C7Cchr28102297C7Cchr2851297C7Cchr28109297C7Cchr28114297C7Cchr28107297C7Cchr28101297C7Cchr28106297C7Cchr2856297C7Cchr28107297C7Cchr2846297C7Cchr28114297C7Cchr2856297C7Cchr2855297C7Cchr2846297C7Cchr28109297C7Cchr28101297C7Cchr2832297C7Cchr28117297C7Cchr28115297C7Cchr28101297C7Cchr28114297C7Cchr2861297C7Cchr2897297C7Cchr2832297C7Cchr28112297C7Cchr2897297C7Cchr28115297C7Cchr2 8115297C7Cchr28119297C7Cchr28111297C7Cchr28114297C7Cchr28100297C7Cchr2861297C7Cchr2897297C7Cchr2832297C7Cchr2899297C7Cchr28111297C7Cchr28110297C7Cchr28110297C7Cchr28101297C7Cchr2899297C7Cchr28116297C7Cchr2895297C7Cchr28116297C7Cchr28105297C7Cchr28109297C7Cchr28101297C7Cchr28111297C7Cchr28117297C7Cchr28116297C7Cchr2861297C7Cchr285029292920as20numeric29): failed to open stream: File name too long in /var/www/html/naruto/lib/datacache.php (62)
Call Stack:
2: fopen("/var/datacache/...", "w") called from /var/lib/datacache.php on line 62
3: updatedatacache("motd-cast((SELECT dblink_...", Array()) called from /var/lib/dbwrapper_mysqli_proc.php on line 65
4: db_query_cached("SELECT motd.*,name AS mot...", "motd-cast((SELECT dblink_...") called from /var/motd.php on line 64

I used (I think this should work on your systems too) following git patch:

Code:
diff --git a/motd.php b/motd.php
index 48b2b6d..a034a2a 100644
--- a/motd.php
+++ b/motd.php
@@ -58,10 +58,16 @@ if ($op=="") {
        /*
        motditem("Beta!","Please see the beta message below.","","", "");
        */
-       $m = httpget("month");
-       if ($m > ""){
-               $sql = "SELECT " . db_prefix("motd") . ".*,name AS motdauthorname FROM " . db_prefix("motd") . " LEFT JOIN " . db_prefix("accounts") . " ON " . db_prefix("accounts") . ".acctid = " . db_prefix("motd") . ".motdauthor WHERE motddate >= '{$m}-01' AND motddate <= '{$m}-31' ORDER BY motddate DESC";
-               $result = db_query_cached($sql,"motd-$m");
+       $month_post = httpget("month");
+       //SQL Injection attack possible -> kill it off after 7 letters as format is i.e. "2000-05"
+       $month_post = substr($month_post,0,7);
+       if (preg_match("/[0-9][0-9][0-9][0-9]-[0-9][0-9]/",$month_post)!==1) {
+               //hack attack
+               $month_post="";
+       }
+       if ($month_post > ""){
+               $sql = "SELECT " . db_prefix("motd") . ".*,name AS motdauthorname FROM " . db_prefix("motd") . " LEFT JOIN " . db_prefix("accounts") . " ON " . db_prefix("accounts") . ".acctid = " . db_prefix("motd") . ".motdauthor WHERE motddate >= '{$month_post}-01' AND motddate <= '{$month_post}-31' ORDER BY motddate DESC";
+               $result = db_query_cached($sql,"motd-$month_post");
        }else{
                $sql = "SELECT " . db_prefix("motd") . ".*,name AS motdauthorname FROM " . db_prefix("motd") . " LEFT JOIN " . db_prefix("accounts") . " ON " . db_prefix("accounts") . ".acctid = " . db_prefix("motd") . ".motdauthor ORDER BY motddate DESC limit $newcount,".($newcount+$count);
                if ($newcount=0) //cache only the last x items
@@ -97,7 +103,7 @@ if ($op=="") {
        while ($row = db_fetch_assoc($result)){
                $time = strtotime("{$row['d']}-01");
                $m = translate_inline(date("M",$time));
-               rawoutput ("<option value='{$row['d']}'".(httpget("month")==$row['d']?" selected":"").">$m".date(", Y",$time)." ({$row['c']})</option>");
+               rawoutput ("<option value='{$row['d']}'".($month_post==$row['d']?" selected":"").">$m".date(", Y",$time)." ({$row['c']})</option>");
        }
        rawoutput("</select>".tlbutton_clear());
        $showmore=translate_inline("Show more");



Title: Re: SQL injection bug in motd.php
Post by: Eliwood on April 27, 2019, 04:50:36 AM
Oh wow, thats bad. Another reminder why prepared queries should be used.

Regarsing the patch:

Wouldnt it be easier to substr month and year and convert them to integers instead of matching a regexp?


Title: Re: SQL injection bug in motd.php
Post by: Nightborn on April 27, 2019, 11:13:00 AM
It's pretty much the same, you'd need to rewrite the queries afterwards then or you have to concat the month+year together again.
Or you check if both are originally integer values, which would be more lines of code.

Pretty much the same I think, regex might be a bit quicker.

Any fix is good as long as it is fixed.
motd.php is accessible from outside the game, making it worse.


Title: Re: SQL injection bug in motd.php
Post by: Sunday on April 28, 2019, 07:35:08 AM
Would the regex \d{4}-\d{2} work as well? The fix is fine but it seems lengthy.


Title: Re: SQL injection bug in motd.php
Post by: Nightborn on April 28, 2019, 02:22:44 PM
Yeah, sure, that's shorter and better =)

© 2019 DragonPrime - LoGD Resource Community
Email Talisman: talisman -at- gmail.com
&oeXs)2U7=V BmܲV.U e=;p\}eG )Jj} C5EH7ˤH=j } mo|*Ŋw{drV_@IV>/- TFQJ׀̀Ve}l1,V O iNYx͘$e$;
Forums: Powered by SMF 1.1.21 | SMF © 2006-2007, Simple Machines