On 7 July 2013 17:20, Maxim Kammerer <[email protected]> wrote: > This thread started off with discussion of peer review, so I have > shown that even expensive, well-qualified peer review (and I am sure > that Veracode people are qualified) didn't help in this case.
As one of the people on this list who does paid security audits, I both want to, and feel obligated to, weigh in on the topic. I don't work for Veracode, I have done audits for projects in the LibTech space, I have not done one for Cryptocat. I would like to, and given enough free time might get around to it, but like _everyone_ in this space, doing something publicly and putting your name on it means holding yourself to a certain standard, and that standard requires time - a lot of time (on the order of 60-100 hours). A good security audit will give you two things. Firstly it will give you bugs. These bugs are usually constrained to issues that are security vulnerabilities (but not always depending on the issue/the auditor/the time available). We find these bugs through meticulous testing and reading source code (see the 60-100 hours), through checklists to make sure we don't omit anything (see https://github.com/iSECPartners/LibTech-Auditing-Cheatsheet for a Creative Commons version of mine), and through experience and hunches. Usually an audit is time boxed so we don't literally read every line - the 60-100 hours would balloon up considerably if it were the case. We read a lot of code, we see and find a lot of bugs. The best auditors are always reading about new bug classes and exploitation vectors so we can recognise these bugs when they are in new projects. The Cryptocat bug, using a random string (or decimal digits) instead of bytes - I've seen before, as most people in my company have. (Usually it's in the form of using a hexadecimal string instead of converting the hexadecimal into bytes.) I know Cryptocat has been audited by humans in this fashion before, I've read their report, and it found good bugs. Of course, no auditor is perfect - we never find every single bug that's in an application and we can never say something is 'safe'. Which is why we give you the second thing: We give recommendations for making your codebase better. In older, more mature codebases this usually takes the form of recommendations like "Everywhere you do file handling is wrong, and you need to centralize it, fix it in the centralized version, and make sure everyone uses that going forward." Those are the straightforward ones. Sometimes they're more defensive, like "You really like to use the php system() function for doing stuff like removing files from the filesystem and chmodding. You do really meticulous character escaping, so we couldn't get command execution - but nonetheless, you should really use the unlink() and chmod() functions, so you can be sure a bug never makes it's way in." Now those are pretty obvious examples. In a project where the developers are making every effort they can to lock things down, where they're making every effort to do things 'right' - if we still can't provide examples, we're not doing a very good job. There are a lot of defense in depth measures that can be applied to web applications. Request preprocessors can look for global IDs, and assert that the current session can access that object (in *addition* to the page-level checks on object access). Database query logging can assert that all queries that go into particular tables use a certain column in the WHERE clause. I can go on and on. A good source of these is an ex-coworker's talk "Effective approaches to web application security" http://www.slideshare.net/zanelackey/effective-approaches-to-web-application-security (honestly, Etsy is driving the industry forward with their techniques for proactive web app security.) Defense in depth lends itself very easily to 'classically' exploitable conditions around things like Cross Site Scripting, Direct Object Reference, Command Injection, Code Execution. Weak RNG and fundamental protocol flaws are much harder to talk about in terms of defense in depth. So, not avoid the hard problem, let's take this particular bug. What I would say is MOAR ABSTRACTION. Now, I'm not actually a fan of abstraction, I hate seeing a ton of one-line functions, but in this case, to prevent a bug like this from happening again, I would structure the code like this (taking into account I'm really bad at naming things): //rng.ext ///This class is a CSPRNG that outputs a stream of random bytes class RandomNumberGenerator { } //rng-types.ext ///This class outputs a random stream of the specified type static class RandomSequences { ///Return a random upper,lowercase alphabetic string static string RandomAlphaString() ... ///Return a random sequence of base 10 digits static string RandomDigits() ... ///Return a random sequence of bytes static byte[] RandomBytes() ... } //rng-objects.ext ///This class produces a specific random application-level object static class RandomObjects { ///Return a random RSA public & private key static RSAKey RandomRSAKey() ... ///Return a random symmetric key static SymmetricKey RandomSymmetricKey() ... ///Return a random username static string RandomUsername() ... //... } The RandomNumberGenerator class is ONLY, EVER, used by RandomSequences. This class will produce specific sequences that are needed. And the most critical part is that RandomSequences is ONLY, EVER, used by RandomObjects. RandomObjects produces all of the application-layer randomness needed by the application. Cryptographic keys, default passwords, usernames, whatever it is that you need, each gets its own function, and 99 times out of 100, that function will be a single-line call into RandomSequences. Each of these classes is pretty modular, and is unit tested up the wazoo. Verbose commenting is crucial, and whatever IDE you're using should grab those comments and display them to you via Intellisense as you're using those functions. (Visual Studio and Eclipse will do this with the /// commenting practice.) RandomNumberGenerator only ever produces random bytes, and is sent through your favorite strong random number generator tester. RandomSequences makes sure 1) that the only characters that appear in the output are the whitelisted ones and 2) that all the whitelisted characters appear at the correct distribution. RandomObjects is a simple, easily skimmable class to see what the type of each random object will be. "Random Username is generated out of AlphaString, okay. Random Password is generated out of AlphaString? Why not AlphaDigitsSymbols?" You apply similar unit tests to RandomObjects as RandomSequences but you can also give a second set of unit tests that make sure the objects fit the applications expectations (e.g. around length of the username). It's very difficult to write code that is defensivly architectured. You usually wind up with code that many consider 'not pretty' because it relies on a lot of dependency injection. DI lets you Mock objects and write more unit tests. Unit tests are lovely for making sure you codify your assumptions (e.g. RSA exponents will always be 65535) in a way that causes _shit to break_ if they are violated. Write lots of unit tests. Extract out the most security-critical code into seperate files, and push changes in those files to people in a persistent-but-not-overwhelming manner, maybe via a crytocat-security-diffs twitter feed.[0] Every bug you fix (or at least, every non-UI bug) you write a regression test for. Put preconditions on functions enforced by your compiler and runtime that all strings are passed around in UTF8 (or 7 bit ASCII until you're ready to try to tackle Unicode). Put assertions from your unit tests (e.g. the RSA exponent) into the actual class itself. Fail early and fail often (in the sense of blowing your program up on unexpected input). Never ever ever have an empty 'catch' block or a switch statement without a default: raise. And none of that even includes memory unsafe languages =P In conclusion: No project is bug free, the best to the worst developers make critical mistakes. Offensively architecting a project with a threat model, protocol spec, and peer review of design is critical. Defensively architecting a project for easy review, deployment, testability, and trying to prevent exploitable conditions at multiple layers of the stack is also critical. If you think this bug could never happen to you or your favorite pet project; if you think there's nothing you can learn from this incident - you haven't thought hard enough about ways it could have been prevented, and thus how you can prevent bugs in your own codebase. -tom [0] https://codereview.crypto.is/ is/was a mostly dormant attempt at getting security experts fed a set of changes in areas of their experience. -- Too many emails? Unsubscribe, change to digest, or change password by emailing moderator at [email protected] or changing your settings at https://mailman.stanford.edu/mailman/listinfo/liberationtech
