Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use-after-free in internAgeRoute #90

Open
lorenz opened this issue Dec 28, 2022 · 2 comments
Open

Use-after-free in internAgeRoute #90

lorenz opened this issue Dec 28, 2022 · 2 comments

Comments

@lorenz
Copy link
Contributor

lorenz commented Dec 28, 2022

There seems to be a problem with routes getting dropped and then running though aging again. I can reproduce this reliably in a specific environment. Compiled with Clang and address sanitizer and undefined behavior sanitizer enabled.

adding VIF, Ix 0 Fl 0x0 IP 0x360ca8c0 enp5s0, Threshold: 1, Ratelimit: 0
adding VIF, Ix 1 Fl 0x0 IP 0x01a0a8c0 virbr0, Threshold: 1, Ratelimit: 0
Joining group 224.0.0.2 on interface virbr0
Joining group 224.0.0.22 on interface virbr0
RECV Membership query   from 192.168.160.1   to 224.0.0.1
RECV V2 member report   from 192.168.160.1   to 224.0.0.22
The IGMP message was from myself. Ignoring.
Route activation request from 192.168.12.54 for 239.255.255.250 is from myself. Ignoring.
Inserted route table entry for 239.255.255.250 on VIF #-1
RECV V2 member report   from 192.168.12.54   to 239.255.255.250
The IGMP message was from myself. Ignoring.
RECV V2 member report   from 192.168.160.1   to 239.255.255.250
The IGMP message was from myself. Ignoring.
RECV Leave message      from 192.168.160.1   to 224.0.0.2
RECV Membership query   from 192.168.12.1    to 239.255.255.250
RECV V2 member report   from 192.168.160.1   to 239.255.255.250
The IGMP message was from myself. Ignoring.
RECV V2 member report   from 192.168.12.54   to 239.255.255.250
The IGMP message was from myself. Ignoring.
RECV Leave message      from 192.168.160.1   to 224.0.0.2
RECV Membership query   from 192.168.12.1    to 239.255.255.250
RECV V2 member report   from 192.168.160.1   to 224.0.0.106
The IGMP message was from myself. Ignoring.
RECV V2 member report   from 192.168.160.1   to 224.0.0.2
The IGMP message was from myself. Ignoring.
RECV Membership query   from 192.168.12.1    to 224.0.0.1
RECV V2 member report   from 192.168.12.195  to 224.0.0.251
Mebership report was received on the upstream interface. Ignoring.
RECV Membership query   from 192.168.160.1   to 224.0.0.1
rttable.c:750:21: runtime error: shift exponent 4294967295 is too large for 32-bit type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior rttable.c:750:21 in 
Removing MFC: 192.168.160.1 -> 239.255.255.250, InpVIf: 0
MRT_DEL_MFC; Errno(2): No such file or directory
=================================================================
==117329==ERROR: AddressSanitizer: heap-use-after-free on address 0x608000000050 at pc 0x55a7b514af39 bp 0x7fff1cca0740 sp 0x7fff1cca0738
WRITE of size 4 at 0x608000000050 thread T0
    #0 0x55a7b514af38 in internAgeRoute src/rttable.c:713:5
    #1 0x55a7b514a587 in ageActiveRoutes src/rttable.c:504:13
    #2 0x55a7b513364a in age_callout_queue src/callout.c:106:14
    #3 0x55a7b513fa66 in igmpProxyRun src/igmpproxy.c:392:17
    #4 0x55a7b513e5af in main src/igmpproxy.c:189:9
    #5 0x7f52bb983082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/../csu/libc-start.c:308:16
    #6 0x55a7b507443d in _start (src/igmpproxy+0x3043d) (BuildId: ba0a9735a29cfce52b295fbaa372702eee3673d8)

0x608000000050 is located 48 bytes inside of 96-byte region [0x608000000020,0x608000000080)
freed by thread T0 here:
    #0 0x55a7b50f8d52 in free (src/igmpproxy+0xb4d52) (BuildId: ba0a9735a29cfce52b295fbaa372702eee3673d8)
    #1 0x55a7b514ae53 in removeRoute src/rttable.c:639:5
    #2 0x55a7b514ae53 in internAgeRoute src/rttable.c:706:13

previously allocated by thread T0 here:
    #0 0x55a7b50f8ffe in __interceptor_malloc (src/igmpproxy+0xb4ffe) (BuildId: ba0a9735a29cfce52b295fbaa372702eee3673d8)
    #1 0x55a7b5147739 in insertRoute src/rttable.c:300:40

SUMMARY: AddressSanitizer: heap-use-after-free src/rttable.c:713:5 in internAgeRoute
Shadow bytes around the buggy address:
  0x0c107fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c107fff8000: fa fa fa fa fd fd fd fd fd fd[fd]fd fd fd fd fd
  0x0c107fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c107fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c107fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c107fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c107fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==117329==ABORTING
@pali
Copy link
Owner

pali commented Dec 28, 2022

removeRoute() always release memory from passed argument:

static int removeRoute(struct RouteTable*  croute) {
...
    // Free the memory, and set the route to NULL...
    free(croute);
    croute = NULL;

    logRouteTable("Remove route");

    return result;
}

And internAgeRoute() after calling removeRoute(croute) it touches croute->ageVifBits, which is clearly use-after-free:

int internAgeRoute(struct RouteTable*  croute) {
...
    // If the aging counter has reached zero, its time for updating...
    if(croute->ageValue == 0) {
        // Check for activity in the aging process,
        if(croute->ageActivity>0) {
...
        } else {
...
            // No activity was registered within the timelimit, so remove the route.
            removeRoute(croute);
        }
        // Tell that the route was updated...
        result = 1;
    }

    // The aging vif bits must be reset for each round...
    BIT_ZERO(croute->ageVifBits);

    return result;
}

So this is a clear bug.

@pali
Copy link
Owner

pali commented Dec 28, 2022

rttable.c:750:21: runtime error: shift exponent 4294967295 is too large for 32-bit type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior rttable.c:750:21 in

On line 750 is:

igmpproxy/src/rttable.c

Lines 745 to 754 in 865a73c

// Set the TTL's for the route descriptor...
for ( Ix = 0; (Dp = getIfByIx(Ix)); Ix++ ) {
if(Dp->state == IF_STATE_UPSTREAM) {
continue;
}
else if(BIT_TST(route->vifBits, Dp->index)) {
my_log(LOG_DEBUG, 0, "Setting TTL for Vif %d to %d", Dp->index, Dp->threshold);
mrDesc.TtlVc[ Dp->index ] = Dp->threshold;
}
}

And shift exponent is too large is probably because Dp->index is -1.

In log is also:

Inserted route table entry for 239.255.255.250 on VIF #-1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lorenz @pali