On 2013-07-08, at 2:00 PM, David Goulet <dgou...@ev0ke.net> wrote: > Hi everyone, > > Very good post Tom! :) > > I would like to point out something here, no bashing, but rather possible > improvements from my point of view. As Tom stated, basically if you don't do > code, you'll have no bugs so in other words there will always be bugs! > > Now, what's troubling me with this disclosure is probably a lack of experience > of the maintainers especially in open source software. There is 19 commits > between Jul 9, 2011 and Jun 3, 2013 (see second table of the decryptocat post) > which basically changes the keying scheme in production. There is 6 of those > only in October 2011! In terms of peer review, this is just not possible to > follow such a paste especially with so little testing upstream. > > Furthermore, looking at those lines of code, there is simply NO comments at > all, > nothing to help peer review, to explain why this or that is done that way and > nothing linked to any design document. This is in my opinion VERY important > that > developers design their system/subsystem beforehand *especially* a crypto > design > in a public document. And, if it has to change, the design should be peer > reviewed before even making one line of code. > > So, the technical critical issue, CryptoCat responded well, quickly but the > point here is that there is a problem in terms of how development is done and > how *little* the maintainability of the code is.
Hold your horses. There is an *Absurd* amount of difference between Cryptocat in October 2011 and today. You simply cannot compare the two codebases. They are completely different as Cryptocat was rewritten completely from scratch twice since 2011. The current codebase is, even if I do say so myself, modestly maintainable and well-documented. But really, bringing up the October 2011 codebase is really beside the point. I appreciate the rest of your post. Thanks for your input, David. NK > > Those guys (CryptoCat) are learning and getting experience to run such a > security critical open source project over the year and this episode should > be a > good wake up call to understand what when wrong and how to improve the > situation. Shit happens, now it's up to them to address this work flow issue > and > the community should help them like Tom did with that post! > > Else, I'm pretty sure we are going to see more and more of those bugs over the > year if the upstream code is treated like a development branch and is not > documented explaining the why of things to facilitate peer review and > maintainability. > > My two cents! > > Cheers! > David > > Tom Ritter: >> 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, 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 -- 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
