3 ms·

Re-reading this, my analysis is slightly incorrect: the `MAX` at [5] with an unsigned arg means we can make `npins` an arbitrary `int` using the loop at [4]. C

Re-reading this, my analysis is slightly incorrect: the `MAX` at [5] with an unsigned arg means we can make `npins` an arbitrary `int` using the loop at [4]. Choosing to make `npins` negative using that loop means we'll end up allocating an array of 87 (`SYS_kbind + 1`) `int`s at [8] and continue with the OOB accesses described. You'd set up your `pinsyscall` entries like this: struct pinsyscall entries[] = { { .sysno = 0x1111, .offset = 0xdeadbeef }, /* first oob write */ { .sysno = 0x2222, .offset = 0xf000f000 }, /* second oob write */ { .sysno = 0xffffffff } /* sets npins to 0xffffffff so we under-allocate */ }; `npins` would be `0xffffffff` after the loop and then the `MAX` at [6] would then return `86`, since `MAX(-1, 86) == 86`.

actionfromafar 3y ago
I must misunderstand something very basic about this code. if (pins[syscalls[i].sysno]) but pins is newly allocated and should just zero or "empty". Why dereference it right after allocation?
- accessvector 3y ago
  Just to handle the case where the same syscall number is specified twice by the ELF header: in that case, the entry is set to -1 (presumably meaning it’s invalid).
  - actionfromafar 3y ago
    I still don't get it. Shouldn't [9] always evaluate to false, and the code be equivalent to: pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO); for (i = 0; i < nsyscalls; i++) { pins[syscalls[i].sysno] = syscalls[i].offset; } Edit: Hang on - npins is already checked in the loop before, and incremented with ++ syscalls[i].sysno can't be larger than what is allocated with: pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO); So I still can't find the problem
    - accessvector 3y ago
      Consider this: struct pinsyscall entries[] = { { .sysno = 1, .offset = 0x1234 }, { .sysno = 2, .offset = 0x5678 }, { .sysno = 1, .offset = 0x9abc } }; Now `nsyscalls` will be 3 and `pin` will be an array of 3 ints, initialised to `{ 0, 0, 0 }`. When we loop through, we'll set: 1. `pin[syscalls[0].sysno] = 0x1234` => `pin[1] = 0x1234` 2. `pin[syscalls[1].sysno] = 0x5678` => `pin[2] = 0x5678` Now when we come to 3, we'll find `pin[syscalls[2].sysno] != 0` since `syscalls[2].sysno == syscalls[0].sysno` - so we set `pin[1] = -1` instead of `0x9abc`.
      - actionfromafar 3y ago
        Oh, thanks, now I understand why there is an if in the for loop! But I still can't see how pin[] could be accessed out of bounds, since the array is allocated to be large enough to hold the largest value of .sysno occurring in the entries[] array.
        accessvector 3y ago
        Right, so this is the crux of the vulnerability. Firstly, note that the `MAX` macro is defined as: #define MAX(a, b) ((a) > (b) ? (a) : (b)) This is important because it doesn't cast either arg to any particular type. You can use it with `float`s, or `int`s, or `u_int`s... or a combination. Referring back to the implementation, the first use of `MAX` (inside the `for` loop) is this: ... for (i = 0; i < nsyscalls; i++) npins = MAX(npins, syscalls[i].sysno); ... `npins` is an `int`, but `sysno` is a `u_int`. C integer promotion rules means that we'll actually be implicitly casting `npins` to a `u_int` here; it's as if we did this: ... npins = MAX((u_int)npins, syscalls[i].sysno); ... This means that `npins` can end up as any value we like -- even up to `0xffffffff`. But remember that `npins` is _actually_ a signed int, so once it comes out of `MAX`, it'll be signed again. Thus we can use this to make `npins` negative. Once we're out of the loop, `MAX` is used again here: ... npins = MAX(npins, SYS_kbind); ... Where `SYS_kbind` is just: ... #define SYS_kbind 86 ... Integer literals in C are signed, so now this use of `MAX` is actually dealing with two signed integers. If we used the loop to make `npins` negative (as described just before) then this line will now take 86 as the maximum of the two values. With `npins = 86`, an array of 86+1 will be allocated, but the `syscalls[i].sysno` in the next loop could of course easily be greater than 86 -- thus leading to out-of-bounds array access.

actionfromafar 3y ago

I must misunderstand something very basic about this code. if (pins[syscalls[i].sysno]) but pins is newly allocated and should just zero or "empty". Why dereference it right after allocation?

accessvector 3y ago
Just to handle the case where the same syscall number is specified twice by the ELF header: in that case, the entry is set to -1 (presumably meaning it’s invalid).
- actionfromafar 3y ago
  I still don't get it. Shouldn't [9] always evaluate to false, and the code be equivalent to: pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO); for (i = 0; i < nsyscalls; i++) { pins[syscalls[i].sysno] = syscalls[i].offset; } Edit: Hang on - npins is already checked in the loop before, and incremented with ++ syscalls[i].sysno can't be larger than what is allocated with: pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO); So I still can't find the problem
  - accessvector 3y ago
    Consider this: struct pinsyscall entries[] = { { .sysno = 1, .offset = 0x1234 }, { .sysno = 2, .offset = 0x5678 }, { .sysno = 1, .offset = 0x9abc } }; Now `nsyscalls` will be 3 and `pin` will be an array of 3 ints, initialised to `{ 0, 0, 0 }`. When we loop through, we'll set: 1. `pin[syscalls[0].sysno] = 0x1234` => `pin[1] = 0x1234` 2. `pin[syscalls[1].sysno] = 0x5678` => `pin[2] = 0x5678` Now when we come to 3, we'll find `pin[syscalls[2].sysno] != 0` since `syscalls[2].sysno == syscalls[0].sysno` - so we set `pin[1] = -1` instead of `0x9abc`.
    - actionfromafar 3y ago
      Oh, thanks, now I understand why there is an if in the for loop! But I still can't see how pin[] could be accessed out of bounds, since the array is allocated to be large enough to hold the largest value of .sysno occurring in the entries[] array.
      - accessvector 3y ago
        Right, so this is the crux of the vulnerability. Firstly, note that the `MAX` macro is defined as: #define MAX(a, b) ((a) > (b) ? (a) : (b)) This is important because it doesn't cast either arg to any particular type. You can use it with `float`s, or `int`s, or `u_int`s... or a combination. Referring back to the implementation, the first use of `MAX` (inside the `for` loop) is this: ... for (i = 0; i < nsyscalls; i++) npins = MAX(npins, syscalls[i].sysno); ... `npins` is an `int`, but `sysno` is a `u_int`. C integer promotion rules means that we'll actually be implicitly casting `npins` to a `u_int` here; it's as if we did this: ... npins = MAX((u_int)npins, syscalls[i].sysno); ... This means that `npins` can end up as any value we like -- even up to `0xffffffff`. But remember that `npins` is _actually_ a signed int, so once it comes out of `MAX`, it'll be signed again. Thus we can use this to make `npins` negative. Once we're out of the loop, `MAX` is used again here: ... npins = MAX(npins, SYS_kbind); ... Where `SYS_kbind` is just: ... #define SYS_kbind 86 ... Integer literals in C are signed, so now this use of `MAX` is actually dealing with two signed integers. If we used the loop to make `npins` negative (as described just before) then this line will now take 86 as the maximum of the two values. With `npins = 86`, an array of 86+1 will be allocated, but the `syscalls[i].sysno` in the next loop could of course easily be greater than 86 -- thus leading to out-of-bounds array access.