The Geek Bin

Hacking Code Bases: A primer

Curtis -

As a security-focused developer, I often get questions from colleagues and readers of this very blog on how to review code and spot vulnerabilities. Many believe this is an incredibly challenging task to complete and they are quite wrong, while there are some challenging moments to get it to exploitability, it’s generally very easy for developers to find flaws in their code.

Before I dive into an example based on a real-world vulnerability, I wanted to highlight that when reviewing code, either yours or peers, you should always take a few moments to read the context of the code and try to understand where the code is being called from. In many IDEs such as PHPStorm and their families, Visual Studio Code and more you can trace the callers with a simple Ctrl-Click on the function/variable name. You can even grep it throughout the entire code base if you prefer.

Example: E-Commerce Store

The fabulous e-commerce store Mittens For The Kittens is about to have their developers release a new payment integration with Stripe, making it easier to accept cards online and you’ve been tasked with reviewing it. In order to make the system easier, the developers created a class called PaymentObject, and PaymentObject contains all the user information, here’s what that code looks like:

class PaymentObject
{
    public $first_name;
    public $last_name;
    public $email_address;
    public $phone_number;
    public $address_one;
    public $address_two;
    public $city;
    public $state_province;
    public $country;
    public $uid_reference = null;
    public $accepted_payment = false;
    public $fraud = false;
}

When reviewing the PaymentObject class you notice it’s being generated by the view on an Ajax request to /PaymentInfo/create, on this page, you observe this block of code:

if( isset($_POST['payment_info']) ) {
    $payment_object = new PaymentObject;
    foreach($helper->defang($_POST['payment_info']) as $key => $value) {
         $payment_object->$key = $value;
    }
}

This all looks suspicious, I note that the value is being assigned directly to the object by it’s key, I wonder immediately if I can input some bad data and have it outputted. Back over on the view side, the ajax render’s the output based on the object information. 

function render_payer_info(payment_info) {
    view.select('payer.info').structure.add(payment_info).rerender();
}

It looks like we’re pushing the data into the stack here without any filtering client-side, still very risky but let’s dig deeper.

Our re-renderer appears to be slapping everything with basic client-side encoding using the strip_tags function in Javascript (third-party code). While this may be safe, it’s definitely risky. While it’s not exploitable immediately, let’s check out the other endpoints.

On the “View Billing Details” page, the values appear to be injected directly to the view like below, therefore, it may be vulnerable on other target pages, not the directly changed page.

$view->setData('payer_name', getObject('payer', $_SESSION)->first_name));

Immediately, one could assert that the same strip_tags function can remove this, while it may be true, let’s not assume anything just yet. Let’s review how the view is handing this information, Mittens is using the Twig templating library, here’s how that information is rendered.

<label for="payer_name">Payer Name:</label>
<input type="text" name="payer_name" id="payer_name" class="custom-stripe--payer-name" value="{{ payer_name }}">

That appears to be an issue, but before sending off the red flags, let’s test out our theory! To recap what we’ve found so far:

  1. User data is not being filtered except upon insert into the database
  2. The page collecting and re-rendering the data is not vulnerable without modifying client-side code
  3. Another page in the application (View Billing Details) appears to not be sanitizing the data collected

Now, we’ll write a basic test case to prove this code is vulnerable. In order to do that, I’m going to automate the tests using an internal testing framework named “Darnit”, here’s how that would look:

const Darnit: DarnitInstance = new DarnitInstance();
const browser: WebInterface = new WebInterface();

Darnit.setBrowser(browser);

browser.navigate('/login', { type: 'POST', info: { username: 'bob@smith.com', password: 'abc123' });
browser.ok();
browser.navigate('/account/set-order-details');
browser.getForm().autofill([
    'Bob "><script>alert(1)</script>',
    'Smith',
    'bob@smith.com',
    '555-555-5555',
    '123 Street',
    'Unit 1',
    'City',
    'State',
    'Country',
    ''
]);
browser.submitForm();
browser.ok();
browser.navigate('/account/view-billing-details');
browser.hook(Darnit.expecting('window.alert', '1'), true);
browser.hooked();
Darnit.PushScreenshotToFolder('./screenshots/', 'xss_view_billing_details', 'png');
browser.close();

Now, we’ve got our test setup to automatically hunt for the vulnerability. In normal circumstances one could argue it’s not worth making a test for it, however, oftentimes vulnerabilities are regressed upon therefore it’s always nice to write a test. Even if it turns out your assumption was wrong, at least there’s code coverage to make sure the assumption is never right.

Now, let’s fire up the test and see what happens…

[Darnit] Running test "mittens xss"
[Darnit:mittens xss] Browser navigated to "/login"
[Darnit:mittens xss] Browser navigated to "/account/set-order-details"
[Darnit:mittens xss] Browser autofilled information
[Darnit:mittens xss] Browser submitted information
[Darnit:mittens xss] Browser navigated to "/account/view-billing-details"
[Darnit:mittens xss] Browser hook requested.......................
[Darnit:mittens xss:hook] Expecting "window.alert" execution.................................... OK
[Darnit:mittens xss:hook] Received "1", expecting "1" OK
[Darnit:Screenshot] Saved screenshot as "xss_view_billing_details_1.png" in "./screenshots/"
[Darnit:mittens xss] Closing Browser

Our test showed that we did get our persistent cross-site scripting attack as expected. Now, we need to figure out a mitigation strategy. In this case, we’re lucky we have a centralized object that is getting passed around we can sanitize data on.

Going back to where we assign data, we can modify the code slightly and remove our immediate risk. Naturally, one could also force validation on the view, but it’s always nice to have it on the object and render.

if( isset($_POST['payment_info']) ) {
    $payment_object = new PaymentObject;
    foreach($helper->defang($_POST['payment_info']) as $key => $value) {
         $payment_object->$key = htmlentities($value);
    }
}

That would be an acceptable change in many cases. Always make sure to filter data, especially on the input to prevent anything such as this bug from sneaking through the cracks.

Key Takeaways

  1. Always sanitize user input and output from the database
  2. Always view the context around code and where it is referenced and used
  3. Write tests for vulnerabilities and assertations to ensure they don’t pass and nothing has regressed
  4. Never stop hunting for vulnerabilities in your code and peers'

That’s all for this Tutorial Tuesday, let me know your thoughts below!