Fix a buffer overflow in compose handling

[lf-]

I believe this has no security impact as the XCompose parser is operating on specifically user-configured data, but it is making my valgrind sad.

Thanks to Omni for the help in finding the root cause of this.

Test case (a full repro with hacks to get valgrind on the
ibus-engine-simple process is available at
https://github.com/lf-/ibus/tree/repro):

~/.XCompose is:

<Multi_key> <g> <h> : "η"
<Multi_key> <g> <v> <t> <h> : "ϑ"
<Multi_key> <g> <h>	: "ɣ"

Errors (from include-directive branch before applying this patch):

Invalid read of size 4
   at 0x48A6378: ibus_compose_data_compare (ibuscomposetable.c:509)
   by 0x4B64A38: ??? (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x4B649DE: ??? (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x48A8485: ibus_compose_table_new_with_file (ibuscomposetable.c:1108)
   by 0x48A871E: ibus_compose_table_list_add_file (ibuscomposetable.c:1204)
   by 0x48C087A: ibus_engine_simple_add_compose_file (ibusenginesimple.c:1863)
   by 0x48C0A8D: ibus_engine_simple_add_table_by_locale (ibusenginesimple.c:1788)
   by 0x109404: __lambda6_ (main.c:230)
   by 0x1094DE: ___lambda6__gsource_func (main.c:280)
   by 0x4B75A83: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x4BC99B0: ??? (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x4B74FD2: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.6600.4)
 Address 0x52926b0 is 0 bytes after a block of size 16 alloc'd
   at 0x483CD7B: realloc (vg_replace_malloc.c:834)
   by 0x4B748F8: g_realloc (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x48A72BA: parse_compose_sequence (ibuscomposetable.c:208)
   by 0x48A77F2: parse_compose_line (ibuscomposetable.c:344)
   by 0x48A79CF: ibus_compose_list_parse_file (ibuscomposetable.c:392)
   by 0x48A79E1: ibus_compose_list_parse_file (ibuscomposetable.c:394)
   by 0x48A83EE: ibus_compose_table_new_with_file (ibuscomposetable.c:1098)
   by 0x48A871E: ibus_compose_table_list_add_file (ibuscomposetable.c:1204)
   by 0x48C087A: ibus_engine_simple_add_compose_file (ibusenginesimple.c:1863)
   by 0x48C0A8D: ibus_engine_simple_add_table_by_locale (ibusenginesimple.c:1788)
   by 0x109404: __lambda6_ (main.c:230)
   by 0x1094DE: ___lambda6__gsource_func (main.c:280)

Invalid read of size 4
   at 0x48A637D: ibus_compose_data_compare (ibuscomposetable.c:510)
   by 0x4B64A38: ??? (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x4B649DE: ??? (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x48A8485: ibus_compose_table_new_with_file (ibuscomposetable.c:1108)
   by 0x48A871E: ibus_compose_table_list_add_file (ibuscomposetable.c:1204)
   by 0x48C087A: ibus_engine_simple_add_compose_file (ibusenginesimple.c:1863)
   by 0x48C0A8D: ibus_engine_simple_add_table_by_locale (ibusenginesimple.c:1788)
   by 0x109404: __lambda6_ (main.c:230)
   by 0x1094DE: ___lambda6__gsource_func (main.c:280)
   by 0x4B75A83: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x4BC99B0: ??? (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x4B74FD2: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.6600.4)
 Address 0x534a860 is 0 bytes after a block of size 16 alloc'd
   at 0x483CD7B: realloc (vg_replace_malloc.c:834)
   by 0x4B748F8: g_realloc (in /usr/lib/libglib-2.0.so.0.6600.4)
   by 0x48A72BA: parse_compose_sequence (ibuscomposetable.c:208)
   by 0x48A77F2: parse_compose_line (ibuscomposetable.c:344)
   by 0x48A79CF: ibus_compose_list_parse_file (ibuscomposetable.c:392)
   by 0x48A83EE: ibus_compose_table_new_with_file (ibuscomposetable.c:1098)
   by 0x48A871E: ibus_compose_table_list_add_file (ibuscomposetable.c:1204)
   by 0x48C087A: ibus_engine_simple_add_compose_file (ibusenginesimple.c:1863)
   by 0x48C0A8D: ibus_engine_simple_add_table_by_locale (ibusenginesimple.c:1788)
   by 0x109404: __lambda6_ (main.c:230)
   by 0x1094DE: ___lambda6__gsource_func (main.c:280)
   by 0x4B75A83: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6600.4)

[fujiwarat]

Probably I don't think your patch is not needed.

static gint
ibus_compose_data_compare (gpointer a,
                           gpointer b,
                           gpointer data)
{
    IBusComposeData *compose_data_a = a;
    IBusComposeData *compose_data_b = b;
    int max_compose_len = GPOINTER_TO_INT (data);
    int i;
    for (i = 0; i < max_compose_len; i++) {
        gunichar code_a = compose_data_a->sequence[i];
        gunichar code_b = compose_data_b->sequence[i];

        if (code_a != code_b)
            return code_a - code_b;
    }
    return 0;
}

max_compose_len is 5 in your case and it will return 0 because <Multi_key> <g> <h> : "η" and <Multi_key> <g> <h> : "ɣ" are same order.

[lf-]

Probably I don't think your patch is not needed.

static gint
ibus_compose_data_compare (gpointer a,
                           gpointer b,
                           gpointer data)
{
    IBusComposeData *compose_data_a = a;
    IBusComposeData *compose_data_b = b;
    int max_compose_len = GPOINTER_TO_INT (data);
    int i;
    for (i = 0; i < max_compose_len; i++) {
        gunichar code_a = compose_data_a->sequence[i];
        gunichar code_b = compose_data_b->sequence[i];

        if (code_a != code_b)
            return code_a - code_b;
    }
    return 0;
}

max_compose_len is 5 in your case and it will return 0 because <Multi_key> <g> <h> : "η" and <Multi_key> <g> <h> : "ɣ" are same order.

Hm. I agree that the input is not a reasonable/valid one, but it was equally accidental: it came from an xcompose file someone else wrote and I was using, and an invalid compose file causing a memory access violation is definitely a bug in my view. I've read carefully through the code again, and this is what's happening:

The space allocated for the first compose sequence is 4 gunichars (size 16; 3 chars and a null). If the sequences match up to and including the null terminator and max_compose_len is greater than the length of allocation for the sequence in question, we will read right over the null terminator into unallocated memory:

i = 2; i < max_compose_len ✔ -> code_a = 'h'; code_b = 'h', match: continue
i = 3; i < max_compose_len ✔ -> code_a = '\0'; code_b = '\0', match: continue
i = 4; i < max_compose_len ✔ -> code_a = out of bounds read; code_b = out of bounds read

The point of this patch is to catch this condition with the identical compose sequences by checking if it's reading the null terminator on both and stop.

Finally, the reason we aren't seeing this with non identical compose sequences is that if they mismatch before the null, we leave; if we read a character from one and a null from the other, we leave. So it can only happen on identical strings such as are found in the repro above.

[fujiwarat]

i = 4; i < max_compose_len ✔ -> code_a = out of bounds read; code_b = out of bounds read

I think the "out of bounds read" should not happen.

[lf-]

i = 4; i < max_compose_len heavy_check_mark -> code_a = out of bounds read; code_b = out of bounds read

I think the "out of bounds read" should not happen.

Well, it's reading 4 items past the start of an allocation of length 4, that's an out of bounds read. In order to not do an out of bounds read here, we either need to check for the null terminator, or allow reading past the null terminator but not into uninitialized memory by making the allocations all the same length.

The reason it's an out of bounds read here is because the if statement does not make the loop stop as every character in the string and the null terminator are equal. Further, the loop bounds max_compose_len are the longest allocation of any of the compose sequences: 5 items (excluding the null), in this case. This is greater than the length of the shorter compose sequence allocation that's causing this Valgrind error (4 items including null), so, since the loop is not stopped by the strings being unequal, or by a null terminator (since we don't have a null check before this patch), it will read out of bounds.

I believe that the original intent of the limit on the for loop is to prevent this out of bounds read, but it doesn't work as designed, because the allocations are different lengths and thus it doesn't stop reading out of bounds of a compose sequence of shorter length. Because our allocations are not the same length as the maximum length, we need to introduce a null check to guard against reading past the end of strings that are shorter than the maximum length.

[fujiwarat]

it's reading 4 items past the start of an allocation of length 4

I mean the length of compose_data_a->sequence should be greater than max_compose_len and your buffer overflow could not happen.
If the compose sequences are same, this logic will compare the output characters after null terminator.

[lf-]

it's reading 4 items past the start of an allocation of length 4

I mean the length of compose_data_a->sequence should be greater than max_compose_len and your buffer overflow could not happen.

This is not what the allocation code does, unfortunately, and that's what the Valgrind report is saying: it's allocated based on the input sequence length in parse_compose_sequence (there's a loop that reads characters and reallocs as needed, with no involvement of the maximum), and then is not subsequently reallocated to be the maximum length, as was probably expected when the comparison function was written. The max_compose_len never gets into the function that allocates the strings.

So my patch doesn't change that and it instead chooses to change the comparison, which does also solve the problem. The current design is sound assuming null terminators are appropriately checked for, which they seem to be elsewhere based on nothing else causing Valgrind errors. I could further change it to remove max_compose_len because it's misleading, I guess.

[fujiwarat]

Sorry max_compose_len does not work as I assumed in that function and I can reproduce your issue with Valgrind.
Thank you very much for your explanation.