Allow viewing a packet dump if OpenPGP decryption fails
Categories
(MailNews Core :: Security: OpenPGP, enhancement)
Tracking
(thunderbird_esr115 fixed)
Tracking | Status | |
---|---|---|
thunderbird_esr115 | --- | fixed |
People
(Reporter: KaiE, Assigned: KaiE)
References
Details
Attachments
(2 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
17.32 KB,
patch
|
wsmwk
:
approval-comm-esr115+
|
Details | Diff | Splinter Review |
Thunderbird may receive OpenPGP messages that it cannot decrypt,
for example, if a message uses an encryption mechanism that Thunderbird cannot decrypt.
In this scenario, it can be helpful to allow the user to easily view technical structure information of the message.
This can allow self-diagnosing.
The RNP library allows includes a mechanism to output such information, using function rnp_dump_packets_to_output.
I suggest:
- by default, don't show this dump, as it could confuse the user.
- when a pref is enabled, in the message body (which is usually empty), show the packet dump
An example where this can be helpful is bug 1872833.
For such messages, the output would include something like this:
:off 495: packet header 0xd4c013 (tag 20, len 211)
AEAD-encrypted data packet
aead algorithm: 2 (OCB)
(tag 20, AEAD, OCB)
Assignee | ||
Comment 1•8 months ago
|
||
Updated•8 months ago
|
Assignee | ||
Comment 2•8 months ago
|
||
Note I'm also creating this patch to simplify life for myself.
It will allow me to more easily remote debugging, when users complain and ask for help, saving me time.
Comment 3•8 months ago
|
||
This is a good debugging idea, Kai! And i think you're right that it's also not something that we want exposed to regular users by default.
Would it be possible to instead emit the packet dump in some sort of debugging dialog box, instead of placing it in the position of the message body? that would minimize risk that the rnp_dump_packets_to_output
data would be mistaken for the original sender's message itself.
Comment 4•8 months ago
•
|
||
I agree with comment 3. Can't we just log it to the error console? That way it's easier for users to find as well.
Comment 5•8 months ago
|
||
Also, how can I test? Do we have a sample message to one of the example keys?
Assignee | ||
Comment 6•8 months ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #5)
Also, how can I test? Do we have a sample message to one of the example keys?
Assignee | ||
Comment 7•8 months ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #4)
I agree with comment 3. Can't we just log it to the error console? That way it's easier for users to find as well.
The output might be big.
Assignee | ||
Comment 8•8 months ago
|
||
A big message might create a big list of many packets, which would spam the contents of the error console a lot.
Displaying it as a replacement for the message body has the following advantages:
-
the memory that holds the contents of the error console isn't spammed with these dumps
-
by displaying as a replacement message body, the memory for the error dump is temporary, it will be released as soon as the user switches to another message
-
the idea to show it in a debugging box isn't immediately practical, because I don't have such a debugging box right now, and it would also involve the introduction of UI, which is always more difficult to do, because it requires buy-in from the user interface owners, so it has to be discussed, agreed, new strings for UI will have to be worded and localized, so this wouldn't be quick
-
the simplicity of my proposed solution would allow us to immediately add it to the stable version branch of Thunderbird, while anything that requires new UI or localized strings usually has to ride the (long) trains for release in the next major version.
Assignee | ||
Comment 9•8 months ago
|
||
Ok, maybe we could do the following:
When we show a message that cannot be decrypted, we already show an icon of a padlock that is striked out with a bold red line, and by clicking that, the user gets a cliffhanger popup that says the message cannot be decrypted.
Inside that cliffhanger, we could have a button for showing the message dump.
I found that we already have the string "OpenPGP Debug Log" that we could use that string as the button label.
But then I'd still need a place to show the (potentially) large message dump.
Note that everything is possible in theory, but because I'm always under pressure to solve as many issues as possible, I need solutions that can be done quickly.
So I'd prefer to find a way that can allow showing the dump easily.
It would be an idea to show it in the message area then. But that isn't trivial, because I would have to trigger the reload of the message, and internally remember that on the next failure I DO want to show the error dump in the message area.
Another idea is to show it in a popup dialog, but I don't know if we have an existing dialog that could handle a large amount of text. I'd have to search for it, or need advice which existing code to reuse.
A third idea is to dump it to the error console after the user presses the buttons. But that wouldn't be obvious to the user, we'd have to inform the user to go there to find the text, but we don't have a string currently to inform the user.
My point it, if it's too complicated to do, we don't get the log, because there's other things to work on.
Assignee | ||
Comment 10•8 months ago
|
||
(In reply to Kai Engert (:KaiE:) from comment #9)
It would be an idea to show it in the message area then.
Ok, I found out how to do that, without having to reload the message.
Here is my updated suggestion:
- by default, show nothing, as we do today
- do not introduce a new pref, offer this functionality to everyone
- when the user sees the existing "broken padlock" icon, they can click it
- they will get our existing cliffhanger popup, which explains that decryption didn't work
- in that cliffhanger, in the encryption section, we add a button with the label "OpenPGP Debug Log"
- when that button is clicked, we show the "packet dump" in the message area
- to further avoid confusion what this text is about, we also prefix the information shown with the string "OpenPGP Debug Log"
Comment 11•8 months ago
|
||
I think your design would be ok -- it's definitely better to have it behind the cliffhanger as you describe! i'm not sure about whether a prefixed string "OpenPGP Debug Log" will be sufficiently clear to most users, but it might be clear enough to those uses who make their way through the UX to make it happen.
What happens when the user replies to the message when its visible message area contains the packet dump?
If space for display is really the problem, what if you just displayed the last ~20 lines of the log in the error console or a standard modal popup (again, accessible through the cliffhanger)? The main reason a legit message will fail to decrypt at the level we're talking about is because the type of encrypted data packet is unsupported. and for messages with a handful of recipients, it'll be preceded by a handful of PKESK packets.
For a message to a large number of recipients, that means the early PKESK packets will be truncated. for a deliberately abusive/maliciously crafted message, you'll still get to see the trailer. And, someone who wants to send stuff upstream for debugging can still forward the entire message, ciphertext and all.
Comment 12•8 months ago
|
||
JFYI: RNP allows to dump packets in JSON format (which we are not going to change as well as FFI API), so it may be handier to work with it, given that TB is JScript-based. This would allow to limit amount of packet information displayed and so on.
Comment 13•8 months ago
|
||
This is a great idea, Nickolay (and a nice feature of rnp βΊ)!
That seems like the right way to go to me. That would allow Thunderbird to easily manipulate the packet dump before deciding what to display.
Comment 14•8 months ago
|
||
(In reply to Kai Engert (:KaiE:) from comment #7)
The output might be big.
The console will clean up old messages after a cap of 10k. Wouldn't worry about that.
Assignee | ||
Comment 15•8 months ago
|
||
(In reply to Nickolay Olshevsky from comment #12)
JFYI: RNP allows to dump packets in JSON format (which we are not going to change as well as FFI API), so it may be handier to work with it, given that TB is JScript-based. This would allow to limit amount of packet information displayed and so on.
This requires some inspection and decision what to show and what not, and JSON isn't human readable.
Thanks, maybe later.
Assignee | ||
Comment 16•8 months ago
|
||
Assignee | ||
Comment 17•8 months ago
|
||
(In reply to Daniel Kahn Gillmor from comment #11)
What happens when the user replies to the message when its visible message area contains the packet dump?
The packet dump is NOT used/quoted, despite it being shown in the message body.
Comment 18•8 months ago
|
||
(In reply to Kai Engert (:KaiE:) from comment #6)
That's not a pgp/mime message. Thunderbird just shows it as text
Assignee | ||
Comment 19•8 months ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #18)
(In reply to Kai Engert (:KaiE:) from comment #6)
That's not a pgp/mime message. Thunderbird just shows it as text
That's true, but only because it couldn't be decrypted.
If you run the latest patch, you'll get the broken padlock, you click it, and can click the "openpgp debug log".
I've renamed the above file, to use a shorter name and indicate it's inline:
In addition, I've uploaded a PGP/MIME file.
You can find both files at
https://gitlab.com/kaie/openpgp-samples/-/tree/master/messages
Assignee | ||
Updated•8 months ago
|
Comment 20•8 months ago
|
||
Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/46ea1d41873e
Optionally show a packet dump of encrypted OpenPGP messages that cannot be decrypted. r=mkmelin
Assignee | ||
Updated•8 months ago
|
Assignee | ||
Comment 21•7 months ago
|
||
Comment on attachment 9373228 [details] [diff] [review]
1874504-esr115-v1.patch
[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: less debugging assistance
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
Comment 22•7 months ago
|
||
Comment on attachment 9373228 [details] [diff] [review]
1874504-esr115-v1.patch
[Triage Comment]
Approved for esr115
Comment 23•7 months ago
|
||
bugherder uplift |
Thunderbird 115.8.0:
https://hg.mozilla.org/releases/comm-esr115/rev/7b8fc4b9edc8
Description
•