DragonPrime - LoGD Resource Community
Welcome Guest
  • Good evening, Guest.
    Please log in, or register.
  • May 25, 2019, 07:57:19 PM
Home Forums News Downloads Login Register Advanced Search
* * *
DragonPrime Menu
Login
 
 
Resource Pages
Search

Pages: [1]   Go Down
  Print  
Author Topic: SQL injection bug in motd.php  (Read 302 times)
0 Members and 1 Guest are viewing this topic.
Nightborn
Captain of the Guard
***
Offline Offline

Posts: 221


View Profile WWW
« 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");

Logged
Eliwood
Member
Codemeister
****
Offline Offline

Posts: 279


View Profile WWW
« Reply #1 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?
Logged

Daenerys LotGD-Remake using PHP7 and a modern, headless approach.
Nightborn
Captain of the Guard
***
Offline Offline

Posts: 221


View Profile WWW
« Reply #2 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.
Logged
Sunday
Codemeister
****
Offline Offline

Posts: 405


So meme'd up.


View Profile
« Reply #3 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.
Logged

Slowly progressing fork with PHP 7 support: https://github.com/stephenKise/Legend-of-the-Green-Dragon
Cheap VPS Hosting (10$ credit!): https://m.do.co/c/acde75b086c5

A new server in the making...
Nightborn
Captain of the Guard
***
Offline Offline

Posts: 221


View Profile WWW
« Reply #4 on: April 28, 2019, 02:22:44 PM »

Yeah, sure, that's shorter and better =)
Logged
Pages: [1]   Go Up
  Print  
 
Jump to:  


*
DragonPrime Notices
Please take the time to read the FAQ and browse the DragonPedia

Support Us
No funds raised yet this year
Your help is greatly appreciated!
Recent Topics
DragonPrime LoGD
Who's Online
114 Guests, 0 Users
Home Forums News Downloads Login Register Advanced Search