Shame on Me: the missing code review (or is it unit testing?)

So a bug was reported not long ago. Let’s say “sensitive data” was available where it shouldn’t have been. I had an API view, a view that was returning json only, or should have been. Apparently early on in the project, I had added a comment to that view, to make sure it was returning the appropriate data. So I had added an html comment to a view that was supposed to be returning json. And I had it outputting everything under the sun it could output.

<!--
"secure data" => "stuff I don't want the user to see",
... 30-40 lines of this ...
-->
{"success": true}

So there’s 2 problems with that. The most important of which is that with DOM inspection tools, like chrome developer tools or firebug, you can inspect the return value of that request and see the data I don’t really want you to see. The other problem is that I’ve created invalid json, and whatever is checking that success is clearly not using that value.

Why did I do this?

This was early on in development and I was debugging the easiest way I could.

What could have prevented this?

A code review of any kind. One look at that file would have been a red flag to anyone. But a view file that was only outputting true or false never seemed like something I ever needed to look at once I got it working.

OWASP Top 10

Often software developers like to ignore security concerns because it’s a huge time sink. Regardless, it’s a great idea to review the OWASP top 10 every so often to make sure you’re refreshed on what to look out for.

1) Injection
taking parameters directly from the query string into a query
use parameterized queries

2) Broken Authentication and Session Management
sending the sessionID in the query string
store sid in cookie only

3) Cross Site Scripting (XSS)
taking parameters directly from the query string and printing them on the page
all user supplied input sent back to browser needs to be properly escaped

4) Insecure Direct Object References
no checking when query parameters are requesting actions that shouldn’t be allowed by the current user
use indirect object references (don’t use db keys in the view, but rather have an internal mapping to the db keys that would obfuscate them)
Or check authorization on every action

5) Security Misconfiguration
general security issues. default passwords, outputting errors, software (dbms, apps, libs) patches
change default passwords
turn off error outputting / all debugging
be proactive about applying software patches to components

6) Sensitive Data Exposure
encryption (storage and transit)
encrypt “sensitive” data
public key encryption with private key decryption only on the backend

7) Missing Function Leven Access Control
UI showing access to admin functions, functions not checking authentication appropriately, checking authentication on server done without relying on user provided information

8) Cross-Site Request Forgery
– each link and form should include an “unpredictable token” otherwise, you can’t be sure it’s a forged request

9) Using Components with Known Vulnerabilities
– keep up to date on the libs you use

10) Unvalidated Redirects and Forwards
– don’t put part of (or the entire) redirect url in the query string