[REGRESSION] pidgin crashes in str_not_empty()

[ildar]

Core was generated by `pidgin'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007f82f7de17ab in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x00007f82f7de17ab in raise () from /lib64/libc.so.6
#1  0x00007f82f7dcc515 in abort () from /lib64/libc.so.6
#2  0x000000000048255a in ?? ()
#3  <signal handler called>
#4  0x00007f82e878b989 in str_not_empty (
    string=0x40459b8d1e6d15bc <error: Cannot access memory at address 0x40459b8d1e6d15bc>) at tgp-utils.c:67
#5  0x00007f82e878f04f in tgp_msg_display (C=0x12c54330, TLS=0x11bbe200)
    at tgp-msg.c:750
#6  tgp_msg_process_in_ready (TLS=TLS@entry=0x11bbe200) at tgp-msg.c:869
#7  0x00007f82e8790323 in tgp_msg_recv (TLS=0x11bbe200, M=<optimized out>,
    before=0x0) at tgp-msg.c:1102
#8  0x00007f82e87a7e95 in get_difference_on_answer (TLS=0x11bbe200,
    q=0x385e390, D=0x6a87c00) at queries.c:3799

#9  0x00007f82e879ea96 in tglq_query_result (TLS=TLS@entry=0x11bbe200,
    id=<optimized out>) at queries.c:479
#10 0x00007f82e87967c7 in work_rpc_result (c=0xe8cb080,
    msg_id=<optimized out>, TLS=0x11bbe200) at mtproto-client.c:848
#11 rpc_execute_answer (TLS=TLS@entry=0x11bbe200, c=c@entry=0xe8cb080,
    msg_id=<optimized out>) at mtproto-client.c:949
#12 0x00007f82e879776d in process_rpc_message (
    enc=0x7f82e9893c80 <Response.11567>, len=<optimized out>, c=0xe8cb080,
    TLS=0x11bbe200) at mtproto-client.c:1135
#13 rpc_execute (TLS=0x11bbe200, c=0xe8cb080, op=<optimized out>,
    len=<optimized out>) at mtproto-client.c:1189
#14 0x00007f82e87856cf in try_rpc_read (c=0xe8cb080) at tgp-net.c:431
#15 try_read (c=0xe8cb080) at tgp-net.c:476
#16 conn_try_read (arg=0xe8cb080, source=<optimized out>,
    cond=<optimized out>) at tgp-net.c:227
#17 0x000000000046a9ae in ?? ()
#18 0x00007f82f82b7b31 in g_main_context_dispatch ()
   from /lib64/libglib-2.0.so.0
#19 0x00007f82f82b7f08 in ?? () from /lib64/libglib-2.0.so.0
#20 0x00007f82f82b81ca in g_main_loop_run () from /lib64/libglib-2.0.so.0
#21 0x00007f82f89a5d37 in gtk_main () from /usr/lib64/libgtk-x11-2.0.so.0
#22 0x0000000000432b50 in main ()

[ildar]

this is the regression: rolling back to 1.4.1 fixed the issue.

[BenWiederhake]

string=0x40459b8d1e6d15bc <error: Cannot access memory at address 0x40459b8d1e6d15bc>

That sounds like some nasty memory corruption bug. Shit.

[JayFoxRox]

Likely same as #525

I have done a rough analysis of this, but not made a fix (and don't intend to do so).

The crash is here, when tgl_message_media_geo is received: https://github.com/majn/telegram-purple/blob/44a1349bf4c57e8b648dae113ec7cf3bdbde0789/tgp-msg.c#L749-L750

That caption field is in a union with geo data: https://github.com/majn/tgl/blob/bec2e6d537c272ed185e8c41ff81b4b8521a131d/tgl-layout.h#L569-L581

The geo data is written here (which fills the caption fields memory with non-char* data): https://github.com/majn/tgl/blob/bec2e6d537c272ed185e8c41ff81b4b8521a131d/structures.c#L803-L806

Similar issues might also affect other package types.

---

I consider this to be a bug in https://github.com/majn/telegram-purple/blob/44a1349bf4c57e8b648dae113ec7cf3bdbde0789/tgp-msg.c#L749-L750

It implies that the caption field is empty. This is not true. It's a union and it only seems to be valid for these 2 types: https://github.com/majn/tgl/blob/bec2e6d537c272ed185e8c41ff81b4b8521a131d/tgl-layout.h#L572-L573

This shows that the union/structure mix is very bad design that's hard to decipher. The photo and document types each have their own caption field anyway (which is probably meant to be used; or it's a different feature using a confusing name).

---

Another instance of the same problem appears to be in this: https://github.com/majn/telegram-purple/blob/44a1349bf4c57e8b648dae113ec7cf3bdbde0789/tgp-msg.c#L527-L535

There might be more locations where this is bad.

---

The bug was introduced by a couple of people doing stupid things. https://github.com/majn/telegram-purple/commit/9e6a0e850a3640eed5c14259129a62b210218e4d is a prominent one because it moved the check from the photo specific handler to all messages, and it introduced bad assumptions. However, the original implementation of the photo captions is equally horrible (as it introduces features which easily break): https://github.com/majn/tgl/commit/a4588778f998d0f27fb7e342a84b2b4ab56274dc

I think the proper solution is to remove the caption field as it too easy to break other union members. Ideally, all of these anonymous structs should be avoided in the union - they don't seem to follow any logical pattern. Also the code that touches captions should be refactored, so it only ever touches captions of objects.

---

A temporary workaround that I've applied locally, is to move the caption out of the union:

[...]
struct tgl_message_media {
  enum tgl_message_media_type type;
  char *caption; // Added here
  union {
    struct {
      union {
        struct tgl_photo *photo;
        struct tgl_document *document;
      };
      // Removed from here
    };
[...]

I don't feel confident in this workaround though - the code is so bad, that there might still be uninitialized accesses somehow. Also the same issue could still happen for other members of the union.

---

I had already intended to migrate away from purple due to frustration with pidgins lack of modern features and the quality of protocol plugins in recent years (also particularly telegram-purple). Reviewing the code and how poorly this situation was dealt with shows how critical it should be to move away from telegram-purple ("This looks a lot like memory corruption. In other words I won't be able to fix it" - in a security relevant piece of code!). Maintainers of a protocol plugin should be able to debug these sort of issues; it only takes a matter of seconds: reproduce it, find erroneous access, add watchpoint, analyze.

[BenWiederhake]

Dude, this is free software. The original maintainer of the library @vysheng has been inactive for very long. I'm only a user who tries to keep it in a "good enough" state for myself. You're barking up the wrong tree.

Come down from your high horse, and implement it better. We nowhere claimed that telegram-purple is good. In fact, I already recommend that people use something else, or implement something better: #480

Finally, telegram-purple is a plugin, and not part of pidgin.

it only takes a matter of seconds: reproduce it, find erroneous access, add watchpoint, analyze.

Then do it? PRs are extremely welcome, and you'll get write access immediately.

[JayFoxRox]

Dude, this is free software. [...] You're barking up the wrong tree.

Maintaining free software still comes with responsibilities. While I'm grateful for the existence of projects like this, it doesn't excuse the state of the project.

We nowhere claimed that telegram-purple is good. In fact, I already recommend that people use something else, or implement something better: #480

That is a technical discussion about switching the backend, not a recommendation [to users] to "use something else" [other than telegram-purple].

I think this bug could be a security relevant issue in this software. All functions using these buffers that I have seen seem to be safe (g_strdup); however, it's really hard to know for sure. The way the code reads wrong fields from a union could still potentially allow an attacker to control values maliciously somewhere. This could possibly allow gadgets for a more powerful attack (such as Remote-Code-Execution).

So until this is analyzed, the code should be rolled back to a safe revision. If code can't be easily reverted, the proper response would be to shut this project down (at least temporarily) and explain in the README that it is potentially unsafe to use. If maintainers can't fix it, then they should be clear about this, and advise users to stay away.

I'm only a user who tries to keep it in a "good enough" state for myself

Fair enough. But this should be documented prominently. Packagers should be advised to not package it anymore.

If you make it available publicly, then it should be safe, and potential security risks should be avoided (and reports taken seriously).

Come down from your high horse, [...]

No need for personal attacks.

[...] and implement it better.

I don't think that pointing out problems comes with responsibility to "do it better". I have also already spent some of my time to investigate the bug. I also took additional time to document the issue. I have also pointed out what changes could be made to make it safer.

I have also clarified that I have no interest in fixing it myself, as I had planned to migrate away anyway (from pidgin / purple based clients; and by extension: telegram-purple). So I don't have any motivation to fix this plugin: I likely won't be using it. However, everyone who keeps using it should worry about these things.

[BenWiederhake]

This is off-topic. Let's stick to @ildar's issue, and nothing else.

[GruensFroeschli]

Have run into the same issue. Traced it with gdb and found this thread.

Thanks for the hint that 1.4.1 is still working. I reverted for now to 1.4.1 for my setup.

[BenWiederhake]

In case JayFoxRox is right, this is fixed in 1.4.3 due to #522.