Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5543 closed defect (fixed)

BridgePassword would be insecure if anybody used it

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The "BridgePassword" option, which a bridge authority can use to provide debugging information, has a stupid side-channel bug: it uses strcmp() to compare the received authenticator with the expected value.

Fortunately, the bridge authority isn't (and hasn't been) using this option, so there is no actual target for the side-channel attack now. (Also, it's pretty hard to get fine-grained timing information out of a loaded Tor server. But we shouldn't count on that.)

The right short-term fix is probably to hash the BridgePassword when it is set, and then to hash any any provided authenticator and compare it against the hashed value.

The right longer-term fix is to replace BridgePassword with something akin to HashedControlPassword, or to remove it entirely.

Child Tickets

Change History (7)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

Please review branch "bridgepassword" on 0.2.2.x in my public repository.

For fun, you can also see branch "di_strcmp" in my public repository: that's how you do a one-sided-data-independent strcmp, I think. But the approach in "bridgepassword" is more solid, I think.

comment:2 in reply to:  1 ; Changed 8 years ago by rransom

Status: needs_reviewneeds_revision

Replying to nickm:

Please review branch "bridgepassword" on 0.2.2.x in my public repository.

base64_encode is probably not protected against side-channel leaks. I don't know whether that's a problem; leaks there can only be exploited by observing the bridge authority while someone who knows BridgePassword fetches the consensus from it.

If alloc_http_authenticator fails, BridgePassword_AuthDigest is silently not set. That would be a royal PITA to debug if it could ever happen.

Storing BridgePassword as a digest isn't what prevents timing attacks, it's what allows you to use a timing-attack-resistant comparison function with it. (That's quite a subtle distinction, but still important enough to justify correcting the comment.)

Other than that, looks good.

For fun, you can also see branch "di_strcmp" in my public repository: that's how you do a one-sided-data-independent strcmp, I think. But the approach in "bridgepassword" is more solid, I think.

di_strcmp is broken: it uses secret information (the length of target) to determine what memory location (ba) to read from.

comment:3 in reply to:  2 ; Changed 8 years ago by nickm

Status: needs_revisionneeds_review

Replying to rransom:

Replying to nickm:

Please review branch "bridgepassword" on 0.2.2.x in my public repository.

base64_encode is probably not protected against side-channel leaks. I don't know whether that's a problem; leaks there can only be exploited by observing the bridge authority while someone who knows BridgePassword fetches the consensus from it.

I'm missing something there. I thought we no longer called base64_encode in response to incoming authenticators. At least, I hope we don't?

If alloc_http_authenticator fails, BridgePassword_AuthDigest is silently not set. That would be a royal PITA to debug if it could ever happen.

Ick, yeah. Better fix that.

Storing BridgePassword as a digest isn't what prevents timing attacks, it's what allows you to use a timing-attack-resistant comparison function with it. (That's quite a subtle distinction, but still important enough to justify correcting the comment.)

There too. Please see branch now?

Other than that, looks good.

For fun, you can also see branch "di_strcmp" in my public repository: that's how you do a one-sided-data-independent strcmp, I think. But the approach in "bridgepassword" is more solid, I think.

di_strcmp is broken: it uses secret information (the length of target) to determine what memory location (ba) to read from.

Argh, you're right. Perhaps if seymour cray rises from the grave and abolishes all cache everywhere, it will be a good idea. Force-pushed a version with an updated commit message to indicate that it is broken.

comment:4 in reply to:  3 Changed 8 years ago by rransom

Replying to nickm:

Replying to rransom:

Replying to nickm:

Please review branch "bridgepassword" on 0.2.2.x in my public repository.

base64_encode is probably not protected against side-channel leaks. I don't know whether that's a problem; leaks there can only be exploited by observing the bridge authority while someone who knows BridgePassword fetches the consensus from it.

I'm missing something there. I thought we no longer called base64_encode in response to incoming authenticators. At least, I hope we don't?

You're right -- I misread the diff. (gitk's ‘New version’ display mode is great; I should have started using it sooner.)

If alloc_http_authenticator fails, BridgePassword_AuthDigest is silently not set. That would be a royal PITA to debug if it could ever happen.

Ick, yeah. Better fix that.

Storing BridgePassword as a digest isn't what prevents timing attacks, it's what allows you to use a timing-attack-resistant comparison function with it. (That's quite a subtle distinction, but still important enough to justify correcting the comment.)

There too. Please see branch now?

Looks good!

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks for the speedy review. merged this into maint-0.2.2 and master.

comment:6 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:7 Changed 7 years ago by nickm

Component: Tor Directory AuthorityTor
Note: See TracTickets for help on using tickets.