search

google / end-to-end

End-To-End is a crypto library to encrypt, decrypt, digital sign, and verify signed messages (implementing OpenPGP)
Apache License 2.0
4.13k stars 298 forks source link

Change wording on import dialog to stress the fact that the user must verify the fingerprints of the keys and subkeys being imported. #65

Closed koto closed 9 years ago

koto commented 9 years ago

From evn@google.com on June 05, 2014 22:51:49

Is this report about the crypto library or the extension?
Extension

What is the security bug?
We should be more explicit when telling the user to verify the fingerprint of keys before importing them.

How would someone exploit it?
Since subkey's fingerprints aren't as easy to verify, some users might not do it, and import a subkey they don't trust (after just verifying the main key).

We could even add some UI to help the user verify them (checkboxes in the confirmation dialog?). Also, we can hide signed subkeys from the list probably.

(this was reported by kbsriram@ - let me know if we can make this bug public, thanks!)

Original issue: http://code.google.com/p/end-to-end/issues/detail?id=28

koto commented 9 years ago

From evn@google.com on June 05, 2014 13:53:11

Hey Radi.

What do you think of the checkboxes idea?

Cc: r...@google.com

koto commented 9 years ago

From evn@google.com on June 05, 2014 13:53:26

(to remind the user to verify every fingerprint)

koto commented 9 years ago

From r...@google.com on June 05, 2014 13:55:18

Certainly doable. I'll take care of it.

koto commented 9 years ago

From r...@google.com on June 05, 2014 13:55:56

Owner: r...@google.com
Cc: -r...@google.com

koto commented 9 years ago

From evn@google.com on June 06, 2014 10:10:26

Hey also, could you do the monospace grouped by 4 styling?

koto commented 9 years ago

From evn@google.com on June 06, 2014 18:59:46

Labels: Usability Type-Defect Priority-High Component-UI Component-Logic

koto commented 9 years ago

From evn@google.com on June 07, 2014 10:22:17

unprivatizing per kb's request.

Labels: -Restrict-View-Commit

koto commented 9 years ago

From kbsriram on June 08, 2014 07:16:00

Just a note/heads-up about the fix - accepting unsigned/invalid signatures on subkeys on public keys would take it out of spec, as it is a MUST requirement for transferable public keys (also personally feel this is the right tradeoff between security and usability.)

I just noticed the comment on transferablekey.js regarding PGP 8.0.3 on with unsigned secret subkeys -- secret keys are permitted to omit signatures, if accompanied by a separate public key with signatures. Section 11.2 has the gory details.

koto commented 9 years ago

From evn@google.com on June 10, 2014 02:47:43

yes you are right:

 - After each Subkey packet, one Signature packet, plus optionally a
   revocation
koto commented 9 years ago

From evn@google.com on June 10, 2014 03:01:18

So, here's the current plan:

  1. Hide every signed subkey from the import list.
  2. Show a checkbox next to every main key.

For secret keys:

  1. We will parse the whole block, then we will try to sign the secret subkeys.
  2. Then we will import the signed subkeys.

Does that sound like a plan? What do you think KB?

koto commented 9 years ago

From kbsriram on June 10, 2014 08:46:27

Re: importing public keys - looks good to me! [I'm assuming it also quietly discards subkeys without a valid signature.]

Re: importing secret keys - if possible, it would be great to find/ask for the associated public key should it encounter unsigned userids/subkeys. Eg: this will let it pick up key-flags (is this subkey meant for certification, signing or encryption) rather than having to guess it. Side-benefit, it can pick up user-id algorithm preferences from the implementation that created these keys, so exporting+publishing this key from e2e won't cause those preferences to change.

koto commented 9 years ago

From evn@google.com on June 16, 2014 03:37:48

koto has been working on this

Owner: koto@google.com

koto commented 9 years ago

From evn@google.com on June 16, 2014 03:46:06

(Note that rejecting keys without signatures somewhat conflicts with issue 22 )

koto commented 9 years ago

From kbsriram on June 17, 2014 10:27:21

Issue 22 should happen only on secret keys, is that correct?

At any rate, key and uid signatures can be verified on secret keys too, as the primary public key is present even with the dummy-s2k extension (as are the signatures naturally...) I've added further notes on issue 22 .

koto commented 9 years ago

From evn@google.com on June 17, 2014 14:22:32

ok, we will be stop accepting unsigned public subkeys soon (still working on it).

thanks so much for your help KB, we are considering this for the Vulnerability Reward Program, although it might not qualify for a reward though.. but really, thanks for your help.

koto commented 9 years ago

From evn@google.com on June 21, 2014 10:08:08

Koto finished the first part. I'll add the "reject unsigned subkeys", but radi could you add the check boxes?

Status: Started
Owner: r...@google.com
Cc: koto@google.com

koto commented 9 years ago

From r...@google.com on June 26, 2014 06:24:38

Status: FixedInStaging

koto commented 9 years ago

From r...@google.com on June 26, 2014 06:27:21

Only the mandatory verification of keys is implemented. We still need to have the ability to strip out untrusted subkeys. evn@, could you take a look as this may require some changes in the actual import logic.

Status: Started
Owner: evn@google.com
Cc: r...@google.com

koto commented 9 years ago

From koto@google.com on July 16, 2014 07:32:18

Blocking: end-to-end:30

koto commented 9 years ago

From evn@google.com on August 11, 2014 21:32:11

I'll take a look.

koto commented 9 years ago

From koto@google.com on August 11, 2014 22:07:52

FWIW, sig verification on subkeys is now done. We can remove the fingerprint and a relevant checkbox from the confirmation dialog (I just need to propagate sig info to Context and then extension).

koto commented 9 years ago

From evn@google.com on August 11, 2014 22:14:03

What's missing then?

koto commented 9 years ago

From evn@google.com on August 11, 2014 22:14:32

Owner: koto@google.com

koto commented 9 years ago

From evn@google.com on August 11, 2014 22:15:08

Oh you said so. Sounds good. Assign back if I should work on it.

koto commented 9 years ago

From kristian...@sumptuouscapital.com on August 12, 2014 00:34:52

Just to ensure that it has been mentioned is this thread. For signing subkeys, it is important to also verify the subkey binding signature, otherwise a malicious user can attach another user's signature key to his own public key, possibly tricking a user into believing this key is the proper one. Some further details are available on https://www.gnupg.org/faq/subkey-cross-certify.html

koto commented 9 years ago

From evn@google.com on August 24, 2014 23:21:15

Koto just sent a CL for preventing this I think.

Thanks!!

koto commented 9 years ago

From koto@google.com on August 25, 2014 04:37:27

Subkey cross-certification is taken care of. Now we also verify subkey and userid/attribute signatures. We also process revocation signatures. Unsigned subkeys/user ids are invalid and are stripped before import. Keys without user ids are rejected.

koto commented 9 years ago

From mwf...@gmail.com on August 27, 2014 12:48:56

I think we already have some plans to discuss, but a SafeSlinger-like protocol could prevent habituation of click-through to determine if any 2 or more users actually physically verified public keys or not.

koto commented 9 years ago

From kbsriram on August 28, 2014 18:37:55

Just a quick heads-up; the signature verification code needs a check for any expiration timestamps within hashed subpackets to avoid (eg:) using expired uids/subkeys.

For convenience, attached a few test-cases (from a slightly bigger set of tests at https://github.com/kbsriram/openpgp-tests should you find anything else there handy.) The pk-expired-uid and the pk-expired-subkey tests are the ones that are not yet passing at commit-id: e193633.

Thanks!

Attachment: kbs_test.html

koto commented 9 years ago

From koto@google.com on September 03, 2014 05:24:11

Blocking: -end-to-end:30

koto commented 9 years ago

From evn@google.com on October 13, 2014 02:43:43

hey KB, have you ran your compat suite with e2e? how did we do?

koto commented 9 years ago

From kbsriram on October 24, 2014 09:35:12

I just reran some of the tests with the source from commit-id e193633, and summary is:

  1. handling unsigned uid, unsigned subkeys, expired uid, expired subkeys looks good.
  2. checking for cross-certification for signing subkeys is working, though invalid signatures are currently failing the entire import by throwing an exception. It might be better if they were ignored (and treated as though that signature was missing) rather than failing the import of the entire certificate. Similar comment for revocation signatures.
  3. A couple of test keys that contain buggy RSA parameters (eg: e=1, or a modulus factorable by a small prime) were accepted. These are rare in practice (and seem to come from buggy implementations) so maybe it'd be nice to add a few basic sanity checks without getting too elaborate.

There are few more tests related to designated key revokers, and misused subkeys (eg: getting something encrypted with a signing subkey) that I haven't tried yet.

sirdarckcat commented 9 years ago

KB, can we close this?

koto commented 9 years ago

@kbsriram, ping on this. My understanding is that all self-signatures are checked, so it's safe only to show the main key to the user (and only main key fingerprint). Right?

kbsriram commented 9 years ago

Yes - should be safe to just display the main key fingerprint to the user.