DEV Community

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

Keff on October 26, 2021

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 securi...

Read full post

RRKallan • Oct 27 '21

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 • Oct 28 '21 • Edited

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

RRKallan • Oct 28 '21 • Edited

@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 • Oct 28 '21

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 • Oct 28 '21 • Edited

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 • Oct 28 '21

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

oz9un • Oct 26 '21

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

oz9un • Oct 26 '21

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

Keff • Oct 26 '21

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 • Oct 26 '21

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 • Oct 26 '21

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 • Oct 26 '21

Yup, you got it right!

Pavel Kutáč • Oct 26 '21

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

Keff • Oct 26 '21

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

Pavel Kutáč • Oct 26 '21

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 • Oct 26 '21

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áč • Oct 26 '21

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

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.
Prepare your own XML config, sample one is in my repo above too
And then just run ./vendor/bin/phpcs --standard=./phpcs.xml

Keff • Oct 26 '21

Fantastic easy enough, thanks a lot for the info.

Bivek • Oct 27 '21 • Edited

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

to:

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

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

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

note: getPin => must always return string.

Keff • Oct 27 '21

Nice, thanks for the solutions! Never trust user input

neoan • Oct 26 '21

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 • Oct 26 '21 • Edited

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

agounane • Oct 26 '21

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

Posandu • Oct 26 '21

Huh?

William Stam • Oct 26 '21

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 • Oct 27 '21

Wow, interesting bug, thanks for sharing the story

Artur Świerc • Oct 27 '21

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

Keff • Oct 27 '21

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