1

I have a section of code where the user completes an input form in HTML. On post I validate their $_POST entry $userAmt against their points available $points in the database and then I proceed.

I've noticed that some users are able to circumvent my initial check and enter more points than are available in the database only as long as it's within the range of the second validation 1-20. So some part of the validation is working, but I'm guessing they are able to use a break point and edit the post variables after my initial validation? Here's a snippet of the code.

$query = mysqli_query($conn, "SELECT * FROM users WHERE id = '$id'");
$queryA = mysqli_fetch_assoc($query);
$points = $queryA['points']; //user points available


if(isset($_POST['submit'])){
$userAmt = $_POST['userInput']; //users input
$userAmt = mysqli_real_escape_string($conn, $userAmt);
$prize = $userAmt * 2;

    if($userAmt > $points ){ //<- attackers are circumventing
    $message = "You do not have enough points";
    } else if ($userAmt < 1 || $userAmt > 20){ //<-attackers are not circumventing
    $message = "Must be between 1 and 20";
    } else {
    // determine outcome and update DB
    // if user wins
    mysqli_query($conn, "UPDATE users SET points = points + '$prize' WHERE id = '$id'");
    }
}

Maybe I shouldn't be doing else if's and instead case switch?

m1xolyd1an
  • 535
  • 5
  • 18
  • This won't solve your initial problem, but a better way of doing this is a white list, rather than an escape string. If it only should contain integers, then you should only allow integers. – Evadecaptcha Aug 04 '15 at 15:06
  • Where do you get the $userAmt from? Is it a hidden field or the users enters something in there? If the user enters something in there, what is the value for $userAmt ? – Cris Kolkman Aug 04 '15 at 15:06
  • 1
    What specific inputs are you seeing? You're checking if "user amount" is greater than "points", but you're updating to "user amount * 2". So the points are always going to be double whatever the user enters. And no, users are no putting a breakpoint in your server-side code and modifying server-side in-memory values. If they could do that, you'd have *much bigger* security concerns. – David Aug 04 '15 at 15:08
  • @CrisKolkman `$userAmt` comes from the form input `$_POST['userInput']` – m1xolyd1an Aug 04 '15 at 15:09
  • And what does the user enter in there? Or better, what SHOULD the user enter in there? – Cris Kolkman Aug 04 '15 at 15:09
  • @David I'm seeing inputs like 20 when the user only has 2 points available in the DB. It somehow passes the check `if($userAmt > $points)` and if they win it will update their balance. – m1xolyd1an Aug 04 '15 at 15:11
  • you should check if that input's numeric also. you're not really doing that. – Funk Forty Niner Aug 04 '15 at 15:12
  • 1
    @m1xolyd1an: I guess I'm having trouble understanding even the logic of what this code is accomplishing, let alone how it may be abused. If a user has 2 points in the DB, they can post 2. Now they have 6 points in the DB. So they can post again with 6. Now they have 18. And so on. Are you sure the abuse is happening in a single post? What exact runtime values are in place that cause this to fail validation in a particular post? That is, for what value of points in the DB and user input value does it fail? – David Aug 04 '15 at 15:14
  • @David It's a game. If they win their input is doubled, if they lose their input is deducted from their available points. Users are able to input more than their available points. So lets say they only have 2 points, they are somehow able to bypass my validation and enter 20 points, if they lose their points are dropped to -18, but if they win it goes up to 42. – m1xolyd1an Aug 04 '15 at 15:27
  • 1
    this Q&A may be of use http://stackoverflow.com/q/27289859/ as per a comment you left under Marc's answer. – Funk Forty Niner Aug 04 '15 at 15:29
  • 1
    @m1xolyd1an: It sounds like there's a lot more code involved. Are you sure the problem is in that exact `if` statement? If a user enters `20` and the system doesn't prevent them from accruing points, can you also enter `20` and debug the code and find where/how exactly it's failing? – David Aug 04 '15 at 15:29
  • If I do it on my end the server blocks it saying "you don't have enough points". The attacker has found a way to bypass that validation, but not the `1-20`. I think that's where the issue is, I was wondering if it was the way I was doing `if` and `if else` etc. You're right there is a lot more code but I didn't want to paste a giant wall of code and ask for free debugging. Instead I wanted to simplify it and try to find the issue myself based on your guys' experience @David – m1xolyd1an Aug 04 '15 at 15:33
  • 1
    @m1xolyd1an: And believe me, we appreciate that :) But it really seems to be that there's something more to the problem, which may indeed take quite a bit more debugging on your end to determine. Being able to replicate the attack is going to be a *huge* step in figuring this out. As it stands in this code, if `$userAmt > $points` is true then the `UPDATE` won't be executed. So if `$points` is 2 and `$userAmt` is 20 then the validation will work. The attack may be circumventing something else entirely, perhaps by producing multiple false "wins" or something. – David Aug 04 '15 at 15:37
  • @Fred-ii- Thanks, You think it might a MIM attack? So I would have to go SSL? I followed the link to the hostname verification and looks like it will take a bit of time for me to digest. – m1xolyd1an Aug 04 '15 at 15:38
  • 1
    You're welcome. TBH, I don't know enough on how to deal with that particular subject (middle man), and Google'd around a bit to see if anything could be done to prevent the middle man issue. – Funk Forty Niner Aug 04 '15 at 15:40
  • @David Ok, I log all the games played, and they are definitely not able to produce false wins. They lose 10 times, then create a new account under a new IP since their account has gone severely negative (which isn't supposed to be allowed)and then try again until they win. I think I might try loggin their input to a temporary table and then validating that temp table against their database amount before updating their points. – m1xolyd1an Aug 04 '15 at 15:43
  • 1
    I am willing to bet this is down to php's type juggling: http://php.net/manual/en/language.types.string.php#language.types.string.conversion Try casting your variables to int : `$userAmt = (int)$_POST['userInput'];` – Steve Aug 04 '15 at 15:50
  • Thanks @Steve I'll give that a try. I guess it's pretty sloppy of me to allow the variables be a string and then try to validate them via greater than or less than arguments. – m1xolyd1an Aug 04 '15 at 15:56
  • 1
    No problem, i wrote an answer with a working example – Steve Aug 04 '15 at 15:59
  • Wow I'm blown away by this. Marked it correct! Thanks @Steve – m1xolyd1an Aug 04 '15 at 16:12

2 Answers2

5

Cargo-cult programming. Why are you sql escaping $userAmt, only to start doing math on it?

You're also simply ASSUMING that your select query succeeds. if it fails for whatever reason, then$points will be a boolean FALSE value, which type-casts to an integer 0 when used in > operations, basically

if ($userAmt > false)

is essentially going to be true, always, as long the user enters ANYTHING.

You need something more like this:

$result = mysqli_query($con, "SELECT ...") or die(mysqli_error($conn));
if (mysqli_num_rows($result) == 0) {
   die("who are you and how did you get here?");
} else {
   $row = mysqli_fetch_assoc($result);
   $points = $row['points'];
}

And beyond this, you shouldn't be doing your own escaping anyways. You're using mysqli - use prepared statements and placeholders, because you're still vulnerable to sql injection attacks.

Marc B
  • 356,200
  • 43
  • 426
  • 500
  • Thanks Marc, I didn't include it the entire code only the portions that I thought were the issue. When the user enters the page I already validate whether or not they are a user and successfully logged in. There are two portions that allow user input stored in a $_POST and those are the only entries being manipulated. The code I showed is a simplified version. A previous attacker last year let me know they were using Charles Web Proxy to edit my post variables which I assume is what is happening here... – m1xolyd1an Aug 04 '15 at 15:21
2

Its down to phps type juggling, and string comparison:

//your code, valunerable:
$userAmt = " 20 ";
$points = "10";

if($userAmt > $points ){ //<- attackers are circumventing
    $message = "You do not have enough points";
} else if ($userAmt < 1 || $userAmt > 20){ //<-attackers are not circumventing
    $message = "Must be between 1 and 20";
} else {
     $message = "winner";
}

You can avoid this by simply casting to int:

echo $message;
//fixed by type cast to int

$userAmt = (int) " 20 ";
$points = (int) "10";

if($userAmt > $points ){ //<- attackers are circumventing
    $message = "You do not have enough points";
} else if ($userAmt < 1 || $userAmt > 20){ //<-attackers are not circumventing
    $message = "Must be between 1 and 20";
} else {
     $message = "winner";
}

echo $message;

Live example: http://codepad.viper-7.com/bwkvG3

Steve
  • 20,703
  • 5
  • 41
  • 67
  • Holy smokes! That's so bizarre that `" 20 " * 2 = 40` but at the same time `" 20 " < 2` is true. Thanks so much! – m1xolyd1an Aug 04 '15 at 16:04
  • No problem. And yes, it is bloody strange, but thats the nature of php! – Steve Aug 04 '15 at 16:06
  • not really strange, just how php handles string->int. consider `" 2 0 " * 2` -> `4`. whitespace is skipped up until the first digit, and then any subsequent digits are considered part of the number, until the first non-digit characters. so `" 20 " ` gives you 40, but "2 0" gives you 4, because the space isn't a valid separator INSIDE a number, only on the before/after " edges". – Marc B Aug 04 '15 at 21:51