Sandbox curl (PHP 7) #814

morrisonlevi · 2020-04-07T21:42:14Z

Description

This sandboxes curl on PHP 7. It has two parts:

The CurlSandboxIntegration, which does a posthook on curl_exec to make a span and add meta.
The C extension instruments curl_exec, curl_copy_handle, curl_close, curl_setopt, and curl_setopt_array to gather headers and to inject distributed tracing headers.

Some calls are not sandboxed yet, and there are a few other todo comments in the code.

Readiness checklist

Tests added for this feature/bug.

Reviewer checklist

Appropriate labels assigned.
Milestone is set.
Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

PHP 5 is stubbed out, but doesn't do anything.

Put ddtrace_curl_handlers_startup behind it.

Note that PHP 5 is stubbed out and doesn't sandbox.

The signature of zend_lookup_class_ex changed from taking "should autoload?" to taking flags. This is not documented in UPGRADING.INTERNALS. Note there is a change there about the "key" changing from zval* to zend_string*; we do not use this arg, so we avoid that change.

SammyK

Lovely PR @morrisonlevi! I left some questions and feedback. :)

src/DDTrace/Integrations/Curl/CurlSandboxedIntegration.php

SammyK · 2020-04-08T16:59:05Z

src/ext/ddtrace.c

+static void ddtrace_activate(void) {}
+static void ddtrace_deactivate(void) {}
+
+static zend_extension _dd_zend_extension_entry = {"ddtrace",


Do we know for sure if running as a Zend extension hybrid will have any side effects? Especially from the perspective of neighboring extensions?

I'm not sure. The only bits of the zend_extension we are using in this PR is the startup -- that should be very safe.

SammyK · 2020-04-08T17:00:09Z

src/ext/ddtrace.c

+    PHP5_UNUSED(extension);
+    PHP7_UNUSED(extension);
+
+    ddtrace_curl_handlers_startup();


Given we make _dd_ext_curl_loaded atomic, would having a one-time RINIT feature achieve this same functionality without having to run as a Zend extension hybrid?

No. The handlers must be overwritten in startup/minit -- rinit is too late. Doing it in minit will not work reliably if the extension we want to change the handlers for is loaded as a shared object, because then we have possible load order issues.

In other words, it's only safe in startup and minit and minit is too early. I think startup is literally the only place we can do this safely.

I see why doing it in MINIT is too early, but what makes RINIT too late? Doing it for every RINIT doesn't make sense, but doing it in a one-time RINIT would allow us to avoid running as a zend extension hybrid, no? Is there something that makes it unsafe to do in RINIT at all?

I guess ultimately my concern is really about converting to a zend extension hybrid which in my opinion has more unknowns in terms of side effects versus a one-time RINIT feature. But if that's our only option, let's definitely do some nice-neighbor framework testing. :)

Replacing an internal function handler in RINIT would only be safe for NTS builds, and I seem to remember a conversation in Stack Overflow PHP chat about how it may not be valid at all with JIT -- I'll try to dig that up.

Doing it in zend_extension startup should always be safe.

SammyK · 2020-04-08T17:01:55Z

src/ext/ddtrace.c

+                                          PHP_RSHUTDOWN(ddtrace),
+                                          PHP_MINFO(ddtrace),
+                                          PHP_DDTRACE_VERSION,
+                                          PHP_MODULE_GLOBALS(ddtrace),


tests/Integrations/Curl/CurlIntegrationTest.php

src/ext/php7/curl_handlers.c

SammyK · 2020-04-08T17:28:36Z

src/ext/php7/curl_handlers.c

+    _dd_curl_close_handler(INTERNAL_FUNCTION_PARAM_PASSTHRU);
+}
+
+static void _dd_instrument_curl_close(void) {


I'm a bit confused by this function name as it mentions instrumentation but I don't see any instrumentation logic. I'm guessing this is just a hook for the custom handler?

Same goes for _dd_instrument_curl_copy_handle(), _dd_instrument_curl_exec(), _dd_instrument_curl_setopt(), and _dd_instrument_curl_setopt_array() below. :)

This series of functions overrides the internal function handlers -- I could change "instrument" to "install"?

SammyK · 2020-04-08T17:44:18Z

src/ext/php7/curl_handlers.c

+
+    zval *curl_var = zend_call_method_with_0_params(NULL, NULL, NULL, "curl_init", &retval);
+    if (curl_var && Z_TYPE_P(curl_var) == IS_RESOURCE) {
+        le_curl = Z_RES_P(curl_var)->type;


Clever! 🔝 #SneakySneaks

package.xml

If we expand this technique to other extensions (like PDO), then the files will be next to each other for convenience.

SammyK

Nicely done. This just needs, milestone, labels, and changelog, but other than that it's good to go. Solid work @morrisonlevi!

morrisonlevi · 2020-04-10T16:18:49Z

Everything still look good after testing on the nice-neighbor framework and also running a build- branch -- going to merge it in.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Add SSRF Rasp capability (#814) Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

…og changes (#3005) * refactor: add feature_flag to Cargo.toml according to libdatadog changes * change reference to libdatadog * Make cbindgen Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Bump the minimum rust version to 1.78 Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * test: fix prof asan nightly version * Avoid undefined symbol errors in test run Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Add SSRF Rasp capability (#814) Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Levi Morrison <levi.morrison@datadoghq.com>

morrisonlevi added 6 commits April 2, 2020 11:20

We are a zend_extension now

8ce8cd5

Add curl handler boilerplate (PHP 7)

9de90a8

PHP 5 is stubbed out, but doesn't do anything.

Sandbox ddtrace_curl_handlers_rinit

de11541

Move isSandboxEnabled check to C and...

d23560a

Put ddtrace_curl_handlers_startup behind it.

Add sandboxed curl and testing infrastructure (PHP 7)

309b1c9

Note that PHP 5 is stubbed out and doesn't sandbox.

morrisonlevi force-pushed the levi/curl branch from cf72b54 to 707aacf Compare April 8, 2020 14:46

SammyK suggested changes Apr 8, 2020

View reviewed changes

Remove unused hasError SpanData property

36236b0

morrisonlevi force-pushed the levi/curl branch from ecca3de to 5005014 Compare April 8, 2020 18:16

Rename curl_handlers to handlers_curl

f2badd9

If we expand this technique to other extensions (like PDO), then the files will be next to each other for convenience.

morrisonlevi force-pushed the levi/curl branch from 5005014 to f2badd9 Compare April 8, 2020 18:25

Rename _dd_instrument_curl* to _dd_install_curl*

5901efb

morrisonlevi force-pushed the levi/curl branch from 8a58ae7 to 5901efb Compare April 8, 2020 18:38

morrisonlevi added 2 commits April 8, 2020 12:56

Sandbox ddtrace_curl_* functions

0238c29

Extract engine helpers to engine_api.{c,h}

3ec937d

morrisonlevi force-pushed the levi/curl branch from 578b38c to 3ec937d Compare April 8, 2020 21:58

Add Configuration isDistributedTracingEnabled checks

59888e2

morrisonlevi marked this pull request as ready for review April 9, 2020 08:12

SammyK approved these changes Apr 9, 2020

View reviewed changes

morrisonlevi added c-extension Apply this label to issues and prs related to the C-extension 🎉 new-integration A new integration labels Apr 9, 2020

morrisonlevi added this to the 0.43.0 milestone Apr 10, 2020

morrisonlevi merged commit c690c04 into master Apr 10, 2020

morrisonlevi deleted the levi/curl branch April 10, 2020 16:19

morrisonlevi mentioned this pull request Apr 11, 2020

Fix curl issues and implement service mapping in one place #817

Merged

4 tasks

SammyK mentioned this pull request Jun 4, 2020

Sandbox cURL (PHP 5) #911

Merged

5 tasks

SammyK mentioned this pull request Jul 27, 2020

[Sandbox] cURL integration #634

Closed

5 tasks

bwoebi added a commit that referenced this pull request Jan 6, 2025

Avoid undefined symbol errors in test run

39df479

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Add SSRF Rasp capability (#814) Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sandbox curl (PHP 7) #814

Sandbox curl (PHP 7) #814

morrisonlevi commented Apr 7, 2020 •

edited

Loading

SammyK left a comment

SammyK Apr 8, 2020

morrisonlevi Apr 8, 2020

SammyK Apr 8, 2020

morrisonlevi Apr 8, 2020 •

edited

Loading

SammyK Apr 8, 2020

morrisonlevi Apr 8, 2020

SammyK Apr 8, 2020

SammyK Apr 8, 2020

morrisonlevi Apr 8, 2020

SammyK Apr 8, 2020

SammyK left a comment

morrisonlevi commented Apr 10, 2020

Sandbox curl (PHP 7) #814

Sandbox curl (PHP 7) #814

Conversation

morrisonlevi commented Apr 7, 2020 • edited Loading

Description

Readiness checklist

Reviewer checklist

SammyK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morrisonlevi Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SammyK left a comment

Choose a reason for hiding this comment

morrisonlevi commented Apr 10, 2020

morrisonlevi commented Apr 7, 2020 •

edited

Loading

morrisonlevi Apr 8, 2020 •

edited

Loading