On 2013-07-08, at 3:34 AM, Tom Ritter <t...@ritter.vg> wrote: > On 7 July 2013 17:20, Maxim Kammerer <m...@dee.su> 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,
Just a quick note out of your awesome email: If you don't have enough free time, let me help you make some. I am able to fund further auditing. Round up a team and get in touch! I sincerely appreciate the perspective in the rest of your email. NK > 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 compa...@stanford.edu or changing your settings at > https://mailman.stanford.edu/mailman/listinfo/liberationtech -- Too many emails? Unsubscribe, change to digest, or change password by emailing moderator at compa...@stanford.edu or changing your settings at https://mailman.stanford.edu/mailman/listinfo/liberationtech
