Automatically refresh GPG keyring on startup and on signature failures. #272

dephekt · 2017-04-13T22:53:13Z

This PR attempts to automatically resolve situations where Tor Browser signing keys are changed upstream, causing signature verification failures:

Keyring is refreshed on every app start, right after keyfiles are imported.
Keyring is refreshed whenever a signature verification failure occurs.
Refresh uses HKPS and a locally provided CA file for verifying the SSL connection.

The user will still receive the verification failure dialog warning, but the key refresh will have happened then in the background, so clicking "Start Over" on the warning results in a successful verification on the next run. This should be a rare or impossible condition since we already refresh the keyring on startup now, but I supposed it wouldn't hurt.

New Packages Requirements:

Ubuntu/Debian: gnupg-curl is required for HKPS fetching to work in GPG during key refresh operations.

Fixes:

PR aims to resolve #271 and future issues related to #260, #261, and #263.

…verify failures.

cfcs · 2017-04-21T13:14:21Z

If the keys are changed upstream, how do we establish trust?
I can follow the --refresh-keys, that makes sense, but to me the --recv-keys for a potentially attacker-provided key ID seems a bit fishy. Can you elaborate a bit on how that works?

dephekt · 2017-04-21T20:09:09Z

@cfcs That is a good point that I never considered: I was assuming trust on the incoming key-id. I'll reconsider how I handle that situation... or we could drop the --recv-keys entirely and leave it as a failure scenario.

My initial thought was: the Tor Project recommends the --recv-keys step against sks-keyservers to retrieve their keys, so if we are missing a key we should do that, but I didn't consider a scenario with a malicious fingerprint from the start, wherein we then have a serious security flaw.

If there was a dedicated, secure source to always obtain signing keys for Tor, that's what we should be talking to for --recv-keys; like if they had their own WKP/HKP server or even all their public keys hosted in a single HTTPS location.

Is running --refresh-keys only good enough?

@micahflee what are your thoughts on this? This would only auto-correct for new sub-keys; still throw errors for entirely new keys, though perhaps that should be our intention. See next commit. Still logic there to later add support for getting new keys from a secure source if one is made available.

cfcs · 2017-04-22T09:04:41Z

torbrowser_launcher/common.py

@@ -205,6 +205,17 @@ def init_gnupg(self):
            self.mkdir(self.paths['gnupg_homedir'])
        self.import_keys()

+    def refresh_keyring(self, fingerprint=None):
+        p = subprocess.Popen(['/usr/bin/gpg', '--status-fd', '2',


Shouldn't we wait for this to terminate so that we can check the result?

If the key doesn't get refreshed you will just get a verification error again after trying again, so I didn't see a need to actually check the results because I'm not sure what else we would do after, if the refresh failed.

I could have it spit out a message/dialog saying the refresh failed or that it worked, but you already find out when you click "Start Over" on the error prompt and it tries over from scratch (after trying a refresh again).

If you have an idea for what more to do in the event a refresh fails, I'm happy to go back and parse the output of the refresh.

You make a good point, I agree that the error handling at this point could get a bit icky.
If you agree with the point about detecting revoked keys, checking the result matters more.
One could also argue that "this is definitely not signed by something we know" is a nicer error message than "maybe you were mitm'ed, but maybe we just failed to contact the key server because the network is shit".

If the user in question has the Python GPGME bindings installed, they will get an error message specifically mentioning the fingerprint of the missing key that resulted in the failure. Now we also spit GPG refresh output to the terminal when ran from the command line:

me@me-OEM:~$ ./torbrowser-launcher Tor Browser Launcher By Micah Lee, licensed under MIT version 0.2.4 https://github.com/micahflee/torbrowser-launcher Refreshing local keyring... Keyring refreshed successfully... New signatures for key: EF6E286DDA85EA2A4BA7DE684E2C6E8793298290 Launching './Browser/start-tor-browser --detach'...

cfcs · 2017-04-22T10:29:34Z

@dephekt Oh, ok! I think your changed patch looks sensible! (but I think it should be doing a forced --refresh-keys regardless of whether the current sig checks out or not)

Here's my take on it:

The Tor GPG key should only be replaced in two cases:
- If the keyholder loses the ability to sign releases with the key (forgets passphrase etc),
- or if the key is compromised. If this happens, Tor should revoke the key and upload the revocation signature to the keyservers. --refresh-keys will alert us to this fact (provided the key servers play nice) and invalidate the key, keeping our users safe, so we should always do that.
- In the event of expiry of the main signing key (currently set to 2020), Tor can renew the expiration date. --refresh-keys will also be useful in this scenario.
I do not think we should have ultimate trust in the X.509 certificate used for https://torproject.org:443. It's an always-online key, which makes it more vulnerable to attacks than an offline signing key. Furthermore there are hundreds of CAs that must be assumed to be under control by "democratically challenged" governments, so exposed Tor users cannot rely on the safety of the HTTPS system unless pinning specific keys (which would introduce more failure modes).
- If we do not already, we could consider pinning the issuer of the X.509 certificate to limit exposure a bit.
It is my perception that Tor recommends --recv-keys for bootstrapping the trust in a trust-on-first-use scenario simply because it is better than nothing. I do not think it should be automated as it is in essence a leap of faith.

My conclusion:
Make torbrowser-launcher execute --refresh-keys on every launch, not just when the sig verification fails.

I skimmed through the GPG man page (which is kind of long), and picked out the following options that we may want to pass: --no-use-agent --exit-on-status-write-error --keyserver-options include-revoked,no-honor-keyserver-url,no-honor-pka-record,ca-cert-file=/PATH/TO/sks-keyservers.netCA.pem:

--no-use-agent: Don't use GPG agent stuff
--exit-on-status-write-error: Sounds like this should always be set.
--keyserver-options
- include-revoked: Get revoked keys, in case it has been revoked
- ca-cert-file: Provide a custom list of CAs (well, one CA) so we limit exposure a bit. More information on how to do this with SKS-keyservers here: https://sks-keyservers.net/overview-of-pools.php#pool_hkps
- no-honor-keyserver-url,no-honor-pka-record: Don't use potentially unsafe values (these default to YES in my gpg install)

I'd be happy to discuss my points further if you disagree or have questions, or if I overlooked something :-) Thanks for working on this!

dephekt · 2017-04-22T13:22:50Z

(but I think it should be doing a forced --refresh-keys regardless of whether the current sig checks out or not)

Agreed: The PR issues a call to refresh_keys upon every start, before a verify is ever attempted.

Everything you said about distrusting HTTPS

Agree. It's my opinion that missing, entirely new keys should trigger the failure dialog and require user-intervention/reporting here; which as-is should be what happens (per the commit after your initial comment).

Make torbrowser-launcher execute --refresh-keys on every launch, not just when the sig verification fails.

This does already happen: self.refresh_keyring is called in Common.import_keys which runs on every start. This felt simpler than writing its own separate task.

Get revoked keys, in case it has been revoked. Provide a custom list of CAs (well, one CA) so we limit exposure a bit.

I will add this to the current PR soon. I will put the CA file in share/torbrowser-launcher/sks-keyservers.netCA.pem. The build process should cause that to install in /usr/share/torbrowser-launcher/sks-keyservers.netCA.pem or wherever the tor-browser-developers.asc file was installed.

 include-revoked
                     When searching for a key with --search-keys, include keys
                     that  are  marked  on the keyserver as revoked.

@cfcs Is include-revoked applicable during a --refresh-keys operation? This reads to me like it's not, but I don't know for sure so I left it in there for now.

no-honor-keyserver-url,no-honor-pka-record

These are also default in my local GPG config (as well as the CA declaration). I will also add these to the arguments.

Thanks for the suggestions and catching the initial trust issue.

dephekt · 2017-04-22T15:25:55Z

@micahflee The commit above introduces a new package dependency for Ubuntu at least. gnupg-curl is required for HKPS fetching to work in GPG during key refresh operations.

(Edited into the original PR post)

dephekt · 2017-04-23T23:47:02Z

The GnuPG output I was printing to the terminal is now parsed and returns nicer log messages:

me@me-OEM:~$ ./torbrowser-launcher 
Tor Browser Launcher
By Micah Lee, licensed under MIT
version 0.2.4
https://github.com/micahflee/torbrowser-launcher
Refreshing local keyring...
Keyring refreshed successfully...
  No key updates for key: EF6E286DDA85EA2A4BA7DE684E2C6E8793298290
Launching './Browser/start-tor-browser --detach'...

me@me-OEM:~$ ./torbrowser-launcher 
Tor Browser Launcher
By Micah Lee, licensed under MIT
version 0.2.4
https://github.com/micahflee/torbrowser-launcher
Refreshing local keyring...
Keyring refreshed successfully...
  New signatures for key: EF6E286DDA85EA2A4BA7DE684E2C6E8793298290
Launching './Browser/start-tor-browser --detach'...

dephekt · 2017-05-10T23:14:21Z

This should be good to go now @micahflee. Let me know if you have any questions.

micahflee · 2017-06-21T00:41:05Z

This looks great, thank you for implementing it! (And sorry about taking so long to review it)

dephekt added 3 commits April 8, 2017 14:05

common.py: Add initial stuff for refresh_keyring method to Common class.

2af60db

launcher.py: Add method calls to common.refresh_keyring on signature …

fd5ea4f

…verify failures.

common.py: Always call refresh_keyring prior to importing keys.

6f77eb1

Only refresh the keyring, don't try to get new keys.

87338d1

cfcs reviewed Apr 22, 2017

View reviewed changes

Use HKPS for GPG key refresh, add sks-keyservers' CA cert.

445834e

Parse output of refresh_keys and display nicer output.

452c998

dephekt mentioned this pull request Apr 24, 2017

On verification error, torbrowser-launcher should try refreshing gpg key #271

Closed

dephekt mentioned this pull request Jun 6, 2017

SIGNATURE VERIFICATION ERROR #275

Closed

cfcs mentioned this pull request Jun 10, 2017

Might want to disable cute network requests and similar grote/qubes-gpg-search-keys#1

Open

micahflee merged commit 452c998 into torproject:master Jun 21, 2017

dephekt mentioned this pull request Aug 27, 2017

Add changelog to GitHub releases #287

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically refresh GPG keyring on startup and on signature failures. #272

Automatically refresh GPG keyring on startup and on signature failures. #272

dephekt commented Apr 13, 2017 •

edited

Loading

cfcs commented Apr 21, 2017

dephekt commented Apr 21, 2017 •

edited

Loading

cfcs Apr 22, 2017

dephekt Apr 22, 2017

cfcs Apr 22, 2017

dephekt Apr 22, 2017 •

edited

Loading

cfcs commented Apr 22, 2017 •

edited

Loading

dephekt commented Apr 22, 2017 •

edited

Loading

dephekt commented Apr 22, 2017 •

edited

Loading

dephekt commented Apr 23, 2017 •

edited

Loading

dephekt commented May 10, 2017

micahflee commented Jun 21, 2017

Automatically refresh GPG keyring on startup and on signature failures. #272

Automatically refresh GPG keyring on startup and on signature failures. #272

Conversation

dephekt commented Apr 13, 2017 • edited Loading

New Packages Requirements:

Fixes:

cfcs commented Apr 21, 2017

dephekt commented Apr 21, 2017 • edited Loading

cfcs Apr 22, 2017

Choose a reason for hiding this comment

dephekt Apr 22, 2017

Choose a reason for hiding this comment

cfcs Apr 22, 2017

Choose a reason for hiding this comment

dephekt Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

cfcs commented Apr 22, 2017 • edited Loading

dephekt commented Apr 22, 2017 • edited Loading

dephekt commented Apr 22, 2017 • edited Loading

dephekt commented Apr 23, 2017 • edited Loading

dephekt commented May 10, 2017

micahflee commented Jun 21, 2017

dephekt commented Apr 13, 2017 •

edited

Loading

dephekt commented Apr 21, 2017 •

edited

Loading

dephekt Apr 22, 2017 •

edited

Loading

cfcs commented Apr 22, 2017 •

edited

Loading

dephekt commented Apr 22, 2017 •

edited

Loading

dephekt commented Apr 22, 2017 •

edited

Loading

dephekt commented Apr 23, 2017 •

edited

Loading