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...
For further actions, you may consider blocking this person and/or reporting abuse
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
<b>Warning</b>: Undefined array key
when pin doesn't exist as paramMy suggested solution would be as follow, with the assumption that
$user->getPin()
is a trimmed alphanumeric value.Nice stuff, now that's how you do it!! Thanks for the detailed explanation
@nombrekeff thx
I see only one missing explanation
In my example i use
!empty($pin)
and notisset($pin)
Explanation as code
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 :)
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? Andempty
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.
I would not define my self as PHP developer. My cup of thea is HTML(5), (S)CSS and Javascript :p
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 assignedcorrect if you mean
!empty() > 0
is not a empty string if character length > 0, better saidempty()
returnstrue
as boolean when character length = 0. NOTE a space is a characterFantastic, thanks again! I might need to refresh my php knowledge
Passing "pin": true would be enough to trick PHP I guess. Good practice though :)
Research of "PHP Type Juggling Vulnerabilities" is a must-do for every PHP developer.
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
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.
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!
Yup, you got it right!
I think it is invalid code, as
params
hereif(params['pin'] != $user->getPin())
are missing$
in the beginning...Good catch, that was my mistake sorry, it should contain the
$
. I've fixed that, though the bug was not that 😜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
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)
I'm running this in CI/CD. Basically you
composer require --dev squizlabs/php_codesniffer
orcomposer require --dev arxeiss/coding-standards
to install directly my prepared set of rules../vendor/bin/phpcs --standard=./phpcs.xml
Fantastic easy enough, thanks a lot for the info.
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.
Nice, thanks for the solutions! Never trust user input
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
Although I didn't test it, the non-strict
!=
will make this check not pass in case you submittrue
. Thanks for sharing.if($params['pin'] === $user->getPin()) {
return "The pin is correct!";
}
throw new HttpException(403, "The pin is incorrect");
// this one is more safer
Huh?
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".
Wow, interesting bug, thanks for sharing the story
$user->getPin() can be nullable? If yes, then the condition will be true even when I put there an empty string.
Hey there! It actually can't be bull, it will always be a string of n-length.