Friday, December 7, 2012

Security is hard. While security-related issues are fun and challenging subject for many - fun enough for them to take part in various CTFs, crackmes etc, it's usually not the first thing a developer cares for. Yes, they do have other priorities. That's why usually leaving a developer with a task of writing security-related code results in either:

Accept that it's a part of the family now (= treat it with as much scrutiny as your own code).

Whatever you're bringing into your codebase, wherever it came from - it's your responsibility now. No matter if it's a snippet found in a forum, a github.com hosted library used by a few dozen people or a project used for many years and extensively tested. It may have a security vulnerability, it may be used insecurely, it may not be a right tool for a task. Be skeptical.

The code allmighty

There are no sacred cows. Any code is just that - a piece of instructions made by humans, with a certain possibility of having errors, some of that security related. And every human makes mistakes, some of them catastrophic.

Let me give you an example - OWASP PHP CSRF Guard - a small library for enriching your PHP application with CSRF protection. Similar to what OWASP CSRFGuard does for Java applications. This small library is presented in Open Web Application Security Project wiki, so it must be secure.....Right?

Application uses separate tokens for every form. Upon form submission, it gets form id (CSRFName) and appropriate token (CSRFToken) from POST request and calls csrf_validate_token(). So far so good. Let's dig deeper though.

Function retrieves the token for a form id from session (get_from_session). Function returning false is some exception, let's skip that. Then token value from POST is compared to its session equivalent. Looks ok. But..

Total CSRF protection bypass. No sacred cows. Of course, the code in OWASP wiki has been fixed now.

Developers, remember: Do not blindly include libraries, even OWASP ones, without testing them, especially for security errors. If you can't do it - there are other people who can ( ^-^ ). After all, even if it's adopted, it's part of the family now.

19 comments:

"Do not blindly include libraries, even OWASP ones, without testing them, especially for security errors."

Yes, good guidance. The owasp code should get people in the right direction and I realize that there is no code immune to errors. Glad to see people digging in, finding issues, and also fixing the code so we're in a better spot.

Hello there bud, I'm AbiusX (the OWASP author of the code), it is mentioned in the page and in the discussion page and in the wiki and everywhere that this CSRF protection code, is a guideline. It is there to show you how to works.

I explicitly made it into parts and not a whole bunch of code so that people don't copy paste, but it seems they never listen. The main problem with the code is, regular expressions can not parse HTML and it only works for simple HTML (see this http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags). BTW thanks for the post, other problems with this code are fixed now.

That will never work. Once you post a code snippet it's bound to be copy-pasted somewhere. No warnings and other obstacles will help (but they should be added, good decision there!). For example - I once made a blog post specifically describing a vulnerable technique only to find my snippet a few months later on Stack Exchange, where a commenter recommended it.