ASP stats code exploit

For discussion of the BB stats code.
nos

ASP stats code exploit

Postby nos » Mon Jan 28, 2008 12:20 pm

Hey all,

I think there is a small security threat in the ASP stats code.

When reading through the code i didnt see any checking/cleaning of the pid data,
this could be exploited by an attacker to gain access to your data base
and if u were using the same data base as the rest of your web site
then the attacker could have allot of sensitive information.

I gather MSSQL is exactly the same as MySql when it comes to injection exploits, if so the user could simply change the pid query and inject SQL in a pids var.

Code: Select all

?pid=101704187&pid=101704187&pid=SELECT * FROM `pid2142` WHERE 1


thats just an example it wont work but its not hard to modify that so it would.

correct me if I'm wrong but I'm sure i didn't see any cleaning anywhere in the script.

User avatar
Mort
Alpha-Forum Whore
Alpha-Forum Whore
Posts: 4232
Joined: Thu Dec 01, 2005 10:40 pm

Re: ASP stats code exploit

Postby Mort » Mon Jan 28, 2008 4:03 pm

I don't expect it would be an issue. The only data (and PIDS) in the database would be the ones we've collected, and anything else would simply not return a match which would result in a blank dataset. It might be worth looking into a bit further, but from memory, the way we treat all things like the PID's, then any SQL code "injected" in it's place would simpy be an invalid value, create a "corrupt" value dataset, or just return an error when the next part of the script tried to parse the results.

As far as security goes (worse case scenario), you would need to be using the same account for the stats database as the other databases on your site. In my case, I have about 7 different databases running on my SQL box and each of them uses an account that only has permissions to it's allocated database. Worse case is they would be able to get access to *all* the stats data we have gethered so far, possibly creating a DOS, but my current server is so slow and crashes enough on it's own I couldn't tell the difference between BAU and DOS.
Image

nos

Re: ASP stats code exploit

Postby nos » Mon Jan 28, 2008 4:51 pm

Mort wrote:I don't expect it would be an issue. The only data (and PIDS) in the database would be the ones we've collected, and anything else would simply not return a match which would result in a blank dataset. It might be worth looking into a bit further, but from memory, the way we treat all things like the PID's, then any SQL code "injected" in it's place would simpy be an invalid value, create a "corrupt" value dataset, or just return an error when the next part of the script tried to parse the results.


it is possible to insert the sql into the GET request made to your script,
an attacker would just grab the pid for an account in your stats list then
inset a new pid field with sql into the GET request.

Code: Select all

?pid=101704187&pid=101704187&pid=SELECT * FROM `pid2142` WHERE 1


now all the pid values match and the sql gets executed.

Even if its not connected with other databases the attacker could
delete your stats database.


simple fix is to just check that each pid, that gets passed to the script by
a GET request is NUMERICAL.


any values that are passed from Client side to the server, that interact with sql
should be checked and cleaned.

User avatar
Mort
Alpha-Forum Whore
Alpha-Forum Whore
Posts: 4232
Joined: Thu Dec 01, 2005 10:40 pm

Re: ASP stats code exploit

Postby Mort » Mon Jan 28, 2008 6:06 pm

I can see what you are trying to say, and ensuring url options are clean is good practice... something to add.

However, I still don't think it's anything (for the stats code) to worry about. The account that the scripts are connecting the to db as should only ever have read permissions anyway, and only to the stats database. There's no deleting that can be done by that account. The only account that would need to write to the DB is the one used by 2142datafeed.php, and that shouldn't be a user accesable script. One reason why I never implemented a user control panel (for enabling/add/disabling etc) was the fact that the system doing it would need some sort of write permissions, and I didn't trust my coding abilities enough to write something that would be secure enough for *anyone* to have access to.

The PID variables are read in into an array, and then seperate queries are run against each value of the array, I'm still not sure there's a way for the injected sql query to run. I'm going to do some debug testing and see what happens, and maybe have a look for other possible weak points like this. While ensuring the pid values are numerical only will fix that vector, there may be other options that exist in different parts of the code that will need to be checked for injected code elements before acceptance.

I'll mention it to Piers and see if it's something he'll look at.
Image

User avatar
Mort
Alpha-Forum Whore
Alpha-Forum Whore
Posts: 4232
Joined: Thu Dec 01, 2005 10:40 pm

Re: ASP stats code exploit

Postby Mort » Mon Jan 28, 2008 7:44 pm

Just did some tests and I'm sure it's perfectly safe.

All SQL queries are made using values read directly from the player2142 table, which is only editable by an admin directly or using pideditor. The variables for PID or anything else used on the URL line are never actually used to generate any SQL queries.

The SQL queries get the relevant details of player PIDs from player2142, then in the display loops, the options given on the URL are matched to known PIDs in the database. If it doesn't match (as in the case where someone has modified the URL) then the script will simply fail. As far as I can tell, nothing that is entered on the URL line is ever used as part of a SQL query line directly.

I didn't go through the code with a fine tooth comb or anything, but I checked the most relevant sections and couldn't spot anything that might be a problem.

If you do spot anything you are concerned about though, let me know so I can look at it more closely.
Image

nos

Re: ASP stats code exploit

Postby nos » Tue Jan 29, 2008 6:34 am

Mort wrote:Just did some tests and I'm sure it's perfectly safe.

All SQL queries are made using values read directly from the player2142 table, which is only editable by an admin directly or using pideditor. The variables for PID or anything else used on the URL line are never actually used to generate any SQL queries.

The SQL queries get the relevant details of player PIDs from player2142, then in the display loops, the options given on the URL are matched to known PIDs in the database. If it doesn't match (as in the case where someone has modified the URL) then the script will simply fail. As far as I can tell, nothing that is entered on the URL line is ever used as part of a SQL query line directly.


the PIDs are taken from the url and queried to the data base.
your right for most of the code the sql request are made by selecting records by dates,
but there is one piece of code that could be used to attack.

Grab pid data from GET Request.

Code: Select all

if Request("pid") = "all" then
   redim pids(numnicks)
else
   pidst = Replace(Request("pid")," ","")
   pids = split(pidst,",")
end if[/quote]



Grab all data for a PID( this is the inject loop hole)

Code: Select all

function getFullData() ' get all the data for a pid
   fixFields = Array("attp-0","attp-1","awin-0","awin-1")

-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   sqlqry = "SELECT * FROM player2142 WHERE [asof] = "&lastDate&" AND ([pid]="& join(pids," OR [pid]=") & ") ORDER BY CONVERT(INT, crpt) DESC"
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------

   Set rstGetRows = cnnGetRows.Execute(sqlqry)
   do until rstGetRows.EOF
       playerID = rstGetRows.Fields.Item("nick") ' pid or nick ??
      Dim nameHash ' hash of fields
      Set nameHash = CreateObject("Scripting.Dictionary")
      for each x in rstGetRows.Fields
            nameHash.Add x.name, x.value
      next
      for ff = 0 to Ubound(fixFields)
         tarr = split(nameHash.Item(fixFields(ff)),",")
         nameHash.Item(fixFields(ff)) = tarr(0)
      next
      playerHash.Add playerID, nameHash ' make multidimensional hash
       rstGetRows.MoveNext
   loop
   closeResult()
end function


join(pids," OR [pid]=") is using the pids gathered from the GET request.

User avatar
Piers
Comfortable with my sexuality
Comfortable with my sexuality
Posts: 1408
Joined: Fri Dec 02, 2005 7:57 am
Location: Horse country

Re: ASP stats code exploit

Postby Piers » Tue Jan 29, 2008 7:45 am

Yeees, but the query is built by joining the pid array with " OR [pid]=", so the resulting query would be invalid SQL:

Code: Select all

SELECT * FROM player2142 WHERE [asof] = "&lastDate&" AND ([pid]=101704187 OR [pid]=101704187 OR [pid]=SELECT * FROM `pid2142` WHERE 1) ORDER BY CONVERT(INT, crpt) DESC


Note that :
(a) you can only supply values that are going to be pids, because you get OR [pid]= inserted
(b) the pid values are expected to be numeric by the query, so a string injection will fail anyway (unless you're clever)

I can think a way to do it, but I'm not sure it would work anyway, let me tinker...

BTW, why did you use the redundant "WHERE 1" ? Does it have a functional difference to having no WHERE at all?
Image

User avatar
Piers
Comfortable with my sexuality
Comfortable with my sexuality
Posts: 1408
Joined: Fri Dec 02, 2005 7:57 am
Location: Horse country

Re: ASP stats code exploit

Postby Piers » Tue Jan 29, 2008 7:47 am

Scott, you might want to remove your testing code from the stats page, it's printing out all the individual SQL queries.
Image

User avatar
Mort
Alpha-Forum Whore
Alpha-Forum Whore
Posts: 4232
Joined: Thu Dec 01, 2005 10:40 pm

Re: ASP stats code exploit

Postby Mort » Tue Jan 29, 2008 10:24 am

yeah... forgot to clean up :oops:

not sure I have access from work to do it, so I may not be able to fix it till I get home.
Image

nos

Re: ASP stats code exploit

Postby nos » Tue Jan 29, 2008 10:26 am

(a) you can only supply values that are going to be pids, because you get OR [pid]= inserted
(b) the pid values are expected to be numeric by the query, so a string injection will fail anyway (unless you're clever)


its possible to insert a whole new query to the OR pid join.

BTW, why did you use the redundant "WHERE 1" ? Does it have a functional difference to having no WHERE at all?


WHERE 1 statement was simply an example, the query i posted was not to show a working hack for the
sql injection just to show where the sql would be placed.

I have not bothered to setup a working sql injection but I'm sure its possible.

As i said b4 u just need to check all pids are numerical. :mrgreen:

User avatar
Piers
Comfortable with my sexuality
Comfortable with my sexuality
Posts: 1408
Joined: Fri Dec 02, 2005 7:57 am
Location: Horse country

Re: ASP stats code exploit

Postby Piers » Tue Jan 29, 2008 7:25 pm

nos wrote:its possible to insert a whole new query to the OR pid join.

Yeah, I tried a few (including ...&pid=pidnum%20OR%20[pid]>1 ) but it didn't inject it, the query returned only one extra record, which wasn't the pid num specified by pidnum (obviously the first one returned that was > 1)

I'll make an edit, thanks.
Image

nos

Re: ASP stats code exploit

Postby nos » Wed Jan 30, 2008 6:36 am

Not a problem mate.

I'm just paranoid about sql, its just so esay to exploit and hack.

User avatar
Piers
Comfortable with my sexuality
Comfortable with my sexuality
Posts: 1408
Joined: Fri Dec 02, 2005 7:57 am
Location: Horse country

Re: ASP stats code exploit

Postby Piers » Fri Feb 01, 2008 9:36 am

Ah, went to make the update and discovered this is another example of you working off an old version.
I dropped that code when I discovered it broke if one of the selected players didn't have an up to date datafeed.

Current code is:

Code: Select all

function getFullData() ' get all the data for a pid
   fixFields = Array("attp-0","attp-1","awin-0","awin-1")

   for each pid in pids

   sqlqry = "SELECT TOP 1 * FROM player2142 WHERE [pid]="&pid
   Set rstGetRows = cnnGetRows.Execute(sqlqry)

       playerID = rstGetRows.Fields("nick") ' pid or nick ??
      Dim nameHash ' hash of fields
      Set nameHash = CreateObject("Scripting.Dictionary")
      for each x in rstGetRows.Fields
            nameHash.Add x.name, x.value
      next
      for ff = 0 to Ubound(fixFields)
         tarr = split(nameHash(fixFields(ff)),",")
         nameHash(fixFields(ff)) = tarr(0)
      next
      playerHash.Add playerID, nameHash ' make multidimensional hash

   Next
   closeResult()
end function


Mort, can you make a new bundle??

Same thing with shortData:

Code: Select all

function getShortData() ' get summary data for basic table
   redim shortdata(numnicks,18)
   For nn = 0 To UBound(nicks, 2)
      sqlqry = "SELECT TOP 1 [asof], [pid], [nick], [gsco], [tt], [klls], [dths], [win], [los], [kkls-0], [kkls-1], [kkls-2], [kkls-3], [crpt], [rnk], [pdtc] FROM player2142 WHERE pid = "&nicks(0,nn)
      Set rstGetRows = cnnGetRows.Execute(sqlqry)
      sqldata = rstGetRows.GetRows()
      For k = 0 To UBound(sqldata, 1)
         if isNull(sqldata(k,0)) then
            shortdata (nn,k) = 0
         else
            shortdata (nn,k) = sqldata(k,0)
         end if
      Next ' k
      shortdata (nn,5) = int(shortdata (nn,9)) + int(shortdata (nn,10))+ int(shortdata (nn,11))+int(shortdata (nn,12))
      ' correct kill count
   Next ' nn
   closeResult()
end function


Note that these updates are to handle database update failures and aren't massively essential.
I never got around to implementing a sort by career points of short data once I'd made this change.
Image

User avatar
Piers
Comfortable with my sexuality
Comfortable with my sexuality
Posts: 1408
Joined: Fri Dec 02, 2005 7:57 am
Location: Horse country

Re: ASP stats code exploit

Postby Piers » Fri Feb 01, 2008 9:40 am

Mort, I've just updated getFullData to be:

Code: Select all

function getFullData() ' get all the data for a pid
  fixFields = Array("attp-0","attp-1","awin-0","awin-1")

  for each pid in pids
  if isNumeric(pid) then
   sqlqry = "SELECT TOP 1 * FROM player2142 WHERE [pid]="&pid
   Set rstGetRows = cnnGetRows.Execute(sqlqry)

       playerID = rstGetRows.Fields("nick") ' pid or nick ??
      Dim nameHash ' hash of fields
      Set nameHash = CreateObject("Scripting.Dictionary")
      for each x in rstGetRows.Fields
            nameHash.Add x.name, x.value
      next
      for ff = 0 to Ubound(fixFields)
         tarr = split(nameHash(fixFields(ff)),",")
         nameHash(fixFields(ff)) = tarr(0)
      next
      playerHash.Add playerID, nameHash ' make multidimensional hash
  End if
  Next
  closeResult()
end function
Image

User avatar
Piers
Comfortable with my sexuality
Comfortable with my sexuality
Posts: 1408
Joined: Fri Dec 02, 2005 7:57 am
Location: Horse country

Re: ASP stats code exploit

Postby Piers » Fri Feb 01, 2008 9:52 am

Quick update - I've added the include directive to tabFuncs, and added a quicksort to shortdata on the stats page so it's now sorted by career points instead of Nick.

mort, you'll definitely have to make a new bundle now.
Image


Return to “The Buddabudda Stats system”

Who is online

Users browsing this forum: CommonCrawl [Bot] and 0 guests