Skip to content

Mitigate Timing Attacks On Basic RPC Authorization #2845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

grayleonard
Copy link

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.

As per bitcoin#2838 . Eliminates the possibility of timing attacks by changing the way the two passwords are compared. See http://rdist.root.org/2010/01/07/timing-independent-array-comparison/ for reference.

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 returns false. Previously, the function would return false on the first char that didn't match, allowing a possible attacker to run a timing attack.
@gmaxwell
Copy link
Contributor

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.

@grayleonard grayleonard reopened this Jul 23, 2013
@grayleonard
Copy link
Author

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.

@gmaxwell
Copy link
Contributor

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.

return strUserPass == strRPCUserColonPass;

//Begin constant-time comparison
if (strUserPass.length() != strRPCUserColonPass.length())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means (theoretically) exposing the password length. If you go as far as making the comparison constant-time, maybe go all the way? Something like comparing x.at(i % x.size) ^ y.at(i % y.size). (not a perfect solution, as it will match in case one is a repetition of the other).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The % test (I would have just min()) is fine if you just add a |= len==len at the end.

Fix to the timing-leak for the length of the password.
@grayleonard
Copy link
Author

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

@gmaxwell
Copy link
Contributor

@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?

@theuni
Copy link
Member

theuni commented Jul 23, 2013

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;

@BitcoinPullTester
Copy link

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.

@gmaxwell
Copy link
Contributor

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

@gmaxwell
Copy link
Contributor

@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.

@theuni
Copy link
Member

theuni commented Jul 23, 2013

@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.

@jgarzik
Copy link
Contributor

jgarzik commented Jul 23, 2013

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.

@grayleonard
Copy link
Author

@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.

Simplified time-leaking solution, using min() to make sure only valid indexes are called.
@BitcoinPullTester
Copy link

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.

return strUserPass == strRPCUserColonPass;

//Begin constant-time comparison
//XOR chars in each password together, and then adds either 0 or 1 (0 if they match) to nResult
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment incorrect now? Adds 1 if they do not match, does not add 0 if they do.
Maybe:
nResult += (length == length ? 1 : 2)
... and
return nResult == 1;
... to get really-and-truly-ought-to-be-constant-time...

Also, commits need to be squashed into one.

Also style nitpick, space after for/if: for (size_t ...) and if (strUser...)

@sipa
Copy link
Member

sipa commented Aug 15, 2013

Superceded by #2886

@sipa sipa closed this Aug 15, 2013
IntegralTeam pushed a commit to IntegralTeam/bitcoin that referenced this pull request Jun 4, 2019
* Fix crash in RemoveInvalidVotes()

Caused by lock not being held while accessing lastMNListForVotingKeys when RemoveInvalidVotes() is called from DoMaintenance() and UpdatedBlockTip() at the same time.

* No need to call RemoveInvalidVotes() from DoMaintenance()

MN list only changes on new blocks and we already call RemoveInvalidVotes() from UpdatedBlockTip()

* No need to call RemoveInvalidVotes() until we are fully synced
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants