Actually it's 1234567890. Which is even worse.

Cryptocat is written by people who don't fully understand how their program works. They don't fully understand how the platform on which their program is built works. They may be smart, but they need to sit down and learn things.

This is the most important take-away. They're not just incompetent at crypto, they're incompetent at programming.

And they don't understand that they don't understand programming.

Fascinating. For those of us not versed in cryptography, can anyone elaborate on why

  foo.random() * 123456789

is insecure, and why it is made moreso with the added zero?

Is this what the author describes, using floating point to generate random numbers (where he cites this code)? I am wondering about the theory behind this.

I'm not a crypto guy, so this is just my interpretation of part of the issue. Anything that should be random that introduces easily predictable could be a problem.

With that in mind, what we've got is a sixteen digit floating point number (their random number generator works in decimal, not binary) multiplied by a very specific number.

This is then turned back into a string, so I'm going to assume it's ascii (I'm sure a similar problem would be if this was in utf-8).

Now, ascii representations of numbers means that each digit is converted to hex 30 to 39

So, what we're passing into the MD5 sum is 3-3-3-3-3-3-3-3... with one of them 2E because of the decimal place.

The first digit cannot be a 0 or the decimal place, so it's going to be 31-39 to start with. This first digit is also significantly more likely to be a 1 than anything else, here's a random sample of the first digit counts:

    Counter(str(get_uniform()*1234567890)[0] for i in range(100000))

Counter({'1': 28042, '2': 9208, '5': 9068, '3': 9053, '7': 9045, '9': 8987, '8': 8972, '4': 8829, '6': 8796})

How about the second digit? Still biased:

Counter({'0': 16368, '1': 16227, '2': 10942, '6': 8141, '5': 8124, '3': 8118, '4': 8067, '9': 8012, '7': 8006, '8': 7995})

Sixth digit? More normalised but the decimal pace is not really that common here.

Counter({'8': 10235, '2': 10166, '4': 10114, '9': 10032, '6': 10007, '3': 9969, '7': 9911, '0': 9883, '1': 9872, '5': 9804, '.': 7})

Ninth digit? Now we're getting flatter

Counter({'2': 9418, '1': 9406, '9': 9279, '3': 9269, '0': 9258, '4': 9248, '7': 9228, '5': 9227, '8': 9215, '6': 9208, '.': 7244})

Where do we see the decimal place?

    Counter(str(get_uniform()*1234567890).find(".") for i in range(100000))

Counter({9: 72887, 10: 18823, 8: 7460, 7: 740, 6: 81, 5: 7, 4: 2})

Fundamentally, we've taken a random series of bytes, turned them into a very regular pattern (3-3-3...) and added a significant bias towards particular digits in particular places.

How about the length?

Counter({13: 90052, 12: 9150, 11: 792, 10: 6})

We've got something with a very predictable structure, in short.

Multiplying by 10 I don't think makes it any less secure, it's just silly. All you do is move the decimal place over by one, although to be fair at least it doesn't add any more bias.

That line isn't used in anything sensitive.

The JsDoc for the function where

  var cnonce = MD5.hexdigest(""
      + (Cryptocat.random() * 1234567890));

occurs is

  /** PrivateFunction: _sasl_challenge1_cb
   *  _Private_ handler for DIGEST-MD5 SASL authentication.
   *
   *  Parameters:
   *    (XMLElement) elem - The challenge stanza.
   *
   *  Returns:
   *    false to remove the handler.
   */

which is only called by

  /** PrivateFunction: _connect_cb
   *  _Private_ handler for initial connection request.
   *
   *  This handler is used to process the initial connection request
   *  response from the BOSH server. It is used to set up authentication
   *  handlers and start the authentication process.
   *
   *  SASL authentication will be attempted if available, otherwise
   *  the code will fall back to legacy authentication.
   *
   *  Parameters:
   *    (Strophe.Request) req - The current request.
   */

Now, I might be nuts, but unless the documentation is incorrect, this appears to be a "sensitive" application.

To make sure I'm not just going on innuendo, I read into SASL authentication a bit ... seems `cnonce' is pretty important:

  A unique, encoded value that is generated by the client 
  for each challenge response, and that is used to avoid
  chosen plaintext attacks, provides some message integrity
  protection, and provides mutual authentication. This
  authentication is provided mutually in that the server
  proves it knows the user’s secret information and not in
  that the server proves its identity. The nonce must be
  specified if a QOP directive is sent.

Am I dead wrong, or are you hand-waving to defend your product?

I notice they've continued their defense in an update to the blog post:

"One more small note: Much has been said about a line of code in our XMPP library that supposedly is a sign of bad practice — this line is not used for anything security-sensitive. It is not a security weakness. It came as part of the third-party XMPP library that Cryptocat uses."