Back in September, Socorro received a security bug relating to the method we were using for processing inputs for the duration of certain reports. The vulnerability included a proof of concept, with an alert box popping up on production when the link was followed.

The Vulnerability

I was quite surprised at the root cause of the vulnerability. We had opted to compare the incoming data against a known set of valid values – a common practice when whitelisting certain inputs. Here is a sample of the type of code we were using to check for valid inputs:

<?php function validateDuration($duration) { $valid = array(3, 7, 14, 28); if(in_array($duration, $valid)) { return $duration; } else { return false; } }

This snippet of code is contrived for this example (you can see the actual code here, but it’s complexity makes for a bad example); however it’s true to form for what we were doing. As expected, when this code is tested, a string of ‘3’ and an integer of 3 work equally well, and a string of ‘5’ and an integer of 5 fail equally. Since the $duration value is coming through the query string, it’s especially important that the function work with strings, since all query string values are presented to PHP in this fashion.

However, we quickly discovered something interesting. As long as we passed in a string that started with a valid numerical value (3, 7, 14, 28), anything we pasted on the end was treated as acceptable by the application. We discovered the reason for this is because in_array() cast $duration to an integer before comparing it to an array of integers. As long as the $duration value started with a known numerical value (which was then cast to a valid integer), the system was fooled.

In PHP, if you cast a string to an integer, and the string begins with a numerical value, the string becomes that numerical value and anything beyond that is lost. If you start a string with alphabetical characters, however, the integer you get is zero. For example:

<?php $string1 = '123abc'; $string2 = 'abc123'; echo (int)$string1; //123 echo (int)$string2; //0

As a result, $duration = ‘3 /><img src=x onerror=”alert(1)’ was being found valid, because when cast to an integer it became int(3), but when returned the whole string was returned!

The code could be fixed with just a small tweak: casting the $duration value to an integer and discarding the original string:

<?php function validateDuration($duration) { $valid = array(3, 7, 14, 28); $duration = (int)$duration; if(in_array($duration, $valid)) { return $duration; } else { return false; } }

This ensures that only the valid values are returned by making sure that only an integer value is possible.

Lessons Learned

At first we thought this surely had to be a bug in PHP. However, Laura Thomson told me “If comparing two values, type juggling is performed first, which means that the string is converted to a number. This is done by taking the first number found in the string (see http://us2.php.net/manual/en/language.types.string.php#language.types.string.conversion ). So this may be confusing/a quirk/a gotcha, but it isn’t a bug.” And she’s right: this isn’t a bug per se, but it’s certainly an interesting “gotcha.”

Were I to write a unit test for this in the future, I would write the test in such a way to pass it both valid and invalid data, including strings that started with valid data but didn’t fit the exact format. The experience also taught me that I cannot implicitly trust non-security functions to provide adequate security protection and that we must completely and comprehensively test their behaviors, because “expected behavior” may not be “secure behavior” for our purposes.

Finally, there’s the lesson that even the most simple web application can have vulnerabilities in it. They’re a part of life. This is important because there’s a bunch of stuff being bandied about right now regarding the supposed insecurity of a significant framework. We can never assume we are secure until we have tested all avenues, and even then, vulnerabilities can crop up.

Frustrated with your company’s development practices? You don't have to be! No matter what the issues are, they can be fixed. You can begin to shed light on these issues with my handy checklist. Plus, I'll help you with strategies to approach the issues at the organization level and "punch above your weight."

Great! We'll be updating you soon on best practices for your team!

Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP