Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#15750 closed patch (fixed)

IRC ACTIONs skip sending-im-msg signal.

Reported by: xnyhps Owned by: tomkiewicz
Milestone: 2.10.10 Component: IRC
Version: 2.10.7 Keywords: irc otr /me
Cc: kroeckx, T(A)ILS, developers, frozencemetery, Robby

Description (last modified by datallah)

For most protocols /me gets sent raw, but is displayed differently. For IRC, it is also sent differently. Because /me is interpreted as a command, it bypasses all normal message logic, including the "sending-im-msg" signal. This implies that, for instance, OTR does not get a chance to encrypt the message and it is sent in plain.

Because this is undesirable when using OTR, the "sending-im-msg" signal should fire. Attached patch implements this.

Note that it encrypts "\001ACTION " part too, to be compatible with other IRC clients with OTR support. I'm not sure this is desirable for all plugins that look for the "sending-im-msg" signal.

Attachments (5)

otrmefix.patch (895 bytes) - added by xnyhps 4 years ago.
Patch to fix this
otrmefix2.patch (2.5 KB) - added by xnyhps 3 years ago.
Updated patch with a cleaner solution
ss.png (3.7 KB) - added by T(A)ILS developers 3 years ago.
otrmefix3.patch (2.7 KB) - added by xnyhps 3 years ago.
otrmefix4.patch (2.9 KB) - added by xnyhps 3 years ago.
Fixed msg incorrectly being freed, made sure sent-chat-msg is fired correctly and removed some dead code.

Download all attachments as: .zip

Change History (45)

Changed 4 years ago by xnyhps

Patch to fix this

comment:1 Changed 4 years ago by ioerror

I can confirm that this is a bug/data leak that bites Pidgin-OTR users - I've recently been contacted by some users and developers about it.

If this is a fine patch for the Pidgin developers, I believe this is the patch we'd like to see landed for Pidgin-OTR users to have encrypted '/me foo` messages.

comment:2 Changed 4 years ago by kroeckx

ON IRC any CTCP (like /me, ping, version, ...) really should be treated like any other messages. If you have a "PRIVMSG target :message\r\n", the "message" part should always be encrypted. That is everything between the : and the \r\n. A non-encrypted /me looks like "PRIVMSG target :\001ACTION message\001\r\n". If you encrypt it, the "\001ACTION message\001" part should be encrypted.

comment:3 Changed 4 years ago by bleeter

Pasting some comments from Paul Wouters on OTR-dev mail list: http://lists.cypherpunks.ca/pipermail/otr-dev/2013-September/001882.html (Note, I think some of this may be outside the scope of the ticket/problem specifically here, I'm just adding for 'completeness')


/commands on non-irc chats should not be processed by the irc module. This way, we don't ever leak on non-irc channels.

On irc channels, /nonexisting should get mapped to plain "nonexisting", so it will get properly encrypted by otr.

On irc channels, /existing commands should go out as-is. They should not get encrypted or they won't work. The user is expected to realise that. (hey - its an irc user, they know

My use of /me really comes from the MUD adventure days. It would really only suppress the " says:" prefix so you could 'emote', eg "/me smiles".

Paul (if someone could relay this back to the irc plugin maintainer, thanks)

comment:4 Changed 4 years ago by datallah

  • Cc T(A)ILS developers added; T(A)ILS developers removed
  • Description modified (diff)
  • Milestone set to 2.10.8

We should evaluate this for the next release.

comment:5 Changed 4 years ago by DrWhax

IMHO, this should be treated as a security issue, Pidgin-OTR lacked a way to encrypt certain messages without the patch of xnyhps and he helped to mitigate this by adding a patch to Libpurple so OTR irc /me message are now encrypted. By doing that, we can also backport it in debian to stable.

Last edited 4 years ago by DrWhax (previous) (diff)

comment:6 Changed 4 years ago by T(A)ILS developers

After confirming the patch basically works between two clients running the latest Pidgin release, I've done some interoperability and backporting tests. (I'm thinking of getting this fixed in Debian stable and oldstable as well, actually.)

OK = pidgin-otr does not complain about messages being received unencrypted

  1. between two clients running 2.10.7-2 from Debian sid + the patch: OK
  1. between one client running 2.10.7-2 from Debian sid + the patch, and another client running the same version, without the patch: patched->non-patched is OK, non-patched->patched FAIL
  1. between one client running 2.10.7-2 from Debian sid + the patch, and another client running Debian Squeeze's 2.7.3-1+squeeze3 + the patch:
    • Squeeze to sid: OK
    • sid to Squeeze: on the receiver side, ${X}ACTION ${MESSAGE}${Y} is displayed (with no "was not encrypted" warning); $X and $Y being funny squares (placeholders for missing Unicode chars), and $MESSAGE being the message originally passed to /me; interestingly, pasting this line to the entry box on Squeeze, and then sending back, results in a normal looking /me message being displayed on the receiver (sid)
  1. between two clients running Debian Squeeze's 2.7.3-1+squeeze3 + the patch: both ways, ${X}ACTION ${MESSAGE}${Y} is displayed (with no "was not encrypted" warning)
  1. between one client running 2.10.7-2 from Debian sid + the patch, and another client running either Debian Squeeze's 2.7.3-1+squeeze3 without the patch, or Debian Wheezy's 2.10.6-3 without the patch:
    • patched sid to non-patched: ${X}ACTION ${MESSAGE}${Y} like above
    • non-patched Squeeze to patched sid: unencrypted warning
  1. between one client running 2.10.7-2 from Debian sid + the patch, and another client running either Debian Wheezy's 2.10.6-3 with the patch:
    • patched sid to patched Wheezy: ${X}ACTION ${MESSAGE}${Y}
    • patched Wheezy to patched sid: OK

comment:7 Changed 4 years ago by T(A)ILS developers

  • Cc T(A)ILS developers removed

comment:8 Changed 4 years ago by T(A)ILS developers

Another interoperability test, with Pidgin 2.10.7-2 from Debian sid + the patch on one side, and irssi-plugin-otr 1.0.0~alpha2-1 backported for Wheezy:

  • from irssi to Pidgin: error messages in French "OTR: General Error: Vous avez transmi un message crypté illisible" (French being the locale that Pidgin runs, not the locale that irssi runs) displayed in irssi. Locale information leak, hmm :( The message itself is not conveyed, and in Pidgin I can see "Nous avons reçu de $NICKNAME un message crypté illisible." ("received unreadable encrypted message from NICKNAME"). But on next /me messages, it works fine.
  • from Pidgin to irssi: irssi receives "AACTION tests /meA"

So perhaps the fix only works really well between patched Pidgin? Or is irssi-plugin-otr misbehaving itself?

Last edited 4 years ago by T(A)ILS developers (previous) (diff)

comment:9 follow-up: Changed 4 years ago by xnyhps

I'm not sure this patch is the right approach anymore. The message is indeed encrypted as \001ACTION ${MESSAGE}\001, but it might be better to encrypt it as /me ${MESSAGE} instead. There's a number of possible scenarios where one side of the conversation sees something as IRC, but the other doesn't (XMPP transports, bitlbee, etc.) leading to compatibility problems. And by handling IRC differently, it might give some information about what client you are using. I'll update the patch to use this approach in the next couple of days.

comment:10 Changed 4 years ago by kroeckx

So I understand that because IRC is the only thing that doesn't send /me foo, we're going to standardize so that even on IRC we're going to send a literal /me foo (when using OTR)?

Something like bitlbee really is the only good reason I can see why we need to do that, since it can't translated the message between for instance XMPP and IRC since it can't actually see it.

comment:11 Changed 4 years ago by xnyhps

Yes, that's what Ian Goldberg proposed and I agree with it.

comment:12 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by T(A)ILS developers

Replying to xnyhps:

I'll update the patch to use this approach in the next couple of days.

Great! Any news?

Changed 3 years ago by xnyhps

Updated patch with a cleaner solution

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by xnyhps

Replying to T(A)ILS developers:

Replying to xnyhps:

I'll update the patch to use this approach in the next couple of days.

Great! Any news?

Sorry, some other stuff came up. I've attached the updated patch.

I think this is a cleaner solution because it makes the sending-im-msg have the same semantics for all protocols: it is prefixed with /me when it is an action. This makes it possible for plugins to do something to turn it into a non-action: if the result from the signal is a string that doesn't start with /me , it is sent as a normal message, otherwise it is is sent as an action.

This means that Pidgin patched with this patch should always encrypt actions as "/me does something", which means they also show up correctly in Pidgin (without patching Pidgin-OTR).

comment:14 in reply to: ↑ 13 Changed 3 years ago by T(A)ILS developers

Replying to xnyhps:

I've attached the updated patch.

Sorry it took me that much time to test it.

OK = pidgin-otr does not complain about messages being received unencrypted

  1. Running 2.10.7-2 from Debian sid + the patch: works fine, but using /me in a IRC chan (not private conversation, no OTR) not only rightly displays the message on the chan, but also opens a new (empty) window for that chan, which seems useless and surprising. This regression is why I have not bothered running as many interoperability tests as last time.
  2. Between two clients running 2.10.7-2 from Debian sid + the patch: OK
  3. Between one client running 2.10.7-2 from Debian sid + the patch, and another client running the same version, without the patch: OK from patched to non-patched, unencrypted warning from non-patched to patched, as expected
  4. Pidgin 2.10.7-2 from Debian sid + the patch on one side, and irssi-plugin-otr 1.0.0~alpha2-1 backported for Wheezy: OK both ways
  5. Between one client running 2.10.7-2 from Debian sid + the patch, and another client running Debian Wheezy's 2.10.6-3 without the patch:
    • patched sid to unpatched Wheezy: OK
    • unpatched Wheezy to patched sid: unencrypted warning, as expected
  6. Between one client running 2.10.7-2 from Debian sid + the patch, and another client running Debian Wheezy's 2.10.6-3 + the patch: OK both ways.

Changed 3 years ago by T(A)ILS developers

comment:15 Changed 3 years ago by T(A)ILS developers

Wrt. the first test case above: not only it opens a new window, but it apparently starts an OTR handshake. See attached ss.png.

AFAICT this is the only blocker to submit this patch for upstream review. xnyhps, any idea when you can come back to it?

Last edited 3 years ago by T(A)ILS developers (previous) (diff)

comment:16 Changed 3 years ago by T(A)ILS developers

DrWhax? just made me realize (thanks!) that I was unclear: the additional (useless) empty chan window opens in the patched Pidgin, not on the other side.

comment:17 Changed 3 years ago by xnyhps

Ah, I get what's going on. It needs to handle group chats and IMs separately.

Working on a new patch, but have to do some work to get my dev environment for Pidgin up again.

Changed 3 years ago by xnyhps

comment:18 Changed 3 years ago by xnyhps

Okay, updated the patch. /me in rooms should now no longer cause any weird effects.

Changed 3 years ago by xnyhps

Fixed msg incorrectly being freed, made sure sent-chat-msg is fired correctly and removed some dead code.

comment:19 follow-up: Changed 3 years ago by DrWhax

Cool! I will test the patch out one of these days. So the Tails folks can look at the results and focus on other things in the mean time.

comment:20 in reply to: ↑ 19 Changed 3 years ago by T(A)ILS developers

Replying to DrWhax:

Cool! I will test the patch out one of these days. So the Tails folks can look at the results and focus on other things in the mean time.

Thanks! Any ETA for this?

comment:21 Changed 3 years ago by datallah

Pidgin 2.10.8 is targeted to be released by the end of the month - if this is ready soon, we can include it.

comment:22 Changed 3 years ago by DrWhax

I'm working away by backlog. I should come back with results this weekend!

comment:23 Changed 3 years ago by Robby

Okay. cool. :-P

comment:24 Changed 3 years ago by datallah

  • Milestone changed from 2.10.8 to Patches Needing Review

comment:25 Changed 3 years ago by DrWhax

Hi everyone,

I have tested xnyhps' otrmefix4.patch. This was tested on two Debian Wheezy machines with Pidgin 2.10.9 (libpurple 2.10.9). From the Debian repositories with the patch applied.

  • It doesn't open a new window when using "/me" in a channel. Which was the case in the previous version of the patch.
  • From patched Pidgin to patched Pidgin a "/me" is encrypted and doesn't leak plaintext. (as expected)
  • from patched Pidgin to unpatched Pidgin a "/me" is encrypted and doesn't leak plaintext. (as expected)
  • From an unpatched Pidgin to a Patched Pidgin a "/me" is unencrypted and leaks plaintext. (as expected)
  • From patched Pidgin to irssi-otr(0.3 Uli Meis) it encrypts the "/me" as a message, not as an action.

That's it for now. I would need to setup some Debian boxes with sid to do more interoperability tests.

comment:26 Changed 3 years ago by T(A)ILS developers

So, DrWhax? and I did the rest of the interoperability tests:

  • patched wheezy -> unpatched sid: no unencrypted warning, as expected
  • unpatched sid -> patched wheezy: unencrypted, as expected
  • patched sid /me in an IRC chan: good
  • patched sid -> patched wheezy: no unencrypted warning, as expected
  • patched wheezy -> patched sid: no unencrypted warning, as expected
  • patched sid -> unpatched sid: no unencrypted warning, as expected
  • unpatched sid -> patched sid: unencrypted, as expected
  • patched sid -> irssi-plugin-otr 1.0: no unencrypted warning, as expected. no locales leaking anymore.
  • irssi-plugin-otr 1.0 -> patched sid: no unencrypted warning, as expected.

So this looks all right! All problems caused by earlier versions of this patch seem to be fixed.

Next step is to have someone review and merge this patch, then.

FTR, once this patch is part of an upstream Pidgin release, I'll have the patch part of a Debian security upload, or of a Debian Wheezy point-release, depending on how the Debian security team feels about it.

comment:27 follow-up: Changed 3 years ago by DrWhax

Ping Pidgin devs? Will this be merged upstream?

Debian OTR team has it already afaik: https://wiki.debian.org/Teams/OTR

comment:28 in reply to: ↑ 27 Changed 3 years ago by T(A)ILS developers

Replying to DrWhax:

Debian OTR team has it already afaik: https://wiki.debian.org/Teams/OTR

This patch is against Pidgin, that is not maintained by the Debian OTR Team. AFAIK, the Pidgin package in Debian does not apply this patch, and it's unlikely to apply it before it's been accepted upstream.

comment:29 Changed 3 years ago by DrWhax

Sorry I was wrong. In that case, I would be grateful if Pidgin team tests and merges this for the next release. So it finally lands in Debian.

comment:30 Changed 3 years ago by T(A)ILS developers

Ping: any reviewer wanting to take it? (Maybe the security implications of this problem are enough to motivate someone, who knows :)

comment:31 Changed 3 years ago by T(A)ILS developers

FTR, the current patch was proposed four months ago, widely tested and confirmed to work two months ago, and I pinged this ticket one month ago.

Is there anything we can do to help this security fix be applied? Anyone in particular I should personally ask to review the patch?

I thought I could simply follow https://developer.pidgin.im/wiki/SecurityVulnerabilityProcess, but the security@... email address is obscured on that page, so it's unclear to me how one can actually follow it in practice.

comment:32 follow-up: Changed 3 years ago by elb

That obfuscation is annoying and undesirable. I have added a "security at pidgin.im" after the mailto URL. I see the actual address when logged in, I'm not sure what's going on there.

I'm probably the person to talk to, and I've provided some feedback on this on IRC and in the jabber conference. I have not had time to do a proper review. Here are my two cents on the ticket itself:

  • This patch causes plugin API issues, but I think the issues are probably trumped by the leak, and I doubt they'll cause a huge problem.
  • I don't particularly like that this drags conversation signal emissions into IRC. There is almost certainly a better solution for this.
  • Neither of the above two issues would cause me to refuse this patch, because I think the former is a "bug" (given that other protocols pass a /me whatever through the signals API anyway) and the latter can always be mitigated later.

If someone wants to apply this patch, I won't scream. Tomasz?

comment:33 in reply to: ↑ 32 Changed 3 years ago by T(A)ILS developers

Replying to elb:

I have added a "security at pidgin.im" after the mailto URL.

Thanks a lot, this does help! (Note that there are a few more obfuscated references to the same email address later on the page; not sure if it's worth applying the same fix there too, or if it would uselessly bloat the text.)

I see the actual address when logged in, I'm not sure what's going on there.

I don't. Likely we don't have the same level of Trac credentials.

  • Neither of the above two issues would cause me to refuse this patch, because I think the former is a "bug" (given that other protocols pass a /me whatever through the signals API anyway) and the latter can always be mitigated later.

If someone wants to apply this patch, I won't scream. Tomasz?

Thanks a lot for these comments, I'm glad this is moving forward again.

Would it be useful to reassign to Tomasz, if they are the ones responsible for the next action? (I'm just asking, I've no idea what kind of workflow your team is using wrt. tickets ownership etc.)

comment:34 Changed 3 years ago by elb

It's up to Tomasz whether we reassign, he is aware of the status of this bug.

I apologize for the holdup on this, while it is not entirely my fault a fair amount of the blame can be laid at my feet. I have been very busy and had very little time for Pidgin for some time now, but I am still the closest thing to an "owner" for IRC and even certain security concerns. This patch hasn't been quite as stalled as it has appeared here, as there have been a number of discussions of it in the various development chats, but it certainly has been more stalled than I would have liked.

Thanks for sticking with us. Believe it or not, this stuff is important to us!

comment:35 Changed 3 years ago by tomkiewicz

  • Owner changed from elb to tomkiewicz

comment:36 Changed 3 years ago by tomkiewicz

  • Milestone changed from Patches Needing Review to 2.10.10

comment:37 Changed 3 years ago by Tomasz Wasilczyk <twasilczyk@…>

  • Resolution set to fixed
  • Status changed from new to closed

(In [3edc70bf4e09]):
Emit sending/sent signals when doing /me irc command. Fixes #15750

comment:38 Changed 2 years ago by arlolra

Should /msg get a corresponding fix? Seems to suffer from the same bypass.

comment:39 Changed 2 years ago by Robby

Separate ticket created by arlolra here: https://developer.pidgin.im/ticket/16397

Last edited 2 years ago by Robby (previous) (diff)

comment:40 Changed 2 years ago by datallah

This seems to have broken stuff - See #16407.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!