DEV Community

Cover image for Can you find the bug in this piece of php code? 🤹
Keff
Keff

Posted on

Can you find the bug in this piece of php code? 🤹

Hey there! 👋

I was doing a bit of bug-hunting in an old project of mine when I found an interesting bug. Can you find it?

If you know your security or PHP this might be quite easy for you. Otherwise, it might be a good exercise.

!! Don't look at the comments to prevent spoilers if you want to solve it by yourself !!


This is the request you would make to the server:

curl --location --request POST 'https://super.secure-api.com/check-pin' \
    --header 'Content-Type: application/json' \
    --header 'Authorization: Bearer <token>' \
    --data-raw '{
      "pin": <you_answer>
    }'
Enter fullscreen mode Exit fullscreen mode

And this is the code for that given endpoint (/check-pin)

if($params['pin'] != $user->getPin()) {
  throw new HttpException(403, "The pin is incorrect");
}

return "The pin is correct!";
Enter fullscreen mode Exit fullscreen mode

PD: This is just a demo, not real code. You should never check passwords/pins/secrets like this.


What input would you need to pass as pin to be able to bypass the check?

I will release a post in a couple of days explaining the bug in detail and how to fix it.

Discussion (28)

Collapse
rkallan profile image
RRKallan

My assumption would be that pin is an alphanumeric (letter or digit) value.

There are a couple possible fixes to prevent a bug / unexpected behaviour

  • change the Abstract Equality Comparison (!=) to Strict Equality Comparison (!==)
  • check if $params['pin'] is not empty. This will avoid <b>Warning</b>: Undefined array key when pin doesn't exist as param
  • check if $user->getPin() is not empty else throw specific error that user doesn't have a pin
  • trim value of $params['pin'] to avoid spaces on begin and end

My suggested solution would be as follow, with the assumption that $user->getPin() is a trimmed alphanumeric value.

$userPin = $user->getPin();
if (!empty($userPin)) throw new HttpException(422, "User has no pin, contact your system administrator");

$pin = !empty($params["pin"]) && ctype_alnum(trim($params["pin"])) ? trim($params["pin"]) : null;
if (!empty($pin)) throw new HttpException(403, "Pin is empty or isn't alphanumeric");

if ($pin === $userPin) return "The pin is correct!";

throw new HttpException(403, "The pin is incorrect");
Enter fullscreen mode Exit fullscreen mode
Collapse
nombrekeff profile image
Keff Author • Edited on

Nice stuff, now that's how you do it!! Thanks for the detailed explanation

Collapse
rkallan profile image
RRKallan • Edited on

@nombrekeff thx

I see only one missing explanation

In my example i use !empty($pin) and not isset($pin)
Explanation as code

$x = " ";
$trimX = trim($x); // $trimX = ""

var_dump(isset($trimX)); // return true
var_dump(!empty($trimX)); // return false
Enter fullscreen mode Exit fullscreen mode

as we can see from the output it would be wise to use empty() in this case

BTW i'm very curious about your solution :)

Thread Thread
nombrekeff profile image
Keff Author

Ohh nice point, I did not know that (I'm not a php dev), so it's really helpfull. I guess isset only checks whether the variable exists right? And empty checks if the length > 0, have I gotten that right?

My solution is quite similar to yours actually, I just make sure the input is what I expect, and I don't trust user input whatsoever.

Thread Thread
rkallan profile image
RRKallan • Edited on

I would not define my self as PHP developer. My cup of thea is HTML(5), (S)CSS and Javascript :p

I guess isset only checks whether the variable exists right?

Yes isset checks if variable is defined.
If i'm right $x = undefined is not possible and needs to be null when no value is assigned

empty checks if the length > 0

correct if you mean !empty() > 0 is not a empty string if character length > 0, better said empty() returns true as boolean when character length = 0. NOTE a space is a character

Thread Thread
nombrekeff profile image
Keff Author

Fantastic, thanks again! I might need to refresh my php knowledge

Collapse
oz9un profile image
oz9un<h1>

Passing "pin": true would be enough to trick PHP I guess. Good practice though :)

Collapse
oz9un profile image
oz9un<h1>

Research of "PHP Type Juggling Vulnerabilities" is a must-do for every PHP developer.

Collapse
nombrekeff profile image
Keff Author

Totally, I know many php devs that don't even know Type Juggling is a thing, and that it can lead to these kinds of behaviours. That's why I decided to post this simple case. Hopefully it reaches the right people

Thread Thread
oz9un profile image
oz9un<h1>

It's a great job for increasing awareness! I would love to contribute your "Find The Bug" series with various of challenges whenever I have time.

Thread Thread
nombrekeff profile image
Keff Author

That would be fantastic, just ping me here on dev and I will be more than happy to include them!!

I'm planing to release one post a month on this series (if I have the time and challenges), so the help would be really apreciated!

Collapse
nombrekeff profile image
Keff Author

Yup, you got it right!

Collapse
arxeiss profile image
Pavel Kutáč

I think it is invalid code, as params here if(params['pin'] != $user->getPin()) are missing $ in the beginning...

Collapse
nombrekeff profile image
Keff Author

Good catch, that was my mistake sorry, it should contain the $. I've fixed that, though the bug was not that 😜

Collapse
arxeiss profile image
Pavel Kutáč

Yeah, the issue is with != operator. And that is the reason I'm using strict rules for PHP CodeSniffer. It is based on Slevomat rules and my setting is here github.com/arxeiss/php-coding-stan...

The important one to avoid those issues is this rule

<!-- Disallow use == or != and must use === and !== -->
<rule ref="SlevomatCodingStandard.Operators.DisallowEqualOperators"/>
Enter fullscreen mode Exit fullscreen mode
Thread Thread
nombrekeff profile image
Keff Author

Nice that's it. Thanks for sharing your solution to the problem. Do you know if there is something similar to run on CI/CD? or would that just work fine? (I'm not a php dev so I don't know how this works in detail)

Thread Thread
arxeiss profile image
Pavel Kutáč

I'm running this in CI/CD. Basically you

  1. Install PHP CodeSniffer github.com/squizlabs/PHP_CodeSniffer with composer require --dev squizlabs/php_codesniffer or composer require --dev arxeiss/coding-standards to install directly my prepared set of rules.
  2. Prepare your own XML config, sample one is in my repo above too
  3. And then just run ./vendor/bin/phpcs --standard=./phpcs.xml
Thread Thread
nombrekeff profile image
Keff Author

Fantastic easy enough, thanks a lot for the info.

Collapse
8ivek profile image
Bivek • Edited on

To make it more secure:
I would change following line:
if($params['pin'] != $user->getPin()) {

to:

if($params['pin'] !== $user->getPin()) {

Or

$user_input = (string) $params['pin'];

if($user_input != $user->getPin()) {

note: getPin => must always return string.

Collapse
nombrekeff profile image
Keff Author

Nice, thanks for the solutions! Never trust user input

Collapse
sroehrl profile image
neoan

This is one of the reasons I recommend PHPStorm (or other capable IDEs) to beginners. It would scream at me when using non-type-safe comparison

Collapse
elidrissidev profile image
Mohamed ELIDRISSI • Edited on

Although I didn't test it, the non-strict != will make this check not pass in case you submit true. Thanks for sharing.

Collapse
agounane profile image
agounane

if($params['pin'] === $user->getPin()) {
return "The pin is correct!";
}
throw new HttpException(403, "The pin is incorrect");
// this one is more safer

Collapse
posandu profile image
Posandu

Huh?

Collapse
williamstam profile image
William Stam

Always check for what you want and not for what you don't. Like in the above its if the passed pin is type and equal to the pin the you have a valid pin.

I remember a thing a while ago where there was (is?) A logic bug in the unreal engine where they basically do something like if the distance you traveled is out of the max then you did something wrong reset else update character position. So anything that triggers the else on that logic block would be "acceptable". Dont do this. Always test for what you want and ease of on the else's. In unreals case the bug was exploited by triggering a null value which if anything null almost always ends in the else block. So the next update on position will be like if last (null) minus now position is out of bounds throw error else update character position. And we know null kicks it to the else so all the positions were basically "ok".

Thread Thread
nombrekeff profile image
Keff Author

Wow, interesting bug, thanks for sharing the story

Collapse
aswierc profile image
Artur Świerc

$user->getPin() can be nullable? If yes, then the condition will be true even when I put there an empty string.

Collapse
nombrekeff profile image
Keff Author

Hey there! It actually can't be bull, it will always be a string of n-length.