Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#14636 closed defect (fixed)

Heap memory corruption using g_markup_escape_text() without sanitizing first

Reported by: dbauche Owned by: elb
Milestone: v2.10.1 Component: SILC
Version: 2.10.0 Keywords: Overflow,Heap,g_markup_escape_text
Cc:

Description

Can anyone confirm if there is no ticket for this yet?

From libpurple/protocols/silc/ops.c:


static void silc_private_message(SilcClient? client, SilcClientConnection? conn,

SilcClientEntry? sender, SilcMessagePayload? payload, SilcMessageFlags? flags, const unsigned char *message, SilcUInt32 message_len)

{

PurpleConnection? *gc = client->application; SilcPurple? sg = gc->proto_data; PurpleConversation? *convo = NULL; char *msg, *tmp;

[...]

if (flags & SILC_MESSAGE_FLAG_UTF8) { tmp = g_markup_escape_text((const char *)message, -1); /* Send to Purple */ serv_got_im(gc, sender->nickname, tmp, 0, time(NULL)); g_free(tmp);

[...]

}


silc_private_message() and silc_channel_message() both take the "flags" argument, which are sent by the client along with the message.

Inside of this function, we can read that if the SILC_MESSAGE_FLAG_UTF8 flag is set, g_markup_escape_text() is called. g_markup_escape_text() in turn calls append_escaped_text() as seen here:

From glib/gmarkup.c:


gchar* g_markup_escape_text (const gchar *text,

gssize length)

{

GString *str;

g_return_val_if_fail (text != NULL, NULL);

if (length < 0)

length = strlen (text);

/* prealloc at least as long as original text */ str = g_string_sized_new (length); append_escaped_text (str, text, length);

return g_string_free (str, FALSE);

}

[...]

static void append_escaped_text (GString *str,

const gchar *text, gssize length)

{

const gchar *p; const gchar *end; gunichar c;

p = text; end = text + length;

while (p != end)

{

const gchar *next; next = g_utf8_next_char (p);

[...]

p = next;

}

}


As seen above, it enters a loop calling g_utf8_next_char() with our user-controlled message until "next" matches "end".

Doing a quick search you can find the documentation for g_utf8_next_char():


#define g_utf8_next_char(p) (char *)((p) + g_utf8_skip[*(const guchar *)(p)])

"Skips to the next character in a UTF-8 string. The string must be valid; this macro is as fast as possible, and has no error-checking. You would use this macro to iterate over a string character by character. The macro returns the start of the next UTF-8 character. Before using this macro, use g_utf8_validate() to validate strings that may contain invalid UTF-8."


The documentation says we need to validate the string as valid UTF-8 first before using g_utf8_next_char(). but this code never does it.

This in turn makes g_utf_next_char() return invalid pointers if the message is not valid utf-8, which in turn means the while(p != end) condition is never met, taking the processor into an infinite loop with p increasing until it tries to read non-segmented memory:

Finally, we get a pidgin crash when using the SILC plugin and sending a channel or private message (Sending a message like "\x61\xf8" does the trick):


Program received signal SIGSEGV, Segmentation fault. append_escaped_text (text=0x845a000 <Address 0x845a000 out of bounds>, length=-1)

at /build/buildd-glib2.0_2.24.2-1-i386-AScyie/glib2.0-2.24.2/glib/gmarkup.c:2040

2040 /build/buildd-glib2.0_2.24.2-1-i386-AScyie/glib2.0-2.24.2/glib/gmarkup.c: No such file or directory.

in /build/buildd-glib2.0_2.24.2-1-i386-AScyie/glib2.0-2.24.2/glib/gmarkup.c

(gdb) x/i $pc 0x471108 <IAg_markup_escape_text+120>: movzx eax,BYTE PTR [esi] (gdb) x/x $esi 0x845a000: Cannot access memory at address 0x845a000 (gdb)


I'm not sure if this is bug is only found inside the SILC plugin, I did not check other protocols, but anything using g_markup_escape_text() without making sure it is proper UTF-8 is potentially susceptible to the same problems.

Wouldn't adding something like g_utf8_validate(message, -1, message+strlen(message)); before calling g_markup_escape_text() fix this?

Seems to be present on the latest releases of pidgin (And in theory, i'm not sure... on all clients using libpurple with the same glib2 libs). Tested on Windows and Linux.

Change History (5)

comment:1 Changed 7 years ago by elb@…

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

(In 7eb1f6d56cc58bbb5b56b7df53955d36b9b419b8):
Salvage incoming SILC text if necessary.

Fixes #14636

comment:2 Changed 7 years ago by elb

I believe I have fixed this issue in 7eb1f6d56cc58bbb5b56b7df53955d36b9b419b8. Please review and test.

dbauche, please contact me off-tracker at elb@….

comment:3 Changed 7 years ago by Robby

  • Milestone changed from 2.10.1 to v2.10.1

comment:4 Changed 7 years ago by markdoliner@…

(In afb9ede3de989f217f03d5670cca00e628bd11f1):
Fix another possible remote crash bug in SILC.

This is the same change that Ethan made in 7eb1f6d56cc58bbb5b56b7df53955d36b9b419b8, but to a different function. Refs #14636

comment:5 Changed 7 years ago by clas

Tested and it fixes the issue i've been having issues with for days.

Nice work Thanks ! /Ivan.


Can anyone confirm if there is no ticket for this yet?

From libpurple/protocols/silc/ops.c:

static void silc_private_message(SilcClient?? events client, SilcClientConnection?? conn,

SilcClientEntry?? sender, SilcMessagePayload?? payload, SilcMessageFlags?? flags, arrangement const unsigned char *message, SilcUInt32 message_len)

{

PurpleConnection?? *gc = client->application; SilcPurple?? sg = gc->proto_data; PurpleConversation?? *convo = NULL; char *msg, *tmp;

[...]

if (flags & SILC_MESSAGE_FLAG_UTF8) { tmp = g_markup_escape_text((const char *)message, -1); /* Send to teambuilding */ serv_got_im(gc, sender->nickname, tmp, 0, time(NULL)); g_free(tmp);

[...]

}

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!