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.

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

Blocking from a hook is not stopping code execution #2836

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Refactor bailing out strategy
  • Loading branch information
estringana committed Dec 16, 2024
commit 7fd4a505ff268433b9f6a91fa28f6337bb7ad124
1 change: 0 additions & 1 deletion appsec/tests/extension/push_params_block_02.phpt
Original file line number Diff line number Diff line change
@@ -35,7 +35,6 @@ $integration = new SomeIntegration();
$integration->init();
var_dump(ltrim(" Calling wrapped function"));
var_dump("THIS SHOULD NOT GET IN THE OUTPUT");

?>
--EXPECTHEADERS--
Status: 404 Not Found
43 changes: 43 additions & 0 deletions appsec/tests/extension/push_params_block_03.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
--TEST--
Push address gets blocked even when within a hook
--INI--
extension=ddtrace.so
datadog.appsec.enabled=1
--FILE--
<?php
use function datadog\appsec\testing\{rinit,rshutdown};
use function datadog\appsec\push_address;

include __DIR__ . '/inc/mock_helper.php';

$helper = Helper::createInitedRun([
response_list(response_request_init([[['ok', []]]])),
response_list(response_request_exec([[['block', ['status_code' => '404', 'type' => 'html']]], ['{"found":"attack"}','{"another":"attack"}']])),
]);
rinit();

class SomeIntegration {
public function init()
{
DDTrace\install_hook("ltrim", self::hooked_function(), null);
}

private static function hooked_function()
{
return static function (DDTrace\HookData $hook) {
push_address("server.request.path_params", ["some" => "params", "more" => "parameters"]);
var_dump("This should be executed");
};
}
}

$integration = new SomeIntegration();
$integration->init();
echo "Something here to force partially booking";
var_dump(ltrim(" Calling wrapped function"));
var_dump("THIS SHOULD NOT GET IN THE OUTPUT");
?>
--EXPECTHEADERS--
Content-type: text/html; charset=UTF-8
--EXPECTF--
Something here to force partially booking
1 change: 1 addition & 0 deletions zend_abstract_interface/sandbox/php7/sandbox.c
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@ extern inline void zai_sandbox_open(zai_sandbox *sandbox);
extern inline void zai_sandbox_close(zai_sandbox *sandbox);
extern inline void zai_sandbox_bailout(zai_sandbox *sandbox);
extern inline bool zai_sandbox_timed_out(void);
extern inline bool zai_is_request_blocked(void);

extern inline void zai_sandbox_error_state_backup(zai_error_state *es);
extern inline void zai_sandbox_error_state_restore(zai_error_state *es);
1 change: 1 addition & 0 deletions zend_abstract_interface/sandbox/php8/sandbox.c
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ extern inline void zai_sandbox_open(zai_sandbox *sandbox);
extern inline void zai_sandbox_close(zai_sandbox *sandbox);
extern inline void zai_sandbox_bailout(zai_sandbox *sandbox);
extern inline bool zai_sandbox_timed_out(void);
extern inline bool zai_is_request_blocked(void);

extern inline void zai_sandbox_error_state_backup(zai_error_state *es);

42 changes: 39 additions & 3 deletions zend_abstract_interface/sandbox/sandbox.h
Original file line number Diff line number Diff line change
@@ -76,7 +76,9 @@
*
* This function should be invoked when a bailout has been caught.
*
* It will restore engine state and continue in all but the case of a timeout
* It will restore engine state and continue in all cases except:
* - Timeout
* - Request is blocked by the extension
*/

/* ######### Timeout ##########
@@ -218,8 +220,25 @@ inline bool zai_sandbox_timed_out(void) {
return false;
}

inline bool zai_is_request_blocked(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the redirection message as well.

{
if (!PG(last_error_message)) {
return false;
}

if (strcmp("Datadog blocked the request and presented a static error page", ZSTR_VAL(PG(last_error_message))) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern with this approach is that if someone changes the error message in appsec, it would stop working. Can you make changes on the appsec side to:

  • Make sure the error messages are validated at compile time?
  • Add comments specifying that the error message must not be changed.

return true;
}

if (strcmp("Datadog blocked the request, but the response has already been partially committed", ZSTR_VAL(PG(last_error_message))) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively you could check the prefix, which is always Datadog blocked the request

return true;
}

return false;
}

inline void zai_sandbox_bailout(zai_sandbox *sandbox) {
if (!zai_sandbox_timed_out()) {
if (!zai_sandbox_timed_out() && !zai_is_request_blocked()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why sandbox.h is written this way, with just C inline functions, but I guess that in the context of its strangeness it's fine.

zai_sandbox_engine_state_restore(&sandbox->engine_state);

return;
@@ -356,8 +375,25 @@ inline bool zai_sandbox_timed_out(void) {
return false;
}

inline bool zai_is_request_blocked(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

Both implementations of this function are exactly the same, perhaps you can define it only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are the same yes. However, this file is full of this duplication. I did it that way so follow the same approach

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there's a good technical reason, don't duplicate the function.

{
if (!PG(last_error_message)) {
return false;
}

if (strcmp("Datadog blocked the request and presented a static error page", ZSTR_VAL(PG(last_error_message))) == 0) {
return true;
}

if (strcmp("Datadog blocked the request, but the response has already been partially committed", ZSTR_VAL(PG(last_error_message))) == 0) {
return true;
}

return false;
}

inline void zai_sandbox_bailout(zai_sandbox *sandbox) {
if (!zai_sandbox_timed_out()) {
if (!zai_sandbox_timed_out() && !zai_is_request_blocked()) {
zai_sandbox_engine_state_restore(&sandbox->engine_state);

return;
3 changes: 0 additions & 3 deletions zend_abstract_interface/symbols/call.c
Original file line number Diff line number Diff line change
@@ -103,9 +103,6 @@ static inline int zai_symbol_try_call(zend_fcall_info *fci, zend_fcall_info_cach
ret = zend_call_function(fci, fcc);
} zend_catch {
ret = 2;
if (PG(last_error_message) && strstr(ZSTR_VAL(PG(last_error_message)), "Datadog blocked the request")) {
zend_bailout();
}
} zend_end_try();
return ret;
}
Loading