[liberationtech] DecryptoCat

[Tom Ritter]

On 7 July 2013 17:20, Maxim Kammerer <mk at 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.