Repository navigation
multi/easy: reject internal handles in the public API - #22917
MegaManSec wants to merge 1 commit into
Conversation
Proof of concept (ASan reproducer)Build an ASan-instrumented libcurl, then compile and run the driver below. /* drv.c - build: cc -fsanitize=address -g -O1 drv.c -lcurl -o drv
run: ./drv exploit (ASan fires)
./drv control (clean exit) */
#include <curl/curl.h>
#include <stdio.h>
#include <string.h>
static int g_exploit = 0;
static void ntfy_cb(CURLM *m, unsigned int type, CURL *easy, void *ud)
{
(void)ud;
if(type == CURLMNOTIFY_INFO_READ && g_exploit) {
curl_multi_remove_handle(m, easy); /* detaches admin, leaves multi->admin dangling */
curl_easy_cleanup(easy); /* frees the admin allocation */
}
}
int main(int argc, char **argv)
{
g_exploit = (argc > 1 && strcmp(argv[1], "exploit") == 0);
curl_global_init(CURL_GLOBAL_DEFAULT);
CURLM *m = curl_multi_init();
curl_multi_setopt(m, CURLMOPT_NOTIFYFUNCTION, ntfy_cb);
curl_multi_notify_enable(m, CURLMNOTIFY_INFO_READ);
CURL *e = curl_easy_init();
curl_easy_setopt(e, CURLOPT_URL, "http://127.0.0.1:1/");
curl_easy_setopt(e, CURLOPT_CONNECTTIMEOUT_MS, 2000L);
curl_multi_add_handle(m, e);
int running = 1, loops = 0;
while(running && loops++ < 100000) {
if(curl_multi_perform(m, &running) != CURLM_OK) break;
if(running) { int n = 0; curl_multi_poll(m, NULL, 0, 50, &n); }
}
CURLMsg *msg; int q;
while((msg = curl_multi_info_read(m, &q))) {}
curl_multi_cleanup(m); /* second Curl_close(&multi->admin): double-free */
curl_global_cleanup();
return 0;
}The bug fires unconditionally — no specific server response required; any completing transfer (success, failure, timeout) queues the notification. |
ac993fe to
8d8ae85
Compare
8d8ae85 to
fb44e16
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The cleanup no-op leaves the API call stack unbalanced, and the reported regression lacks automated coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents applications from freeing libcurl-owned internal easy handles.
Changes:
- Rejects removal of internal easy handles.
- Makes their cleanup a no-op.
- Documents internal-handle restrictions.
File summaries
| File | Description |
|---|---|
lib/multi.c |
Rejects internal handles during removal. |
lib/easy.c |
Skips cleanup for internal handles. |
docs/libcurl/opts/CURLMOPT_NOTIFYFUNCTION.md |
Documents notification handle ownership. |
docs/libcurl/curl_multi_remove_handle.md |
Documents the new error behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I think we should reject internal handles in all API calls. The multi calls that act on easy handles need an extra check, for sure. |
A notify callback that removed and cleaned up the handle it gets for CURLMNOTIFY_INFO_READ freed multi->admin, leading to a use-after-free and a double-free in curl_multi_cleanup().
fb44e16 to
187bb0d
Compare
187bb0d to
3effdfe
Compare
|
Stole it and cleaned it for CI in #22966 😈 |
|
A travesty of justice |
|
I kept your name on it, just in case we need someone to blame. |
|
Passing blame? I see you're working on becoming a bit of a "security professional" yourself 😏 |
- Change curl_multi_add_handle(), curl_multi_remove_handle() and Curl_eapi_enter() to refuse easy handles with state.internal set. Prior to this change, a notify callback that removed and cleaned up the handle it gets for CURLMNOTIFY_INFO_READ freed multi->admin, leading to a use-after-free and a double-free in curl_multi_cleanup(). Co-authored-by: Stefan Eissing Closes #22917 Closes #22966
|
Thanks |
Curl_eapi_enter()now refuses easy handles withstate.internalset, as docurl_multi_add_handle()andcurl_multi_remove_handle().A
CURLMNOTIFY_INFO_READcallback that applies the documented remove-then-cleanup pattern to the handle it receives freesmulti->admin, causing a use-after-free and a double-free incurl_multi_cleanup().Putting the check in
Curl_eapi_enter()covers every easy API at once and keeps the call-stack guard balanced: makingcurl_easy_cleanup()a no-op instead would leave the entry unpopped, sinceeasy_cleanupis marked as killing its handle.Test 3107 covers it.