heap-buffer overflow in read_taskinfo()

[gy741]

Hello,

I found heap-buffer overflow bug.

If there is a large amount of data(,) in the taskinfo:tids of the info file, it falls into an infinite loop.

https://github.com/namhyung/uftrace/blob/2d6c907dedaecb37fb73b9a42f97020842c5b6d4/cmds/info.c#L560-L569

PoC:

cat uftrace.data/info

taskinfo:tids=,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,

Crash info:

$ uftrace info
uftrace: malloc.c:2401: sysmalloc: Assertion `(old_top == initial_top (av) && old_size == 0) || ((unsigned long) (old_size) >= MINSIZE && prev_inuse (old_top) && ((unsigned long) old_end & (pagesize - 1)) == 0)' fa

$bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f5b6b557801 in __GI_abort () at abort.c:79
#2  0x00007f5b6b5aaa91 in __malloc_assert (file=<optimized out>, function=<optimized out>, line=<optimized out>, assertion=<optimized out>) at malloc.c:298
#3  sysmalloc (nb=nb@entry=112, av=av@entry=0x7f5b6b902c40 <main_arena>) at malloc.c:2398
#4  0x00007f5b6b5abff0 in _int_malloc (av=av@entry=0x7f5b6b902c40 <main_arena>, bytes=bytes@entry=100) at malloc.c:4125
#5  0x00007f5b6b5ae0fc in __GI___libc_malloc (bytes=100) at malloc.c:3057
#6  0x00007f5b6b59f525 in _IO_vasprintf (result_ptr=0x7ffe0645e6d0, format=0x7f5b6b6ce7d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", args=args@entry=0x7ffe0645e5c0)
    at vasprintf.c:47
#7  0x00007f5b6b57c154 in ___asprintf (string_ptr=string_ptr@entry=0x7ffe0645e6d0, format=format@entry=0x7f5b6b6ce7d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n")
    at asprintf.c:35
#8  0x00007f5b6b5472f4 in __assert_fail_base (fmt=0x7f5b6b6ce7d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x45b28a "nr_tid == info->nr_tid",
    file=file@entry=0x45af00 "/workspace/security/uftrace/cmds/info.c", line=line@entry=576, function=function@entry=0x45ba90 <__PRETTY_FUNCTION__.7725> "read_taskinfo")
    at assert.c:57
#9  0x00007f5b6b547412 in __GI___assert_fail (assertion=0x45b28a "nr_tid == info->nr_tid", file=0x45af00 "/workspace/security/uftrace/cmds/info.c", line=576,
    function=0x45ba90 <__PRETTY_FUNCTION__.7725> "read_taskinfo") at assert.c:101
#10 0x000000000040c3f6 in read_taskinfo (arg=0x7ffe0645e8b0) at /workspace/security/uftrace/cmds/info.c:576
#11 0x000000000040d6fd in read_uftrace_info (info_mask=15359, handle=0x7ffe064609f0) at /workspace/security/uftrace/cmds/info.c:950
#12 0x000000000042edfc in open_info_file (opts=0x7ffe06460c40, handle=0x7ffe064609f0) at /workspace/security/uftrace/utils/data-file.c:492
#13 0x000000000040e6e9 in command_info (argc=0, argv=0x7ffe06460ea8, opts=0x7ffe06460c40) at /workspace/security/uftrace/cmds/info.c:1212
#14 0x0000000000405ee6 in main (argc=0, argv=0x7ffe06460ea8) at /workspace/security/uftrace/uftrace.c:1171

[gy741]

If someone is interested, can contribute to this problem.

[ParkSeungHyeok]

Hello, i am trying to solve this issue I think the solution to this issue is to add exception handling for nr_tid And i think also that if exception handling for nr_tid is added, if (*endp != ',' && *endp != '\n') sentence is not needed

So i tried implementing like following

            while (*endp != '\n') {
                int tid = strtol(tids_str, &endp, 10);
+               if(nr_tid < info->nr_tid){
                    tids[nr_tid++] = tid;
+               }
+               else{
+                   free(tids);
+                   goto out;
+               }

-               if (*endp != ',' && *endp != '\n') {
-                   free(tids);
-                   goto out;
-               }

                tids_str = endp + 1;
            }

how about it?

[namhyung]

Please take a look at the man page of strtol carefully and handle error cases according to the description. It seems strtol returns 0 for the invalid output. I don't think 0 is a valid value for tid in uftrace.

[ParkSeungHyeok]

Okay! thank you for your commant @namhyung . I will reflect your opinion.

Can i understand your opinion more exactly?

I understood this issue as following.

1. This issue's surface problem is heap-overflow by data(,).
2. Fundamental problem is lack of exception handling for tid.

So my approach is to add exception handling that work well whatever value was written to tid.

And regarding tid input, In normal state, tid should have same number of values as info->nr_tid and Each are separated by (,).

did i understand this correctly?

[ParkSeungHyeok]

I think about the cases like the following

Normal case of tid values

eg) if info->nr-tid is 5
tid=1,2,3,4,5

Abnormal case of tid values

1. Less tid values

   eg) if info->nr-tid is 5
   tid=1,2,3,4

2. More tid values

   eg) if info->nr-tid is 5
   tid=1,2,3,4,5,6 

3. Include character except ,

   tid=a,1,2,3,4

4. Include Continuous ,

   tid=,,,,,

So i tried implementing like below

            while (*endp != '\n') {
                int tid = strtol(tids_str, &endp, 10);
                if(nr_tid >= info->nr_tid){     // more tib values
                    free(tids);
                    goto out;   
                }
                if(tid){                // normal case
                    tids[nr_tid++] = tid;
                }
                else {                  // include character except ',' or include Continuous `,`
                    free(tids);
                    goto out;
                }

                tids_str = endp + 1;
            }

            if(nr_tid < info->nr_tid){          // less tib values
                free(tids);
                goto out;   
            }

[namhyung]

You can also check if it's separated by , as in the original code.

[ParkSeungHyeok]

Ah... i see

5. The case that another character is placed in place of (,)

   tid=1a2,3,4,5

So the reflected code looks like below

            while (*endp != '\n') {
                int tid = strtol(tids_str, &endp, 10);
                if(nr_tid >= info->nr_tid){     // more tib values
                    free(tids);
                    goto out;   
                }

                if(tid){                // normal case
                    tids[nr_tid++] = tid;
                }
                else {                  // include character except ',' or include Continuous `,`
                    free(tids);
                    goto out;
                }

                if (*endp != ',' && *endp != '\n') { 
                        free(tids); 
                        goto out; 
                } 

                tids_str = endp + 1;
            }

            if(nr_tid < info->nr_tid){          // less tib values
                free(tids);
                goto out;   
            }

In my opinion, i think there are to many if sentence I also think another code

            while (*endp != '\n') {
                int tid = strtol(tids_str, &endp, 10);
                if(tid && (nr_tid < info->nr_tid) && (*endp == ',' || *endp == '\n')){
                    tids[nr_tid++] = tid;
                }
                else {
                    free(tids);
                    goto out;
                }

                tids_str = endp + 1;
            }

            if(nr_tid < info->nr_tid){          // less tib values
                free(tids);
                goto out;   
            }

[honggyukim]

@ParkSeungHyeok Please send a PR instead of dropping code snippets here. That's much easier for us to review the code.

[honggyukim]

It'd be much better if you could add unittests for the cases you mentioned above.

[ParkSeungHyeok]

@namhyung @honggyukim I have submitted a PR for the code as a first step. Please review it when you have the chance. And regarding unit tests: in order to learn how to add tests, would you recommend studying the code within the test folder? could you suggest any effective methods or resources for studying unit tests?