Skip to content

curl: Fix UAF on clone due to callback data pointer mismatch - #24760

Open
ndossche wants to merge 1 commit into
php:PHP-8.4from
ndossche:curl-clone-callback-data
Open

ndossche wants to merge 1 commit into
php:PHP-8.4from
ndossche:curl-clone-callback-data

Conversation

@ndossche

Copy link
Copy Markdown
Member

When curl opaque objects are cloned, the curl handle was only updated if the callback function wasn't null. However, the C handler in PHP remains attached even if the callback was set to NULL (*). So we must unconditionally update the callback data pointer to the new curl handle.

(*) We can't set the C handler to NULL unconditionally on stable branches because that invokes a behaviour change (e.g. built in progress bar).

When curl opaque objects are cloned, the curl handle was only updated if
the callback function wasn't null. However, the C handler in PHP remains
attached even if the callback was set to NULL (*). So we must
unconditionally update the callback data pointer to the new curl handle.

(*) We can't set the C handler to NULL unconditionally on stable branches
because that invokes a behaviour change (e.g. built in progress bar).
Comment thread ext/curl/interface.c
curl_easy_setopt(ch->cp, CURLOPT_FILE, (void *) ch);
curl_easy_setopt(ch->cp, CURLOPT_INFILE, (void *) ch);
curl_easy_setopt(ch->cp, CURLOPT_WRITEHEADER, (void *) ch);
curl_easy_setopt(ch->cp, CURLOPT_DEBUGDATA, (void *) ch);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already done now by the php_curl_copy_fcc_with_option CURLOPT_DEBUGDATA code below

Comment thread ext/curl/interface.c
php_curl *ch = (php_curl *)clientp;
int rval = CURLKHMATCH_MISMATCH; /* cancel connection in case of an exception */

if (!ZEND_FCC_INITIALIZED(ch->handlers.sshhostkey)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is adjacently related, matches other handlers, otherwise a nulled callback can crash

@iliaal iliaal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix as always ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants