make a stronger nonce

since checking in the database after creating a nonce could create a race condition, making the nonce stronger seems like a good solution.

actually, on second thought, the best thing to do here would be to come up with something clever, have that added to core's openid module, then just call their function. for d6 & d7, we can just be clever out here in contrib.

the openid nonce is the time plus four characters in the range [A-Z], or 26^4 possible nonces per second. if during a daily traffic peak 1000 users log into a given site in the same second, the odds of a collision are 1000/(26^4) = ~0.2188%. we thus could expect a collision at least once every 500 days on average, not even counting all the lower-probability collisions at other times of day. this is far too likely, in my opinion.

i realize that all these users would have to be using the same drupal site as their identify provider (idp) for a collision to happen. more importantly, i see that openid_verify_assertion_nonce() first writes a row to the openid_nonce table and then checks the count() of rows matching the current nonce and idp_endpoint_uri, to help avoid race conditions. however, count() cannot be relied upon for transactional consistency when using mysql cluster replication: http://dev.mysql.com/doc/refman/5.1/en/mysql-cluster-limitations-transac...

i believe this could happen in practise because 1000 users represents, for example, just 0.02% of the user base of a site like economist.com with > 5 000 000 registered users. that said, economist.com doesn't currently use openid :)

so, we should make the nonce longer, since the spec says it can be up to 255 characters and we're only using 24.

since drupal does not include an openid identity provider in core, openid.module only uses its function _openid_nonce() while running tests. the openid_provider module calls _openid_nonce() but should work around this flaw by generating its own, stronger nonces.

once we've done this for openid_provider, we can submit a patch to D8 core to use our new, improved nonces, or at least add a comment noting that _openid_nonce() might not be cryptographically secure.

dkg points out:

Just increasing the number of bytes without increasing the underlying pool of entropy isn't actually a win, of course. (e.g. if you use a deterministic PRNG that is only seeded with 24 bits, then it doesn't actually make any sense to have more than 5 alphabetic characters, since 26^5 > 2^24). Once you seed the PRNG with a reasonable amount of entropy, though, you can justify using more characters (e.g. you need to use 28 alphabetic characters to be able to transmit the full entropy implied by a 128-bit seed to the PRNG).

so, we should pull X bytes from drupal_random_bytes(), then encode that in at least the equivalent number of bits with whatever characters we're allowed to use here (is it just [A-Z]?). now we just need to decide what X should be.

and of course all of this means that drupal_random_bytes() will have to be backported to the D6 version of this module as _openid_provider_drupal_random_bytes(), but that looks easy.

(note: i sent a message about this to the security team, and they advised me to take it to the public issue queue.)

Are you sure the nonce needs to be cryptographically secure? http://openid.net/specs/openid-authentication-2_0.html#rfc.section.10.1 suggests it only needs to be unique, so they could even be sequential if we wanted (though hitting the database might be slower than a random solution).

BTW, this is a birthday problem, the chance of a collision in your example is approximately 66%.

the spec requires us to assure that there will be no collisions. so, we either need to check the database and be certain that there are no race conditions possible or make the nonce cryptographically secure. as i was trying to say above, drupal takes the first approach by using the db in openid_verify_assertion_nonce(), but i believe that in some cases with mysql clustering that we may not be able to guarantee that this will work. so, we need to guarantee the second with a stronger nonce.

thanks for reminding me of the formula for calculating those odds, it's been a while since my math days :)

I know of MySQL specific ways to generate sequential identifiers, but those of course are out unless the D7 database abstraction layer includes a transaction-safe way to do that. I'm guessing it doesn't (see note in comment #3 above about the inconsistency of MySQL's COUNT() when using cluster replication). In general the second approach will mean trusting every possible database configuration this module could be running on top of, which seems far harder to guarantee than the first approach. So, I think we should use drupal_random_bytes() in D7. We can easily backport that function to D6 to fix the problem there.

looks good here. i think we ought to suggest this for D8 core.

i didn't know about _openid_get_bytes(). that sounds like a good solution for D6.