1663925 - Non-Trivial to Accept New User IDs

Status: NEW

(MailNews Core :: Security: OpenPGP, defect, P3)

Description

[neal]

Attachment: alice.pgp

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

• I imported the key alice.pgp using Tools -> OpenPGP Key manager -> Import Public Keys from File
• I accepted the key ("Yes, I've verified in person this key has the correct fingerprint.")
• I imported the key mallory.pgp, which adds a new User ID to the same key
• The key is still considered accepted, but when I try to send a mail to [e-mail], I get an error that the key is not accepted.

Expected results:

First, the OpenPGP code correctly records whether a binding (User ID + Key) is accepted not just a key. The UX, however, is not clear about this. Further, it should let the user confirm individual email addresses.

Second, when a new User ID is added to an existing OpenPGP certificate, there is no easy way to accept the new User ID. My workaround is to reject the key ("No, reject this key") and then immediately change the acceptance back to "Yes, I've verified in person...". Then I can send an encrypted email to the new User ID.

Comment 1

[neal]

Attachment: mallory.pgp

Comment 2

[neal]

I've spent some time reading openpgp/content/modules/sqliteDb.jsm. Unfortunately, I think the DB schema does not correctly model what should be modeled.

The relevant tables are created at around line 374 of the aforementioned file:

await connection.execute(
"create table acceptance_email (" +
"fpr text not null, " +
"email text not null, " +
"unique(fpr, email));"
);

await connection.execute(
"create table acceptance_decision (" +
"fpr text not null, " +
"decision text not null, " +
"unique(fpr));"
);

We have a table with one row per fingerprint and email address, and another row with a fingerprint and a decision. That is, the identities associated with a given key have the same decision (unless the identity is not listed in which case it has no decision). This is unusual. If you consider X.509 or the WoT, it is possible to certify a subset of bindings. So, if we have a certificate like above, it is possible to certify "[e-mail]" and not certify "[e-mail]".

Independent of how TB displays this choice to the user (perhaps the UX will only allow accepting all identities or rejecting all identities), I think the DB should store decisions on a per binding basis. This can be done by using just a single table with three columns: fingerprint, email, decision.

By modeling it this way, we also avoid the temptation of asking: "has this key be accepted", which is non-sense, when we mean: "has this identity been accepted for this/a key".

Comment 3

[Kai Engert [:KaiE:]]

I'd like to hide the complexity of individual acceptance decisions per user ID.

The current model allows us to store one decision per key, and which email addresses this decision covers.
Non-included emails are treated as "undecided".

Yes the current UI has a limitation, as stated in the bug description "non-trivial to accept new user IDs".

I think it might be sufficient to have a UI that highlights new user IDs that haven't been accepted yet, and ask if the user is willing to accept them, too (or keep them excluded). We'll have to discuss this with our UI experts once we're ready to work on it.

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1663925 - Use IDB_TRY* in CreateStorageConnection. r=#dom-workers-and-storage

Depends on D89897

Comment 5

[Phabricator Automation]

Comment on attachment 9175173 [details]
Bug 1663925 - Use IDB_TRY* in CreateStorageConnection. r=#dom-workers-and-storage

Revision D89905 was moved to bug 1663924. Setting attachment 9175173 [details] to obsolete.

Comment 6

[neal]

Hi, I reported this 90 days ago. I plan to publish a report in the next few weeks. Do you need additional time?

Comment 7

[neal]

(In reply to neal from comment #6)

Hi, I reported this 90 days ago. I plan to publish a report in the next few weeks. Do you need additional time?

Sorry, I thought this was a protected security issue. Please ignore.