Skip to content

Commit

Permalink
wispr: Address unbalanced reference counting with proxy handling.
Browse files Browse the repository at this point in the history
This balances previously-unbalanced WISPr/portal context reference
counting associated with proxy handling that can lead to a heap-use-
after-free fault.

Prior to this change, 'proxy_callback' contained an unbalanced release
on the reference to a WISPr/portal context that could lead to its
premature destructio and a subsequent heap-use-after-free fault in
'free_wispr_routes'.

This situation could occur, due to this imbalance, when a new WISPr/portal
request for given service and IP configuration combination displaced a
prior, but unclosed and pending within 'g_web_request_get' request,
WISPr/portal context leading to its premature deletion. Later, when
the prior 'g_web_request_get' completed, either successfully or
unsuccessfully, this resulted in a different, yet balanced release on
the reference at the end of the 'wispr_portal_web_result' callback to
'g_web_request_get'. This also resulted in its (now redundant) destruction.
The heap-use-after-free actually occurred BEFORE the reference release,
in 'free_wispr_routes' since in 'wispr_portal_web_result',
'free_wispr_routes' is invoked BEFORE the reference is released.

This imbalance is addressed by balancing the reference release in
'proxy_callback' with a reference retain prior to either invoking
'connman_proxy_lookup' with the 'proxy_callback' callback or invoking
'g_idle_add' with the 'no_proxy_callback', which ultimately calls
'proxy_callback'.

The address sanitizer report associated with the potential heap-use-
after-free is as follows:

=================================================================
==2188==ERROR: AddressSanitizer: heap-use-after-free on address 0x72b0f4b4 at pc 0x000f36b5 bp 0x7eac98c8 sp 0x7eac98cc
READ of size 4 at 0x72b0f4b4 thread T0
    #0 0xf36b2 in free_wispr_routes src/wispr.c:140
    #1 0xf5fde in wispr_portal_web_result src/wispr.c:871
    #2 0x3285e in call_result_func gweb/gweb.c:465
    #3 0x3285e in received_data gweb/gweb.c:920

0x72b0f4b4 is located 100 bytes inside of 108-byte region [0x72b0f450,0x72b0f4bc)
freed by thread T0 here:
    #0 0x76abf264 in __interceptor_free /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0xf3916 in free_connman_wispr_portal_context src/wispr.c:211
    #2 0xf4706 in wispr_portal_context_unref_debug src/wispr.c:239
    #3 0xf6408 in __connman_wispr_start src/wispr.c:1101
    #4 0x9830a in __connman_service_wispr_start src/service.c:2421
    #5 0x990d2 in start_wispr_if_connected src/service.c:2375
    #6 0x9ae2c in default_changed src/service.c:2637
    #7 0x9afb6 in __connman_service_indicate_default src/service.c:7660
    #8 0x856de in set_default_gateway src/connection.c:509
    #9 0x86e3a in __connman_connection_update_gateway src/connection.c:1053
    #10 0x9b0aa in service_update_preferred_order src/service.c:7320
    #11 0x9b2d2 in service_indicate_state src/service.c:7400
    #12 0x9baaa in __connman_service_ipconfig_indicate_state src/service.c:7848
    #13 0x9ed28 in complete_online_check src/service.c:2219
    #14 0xf44fc in portal_manage_success_status src/wispr.c:516
    #15 0xf60ac in wispr_portal_web_result src/wispr.c:820
    #16 0x3285e in call_result_func gweb/gweb.c:465
    #17 0x3285e in received_data gweb/gweb.c:920

previously allocated by thread T0 here:
    #0 0x76abfa6c in __interceptor_calloc /data/jenkins/workspace/GNU-toolchain/arm-12/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77

SUMMARY: AddressSanitizer: heap-use-after-free src/wispr.c:140 in free_wispr_routes
  • Loading branch information
gerickson committed Nov 10, 2023
1 parent e21639c commit d92d036
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/wispr.c
Original file line number Diff line number Diff line change
Expand Up @@ -1033,9 +1033,18 @@ static int wispr_portal_detect(struct connman_wispr_portal_context *wp_context)
* connman_proxy_lookup, since the former will always result in
* a WISPr request "falling down a hole" that will only ever
* result in a failure completion.
*
* Whether following the connman_proxy_lookup / proxy_callback
* path or the g_idle_add / no_proxy_callback path, both funnel to
* proxy_callback. Retain a reference to the WISPr/portal context
* here and release it there (that is, in proxy_callback) such
* that the context is retained while the lookup or the idle
* timeout are in flight.
*/
if (proxy_method != CONNMAN_SERVICE_PROXY_METHOD_DIRECT &&
proxy_method != CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN) {
wispr_portal_context_ref(wp_context);

wp_context->token = connman_proxy_lookup(interface,
wp_context->status_url,
wp_context->service,
Expand All @@ -1046,6 +1055,8 @@ static int wispr_portal_detect(struct connman_wispr_portal_context *wp_context)
wispr_portal_context_unref(wp_context);
}
} else if (wp_context->timeout == 0) {
wispr_portal_context_ref(wp_context);

wp_context->timeout = g_idle_add(no_proxy_callback, wp_context);
}

Expand Down

0 comments on commit d92d036

Please sign in to comment.