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>
    }'

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!";

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.

Comments

[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");

[Keff]

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

[RRKallan]

@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

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 :)

[Keff]

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.

[RRKallan]

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

[Keff]

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

[oz9un]

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

[oz9un]

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

[Keff]

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

[oz9un]

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.

[Keff]

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!

[Keff]

Yup, you got it right!

[Pavel Kutáč]

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

[Keff]

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

[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"/>

[Keff]

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)

[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

[Keff]

Fantastic easy enough, thanks a lot for the info.

[Bivek]

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.

[Keff]

Nice, thanks for the solutions! Never trust user input

[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

[Mohamed ELIDRISSI]

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

[agounane]

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

[Posandu]

Huh?

[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".

[Keff]

Wow, interesting bug, thanks for sharing the story

[Artur Świerc]

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

[Keff]

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