Mitigate Timing Attacks On Basic RPC Authorization #2845

As per #2838 . Eliminates the possibility of timing attacks by changing the way the two passwords are compared. It iterates through each char in the strings, and if the two chars it is comparing aren't the same, then it adds 1 to nReturn and the function, once it's done comparing all the chars, will return false. Previously, the function would return false on the first char that didn't match, allowing a possible attacker to run a timing attack.

See http://rdist.root.org/2010/01/07/timing-independent-array-comparison/ for reference.

Can you also make the short password delay into an unconditional delay on failure?

Your current code timing-leaks the length, but I don't know if I care. You could avoid this by only comparing the input number of characters every single time, with a min() on the offset (take care to avoid a fence post error) on the actual password. Then compare the lengths.

Woops, didn't mean to close it. And I was thinking along the same lines, but with something a little simpler. If the lengths don't match, you can just compare the actual password with itself (to get the timing right), but add 1 to nReturn also so it returns false.

The only reservation I have with that is that, depending on how you write it, it is code that the optimizer is very likely to optimize out. E.g. if it were if(len1==len2){} else {selfcompare} that will quite probably get optimized.

I updated it to fix the length-leaking, seems like the simplest way to do it.

@grayleonard The extra loop with the "return ++nResult == 0;" is ... a little perplexing. The % that sipa proposed (or the min) should actually result in simpler looking code. Just move the length check to the end, and use the % to make sure that both only access valid indexes. Care to give it a shot?

There's no telling what compilers will do to this. If you're that concerned about timing attacks, why not just do something like this pseudocode?

const minwait = 50; //msec
timeBefore = GetCurrentTime();
result = val1 == val2;
sleep(minwait - (GetCurrentTime() - timeBefore));
return result;

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bd8420dda743c36940d3986fb7e81a2f195495f8 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Jeff argues that we should probably just drop basic auth entirely and require digest auth, which would solve this as a side effect.

@theuni we're deep in pedantry land, but expecting very high accuracy sleeps results in sadness. It's perfectly possible to leak data through a sleep like that.

@gmaxwell fair point on pedantry, but if you're on a system with <50msec sleep precision, i'd guess you'd have bigger concerns. In this case, it'd likely even spoil the very attack the evildoer is attempting.

Also -1 on convoluted schemes that the compiler might try to micro-optimize, or might impact the authentication result.

Heck, even unconditionally sleeping for a random interval would be better. Remove the 'if' check on password size.

But yes, approaching 1.0 it is reasonable just to require Digest auth.

@theuni From what I've read it seems like there are two ways to mitigate timing attacks - one can hold all responses with a constant delay, say 50ms. The other returns faster, I've seen ~10ms in this instance from tests I've run, with a constant-time comparison algorithm. Either one works.

I'm not sure how soon 1.0 is going to be available, but if we are treating this as a vulnerability instead of a bug it's important we get it out as soon as possible, regardless of the technique we use.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/351229cfc0d0f6c54af8e4d7ac7426c16e776fb1 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Superceded by #2886