Closed Bug 1734384 Opened 3 years ago Closed 2 years ago

Chat OTR is broken after upgrading Thunderbird from 78 to 91.

Categories

(Chat Core :: Security: OTR, defect)

Product:
Chat Core
Component:
Security: OTR
Version:
Thunderbird 91
Platform:
x86_64
macOS
Type:
defect

Tracking

(thunderbird_esr91+ fixed, thunderbird95 affected, thunderbird96 affected)

Status:
RESOLVED FIXED
Milestone:
97 Branch
Tracking Flags:
Tracking Status
thunderbird_esr91 + fixed
thunderbird95 --- affected
thunderbird96 --- affected

People

(Reporter: lee, Assigned: mkmelin)

References

Details

(Keywords: regression, Whiteboard: [TM:91.5.1?])

Attachments

(2 files)

analysis-1734384.patch
4.73 KB, patch
freaktechnik
: feedback+
Details | Diff | Splinter Review
Bug 1734384 - fix OTR preference reading. r=freaktechnik
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr91+
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:92.0) Gecko/20100101 Firefox/92.0

Steps to reproduce:

Running Thunderbird on a Mac. Thunderbird 78 popped up a notification to restart Thunderbird to get the update. It upgraded to Thunderbird 91. After the upgrade, OTR is broken in chat.

Actual results:

OTR is not working. I get error messages like this:
"xxxxx has requested an Off-the-Record private conversation. However, you do not have a plugin to support that.
See https://otr.cypherpunks.ca/ for more information."

Expected results:

I expected OTR to still work as before.

Component: Untriaged → Security: OTR
OS: Unspecified → macOS
Product: Thunderbird → Chat Core
Hardware: Unspecified → x86_64

Hey Rob -- do you know if there's been any packaging issues with OTR on macOS starting with Thunderbird 91? Did we change anything about how we provide the library?

Flags: needinfo?(rob)

The shared library filename would have changed from libotr.5.dylib to libotr.dylib, and it's now a universal (intel/arm) binary. We do have a very basic xpcshell test that checks that the library can be loaded and otrl_version (or whatever the function is called) returns. That test has been passing in CI.
The jsconsole will have a message saying whether or not the library can be loaded.

Flags: needinfo?(rob)

I'm doing some libotr work, and I seem to have encountered this bug.

I've got 95.0b1 on Linux x64 and 96.0a1 (2021-11-04) on Win10 x64, and when I try to set up the encrypted conversation from either direction I get:

?OTRv2?
xxxxx@irc.libera.chat has requested an Off-the-Record (OTR) encrypted conversation. However, you do not have a plugin to support that. See https://en.wikipedia.org/wiki/Off-the-Record_Messaging for more information.

I tried using the OS's libotr.so from the Linux box, and got the same result.

I ran mozregression on Thunderbird Daily from 78.0a1 - 91.0a1. I started with a Thunderbird 78 profile, with an IRC connection configured and OTR set up to securely communicate with another IRC user running Hexchat with the Hexchat OTR plugin. Mozregression made a fresh clone of that profile for each build tested.

The failing build is: https://treeherder.mozilla.org/jobs?repo=comm-central&revision=302172a9e62fa393bbe26479749aa3168022ccf3

That particular push is mostly for bug 1682939 (replace idl nsISimpleEnumerator usage with Array<T> in chat/), which is a whole mess of fun changes to chat.

Status: UNCONFIRMED → NEW
status-thunderbird95: --- → affected
status-thunderbird_esr91: --- → affected
Ever confirmed: true

From what I can tell none of the changes affect functionality of OTR (it is still aware of the buddy, the conversations, the messages etc.). I've tried looking through the matching m-c log and can't see anything obvious either. However I'm also not quite sure where exactly the OTR code is supposed to handle the starting handshake.

(In reply to Martin Giger [:freaktechnik] from comment #5)

From what I can tell none of the changes affect functionality of OTR (it is still aware of the buddy, the conversations, the messages etc.). I've tried looking through the matching m-c log and can't see anything obvious either.

However I'm also not quite sure where exactly the OTR code is supposed to handle the starting handshake.

I think it is handled on the onSend code in otr.jsm: https://searchfox.org/comm-central/rev/cb443007f4887cf7f1d36887121c3050363c9070/chat/modules/OTR.jsm#1227-1304 (but this code is all very confusing...)

86.0b1 still worked.
87.0b1 is broken.

I think this means that changes that introduced the regression were made between 2020-12-14 and 2021-01-25.

Not many changes were made to comm/chat in the regression range, bug 1682917 and bug 1634341.

FYI, I found the cause. I'll soon provide additional details.

The reason why OTR is completely disabled is in OTR.policy_cb.

Because we fail to obtain a prefBranch, we return value zero to the OTR library. That says we don't want to use OTR at all, which causes all further piping of messages into the OTR library to skip OTR processing.

See the patch comments. I guess that in the past, setting the attribute on the imAccount worked, and allowed the policy_cb to see it. I guess in the general chat code there were changes in what account objects are being passed around. I added a workaround to construct the prefBranch from the attributes that are available.

We have additional regressions. As noted in the patch, we see null values in various places (and I guess those things didn't happen in the past). I assume caused by other changes in the general chat code (outside of OTR).

I've added null checks and skipping execution of impossible commands, but this results in more visible regressions. OTR status text lines are missing (e.g. the information that we have switched conversation to encrypted). Also, there are missing encryption flag icons for outgoing messages, while we are encrypting.

Also the layout of OTR notifications turned ugly.

In 78, we had header line, a body section in the middle, verify button in a bottom line.

Now we have two columns, the header line is gone, the text block is in a very narrow left column with lots of wrapping, and the button is in the right column which is otherwise empty and taking up a lot of free space.

Comment on attachment 9253662 [details] [diff] [review]
analysis-1734384.patch

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

Looks good. I haven't tested any of it yet - and as such don't fully understand the changes in OTRUI.

::: chat/content/conversation-browser.js
@@ +554,5 @@
>  
>        // Right trim before displaying. This removes any OTR related
>        // whitespace when the extension isn't enabled.
> +
> +// aMsg.displayMessage is null !

Indent (other comments seem to have the same issue)

@@ +555,5 @@
>        // Right trim before displaying. This removes any OTR related
>        // whitespace when the extension isn't enabled.
> +
> +// aMsg.displayMessage is null !
> +      let msg = aMsg.displayMessage ? aMsg.displayMessage.trimRight() : "";

suggestion: `aMsg.displayMessage?.trimRight() ?? "";`

::: chat/modules/OTR.jsm
@@ +680,5 @@
>        if (
>          wContext.account == acc.normalizedName &&
>          wContext.protocol == acc.protocol.normalizedName
>        ) {
> +// acc.prefBranch is undefined, or rather, attribute is missing!

It should be available via `wrappedJSObject`. This also explains why it would've broken with the nsEnumerator -> Array<> patch. Array<> enforces the interface of its contents, so you can't read properties that aren't in the interface anymore. Just out of curiosity, is there any reason not to just use the pref modification methods that imAccount exposes?

@@ +698,5 @@
>      let pb = OTR.getPrefBranchFromWrappedContext(wContext);
>      if (!pb) {
> +// no prefBranch means we return 0, which signals "never use OTR"
> +// to the external OTR protocol library !
> +console.log("internal error, this shouldn't have happened, disabling OTR");

Doesn't OTR have some logging construct it uses?
Attachment #9253662 - Flags: feedback+

Please note I didn't attempt to fix the bug, and I'm not asking for review of this patch as is.
I don't think this patch should be landed as is.
I think this bug needs to be assigned to someone who is able to work on a proper fix.

NI myself so i can take a look at the notification layout problem.

Flags: needinfo?(alessandro)

I think this might also break outgoing actions (/me) from being displayed in unencrypted conversations, since OTR appears to be marking them as cancelled.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED

I've filed issues I can still see after Magnus' patch as separate bugs:

See Also: → 1746863
Flags: needinfo?(alessandro)
Blocks: 1746863
See Also: 1746863 →

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3913bcb10f66
fix OTR preference reading. r=freaktechnik

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

I guess the other bugs will each take care of the other brokenness.

For referencing bugs, please just use bug numbers (so bugzilla can autolink)
Bug 1746864, bug 1746863, bug 1746865, bug 1746867.

status-thunderbird96: --- → affected
Target Milestone: --- → 97 Branch

Comment on attachment 9256034 [details]
Bug 1734384 - fix OTR preference reading. r=freaktechnik

[Approval Request Comment]
Regression caused by (bug #): didn't find the regression
User impact if declined: Off-the-record chat encryption is broken.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): it should be fairly low as OTR has been broken on 91 for some time.

Attachment #9256034 - Flags: approval-comm-esr91?
Attachment #9256034 - Flags: approval-comm-beta?
Attachment #9256034 - Flags: approval-comm-esr91?
Attachment #9256034 - Flags: approval-comm-esr91?

Is there a plan for addressing the additional regressions mentioned in comment 11?

(In reply to Kai Engert (:KaiE:) from comment #11)

We have additional regressions. As noted in the patch, we see null values in various places (and I guess those things didn't happen in the past). I assume caused by other changes in the general chat code (outside of OTR).

I've added null checks and skipping execution of impossible commands, but this results in more visible regressions. OTR status text lines are missing (e.g. the information that we have switched conversation to encrypted). Also, there are missing encryption flag icons for outgoing messages, while we are encrypting.

I've applied both this patch, and the patch from bug 1746863, on top of current comm-beta (version 96).
An OTR connection can be established. A received OTR message is shown. Encryption status is shown.
Outgoing OTR messages are received on the remote site.

However, there is still a major bug:
The outgoing OTR message is NOT shown locally in Thunderbird.

Comment on attachment 9256034 [details]
Bug 1734384 - fix OTR preference reading. r=freaktechnik

There are no more betas this cycle, so it will go out in beta 97

Attachment #9256034 - Flags: approval-comm-beta?

I'm testing comm-central.
If OTR is active, and when sending multiple consecutive messages, only the first outgoing message is marked with a padlock. No padlock icon for the consecutive messages.

(In reply to Kai Engert (:KaiE:) from comment #24)

Is there a plan for addressing the additional regressions mentioned in comment 11?

(In reply to Kai Engert (:KaiE:) from comment #11)

We have additional regressions. As noted in the patch, we see null values in various places (and I guess those things didn't happen in the past). I assume caused by other changes in the general chat code (outside of OTR).
I've added null checks and skipping execution of impossible commands, but this results in more visible regressions.

I have not encountered any such issues (and off the top of my head only remember the empty message handling fix, which isn't necessarily OTR specific and just means we don't display empty messages currently). I also didn't see any visible regressions for impossible commands.

OTR status text lines are missing (e.g. the information that we have switched conversation to encrypted).

This was fixed as bug 1746864 (which I'm not currently going to request uplift for, due to answering this in my lunch break)

Also, there are missing encryption flag icons for outgoing messages, while we are encrypting.

Even with your expanded testing comment that's not what I have observed, from what I've seen it is behaving correctly for how the UI displays messages. Either way, if you can see it on m-c, it would probably be most productive to file a bug with a screenshot that details the behavior.

Whiteboard: [TM:91.5.1?]

(In reply to Martin Giger [:freaktechnik] from comment #28)

OTR status text lines are missing (e.g. the information that we have switched conversation to encrypted).

This was fixed as bug 1746864 (which I'm not currently going to request uplift for, due to answering this in my lunch break)

Thanks. I've added a reminder to request uplift in the bug.

Also, there are missing encryption flag icons for outgoing messages, while we are encrypting.

Even with your expanded testing comment that's not what I have observed, from what I've seen it is behaving correctly for how the UI displays messages. Either way, if you can see it on m-c, it would probably be most productive to file a bug with a screenshot that details the behavior.

I've tested again, and I see that we had the same behavior with 78. So this isn't a regression. For consecutive sent or received messages, only the first message in a series gets a lock icon shown.

Comment on attachment 9256034 [details]
Bug 1734384 - fix OTR preference reading. r=freaktechnik

[Triage Comment]
Approved for esr91

Attachment #9256034 - Flags: approval-comm-esr91? → approval-comm-esr91+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: