1695590 - openpgp alias: prevent sending if alias file was malformed, don't send unencrypted

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Magnus Melin [:mkmelin]]

If the alias file is malformed (or other problems with accessing it), we currently send unencrypted, which we shouldn't. This is due to messy code...

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1695590_alias_parsing_fail.patch

We of course need to get rid of the capi.sync... but for now we can paper over the problem a bit.

When the promise rejects with an Error, that needs to be propagated up. Due to how XPCOM treats the object it's not even instanceof Error at the receiving end anymore...

At the moment, gotResult is an object. We could potentially return null also, but actually throwing is what it would look like when we get rid of the capi.sync() calls later, so maybe better to just do it right away. With the throwing the error is also not swallowed, but you get to see the error message in the error console.

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

Review of attachment 9206024 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/content/modules/cryptoAPI/interface.js
@@ +43,5 @@
>          inspector.exitNestedEventLoop();
>        })
>        .catch(gotResult => {
>          console.log("CryptoAPI.sync() failed result: %o", gotResult);
> +        if (gotResult instanceof Error) {

Can we be certain  that any related code will always throw type Error() ?
If we aren't certain, it might be better to throw any error.

As a test, I added ```throw "error";``` in enigmailMsgComposeOverlay.js determineSendFlags() and a message was sent unencrypted.

Comment 3

[Magnus Melin [:mkmelin]]

Syntax errors and IO errors will be "the right type". It's generally a bad idea to throw strings, but it can technically occur.
In general, it should throw for all errors here, but I wanted to be conservative since the api is used elsewhere too and it's hard to reason about all the call sites. If it's not an error that's thrown, we also handle a falsy return value ok for the send case.

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

Thanks for the explanation.

I'm OK with this, r=kaie, but as I understand it, it won't catch any kind of potential exception, so it seems we should still implement an additional fix for bug 1648019.

(You say it catches other types if they are falsy - but a string isn't falsy.)

Comment 5

[Kai Engert [:KaiE:]]

Looking more deeply and after some experiments - in the third chunk of your patch (sendMessageListener) you're handling exceptions of any type. With that, most exceptions should cause sending to abort.

But in CryptoAPI.sync you limit it to exceptions instances of type Error. As a result, exceptions in code that is called by CryptoAPI.sync, which throws an exception of an unexpected type, will not cause sending to abort. I think that's inconsistent.

You say the CryptoAPI.sync API is used elsewhere, and you want to be conservative.

However, your patch already changes the behavior of the API. Before your patch, it never throws. Now, it throws in some scenarios. I think by allowing the function to throw, you've no longer conservative. It seems to me that throwing for all sorts of exceptions isn't much worse.

Comment 6

[Kai Engert [:KaiE:]]

I think we shouldn't delay this patch, because it fixes the known issue. Follow up fixes can be added in bug 1648019.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7ce196669f1c
openpgp alias: prevent sending if alias file was malformed, don't send unencrypted. r=kaie

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: bug1695590_alias_send_testfail.patch

This is causing a test failure for testEncryptedOneRecipientKeyNotAvailableMessageComposition. But I think the explanation is easy: no key -> can't get key block from empty string -> exception (which was earlier just swallowed)

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 9206629 [details] [diff] [review]
bug1695590_alias_send_testfail.patch

Function commonProcessAttachedKey already returns without data, if no list could be extracted from the parameter, so this change should be safe.

r=kaie

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1d767781ecba
don't try to process empty key data; that will (now) throw and prevent normal functionality. r=kaie DONTBUILD

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

[Approval Request Comment]
Regression caused by (bug #): 1644085
User impact if declined: risk of accidentally sending wihtout encryption
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 9206629 [details] [diff] [review]
bug1695590_alias_send_testfail.patch

[Approval Request Comment]
Regression caused by (bug #): this bug
User impact if declined: none, only affects tests
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

this will go in the next beta

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9206629 [details] [diff] [review]
bug1695590_alias_send_testfail.patch

this will go in the next beta

Comment 15

[Kai Engert [:KaiE:]]

Thanks for the beta approval. That's actually what I had intended. Let's postpone the esr78 request for a bit.

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9206629 [details] [diff] [review]
bug1695590_alias_send_testfail.patch

[Triage Comment]
Approved for beta

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

[Triage Comment]
Approved for beta

Comment 18

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-beta/rev/6347a22cbf3d78edeaed37697c085feb9ab72f81
https://hg.mozilla.org/releases/comm-beta/rev/0fbe41e5cf6d746fb156181bf88b96c62bcd5dd4

Comment 19

[Kai Engert [:KaiE:]]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

[Approval Request Comment]
Regression caused by (bug #): 1644085
User impact if declined: required follow-up fix for bug 1644085
Testing completed (on c-c, etc.): c-c, beta
Risk to taking this patch (and alternatives if risky): low

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9206629 [details] [diff] [review]
bug1695590_alias_send_testfail.patch

[Triage Comment]
Approved for esr78

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 9206024 [details] [diff] [review]
bug1695590_alias_parsing_fail.patch

[Triage Comment]
Approved for esr78

Comment 22

[Kai Engert [:KaiE:]]

78.9.1
https://hg.mozilla.org/releases/comm-esr78/rev/5721bd6b15585c53c37710565a9ff8ef32eadeca
https://hg.mozilla.org/releases/comm-esr78/rev/ffaf6912f34232a382f4f3b0781b8134c2677c4a