DEV Community

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

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...
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 • Edited

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

Collapse
 
rkallan profile image
RRKallan • 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
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

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

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

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

Collapse
 
oz9un profile image
oz9un

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

Collapse
 
oz9un profile image
oz9un

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

Collapse
 
nombrekeff profile image
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

Thread Thread
 
oz9un profile image
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.

Thread Thread
 
nombrekeff profile image
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!

Collapse
 
nombrekeff profile image
Keff

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

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

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

Fantastic easy enough, thanks a lot for the info.

Collapse
 
8ivek profile image
Bivek • Edited

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

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

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

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

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