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

feat(agent): add support for csec php agent #918

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion agent/config.m4
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ if test "$PHP_NEWRELIC" = "yes"; then
php_pdo_mysql.c php_pdo_pgsql.c php_pgsql.c php_psr7.c php_redis.c \
php_rinit.c php_rshutdown.c php_samplers.c php_stack.c \
php_stacked_segment.c php_txn.c php_user_instrument.c \
php_user_instrument_hashmap.c php_vm.c php_wrapper.c"
php_user_instrument_hashmap.c php_vm.c php_wrapper.c csec_metadata.c"
FRAMEWORKS="fw_cakephp.c fw_codeigniter.c fw_drupal8.c \
fw_drupal.c fw_drupal_common.c fw_joomla.c fw_kohana.c \
fw_laminas3.c fw_laravel.c fw_laravel_queue.c fw_lumen.c \
Expand Down
48 changes: 48 additions & 0 deletions agent/csec_metadata.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#include "csec_metadata.h"
#include "util_strings.h"
#include "php_hash.h"
#include "php_api_internal.h"

static void nr_csec_php_add_assoc_string_const(zval* arr,
const char* key,
const char* value) {
char* val = NULL;

if (NULL == arr || NULL == key || NULL == value) {
return;
}

val = nr_strdup(value);
nr_php_add_assoc_string(arr, key, val);
nr_free(val);
}

#ifdef TAGS
void zif_newrelic_get_security_metadata(void); /* ctags landing pad only */
void newrelic_get_security_metadata(void); /* ctags landing pad only */
#endif
PHP_FUNCTION(newrelic_get_security_metadata) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this function need to be called from PHP?

Copy link
Author

Choose a reason for hiding this comment

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

No but this function will be called by the Security Agent

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case then this shouldn't be a PHP user function but a function exported by newrelic-php-agent shared object, callable from a program by the means of dynamic binding - see https://linux.die.net/man/3/dlsym.

Copy link
Member

Choose a reason for hiding this comment

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

newrelic-php-agent already does similar thing - see here.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Michal, I will update the code and not expose this as a PHP user function. Thanks for the reference too.

Copy link
Author

Choose a reason for hiding this comment

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

I tried the dynamic call using dlopen and dlsym, but seems like the values may not be accessible.
nr_app_get_entity_name(NRPRG(app)) is one of the calls which is required, but NRPRG is the zend module's address space which does not seem accessible directly.

fprintf(stderr, "%p : newrelic_globals.app\n", NRPRG(app));

gives (nil) when called from dlopen'ed binary while from inside a valid address is printed.

Lets connect over call to finalise a solution.

Copy link
Member

Choose a reason for hiding this comment

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

@anmol-ap Please take a look at #922 - it should have what's needed.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the special purpose value for handle worked. Thanks.


NR_UNUSED_RETURN_VALUE;
NR_UNUSED_RETURN_VALUE_PTR;
NR_UNUSED_RETURN_VALUE_USED;
NR_UNUSED_THIS_PTR;
NR_UNUSED_EXECUTE_DATA;

array_init(return_value);

nr_csec_php_add_assoc_string_const(return_value, KEY_ENTITY_NAME, nr_app_get_entity_name(NRPRG(app)));
nr_csec_php_add_assoc_string_const(return_value, KEY_ENTITY_TYPE, nr_app_get_entity_type(NRPRG(app)));
nr_csec_php_add_assoc_string_const(return_value, KEY_ENTITY_GUID, nr_app_get_entity_guid(NRPRG(app)));
nr_csec_php_add_assoc_string_const(return_value, KEY_HOSTNAME, nr_app_get_host_name(NRPRG(app)));
nr_csec_php_add_assoc_string_const(return_value, KEY_LICENSE, NRPRG(license).value);

if (NRPRG(app)) {
nr_csec_php_add_assoc_string_const(return_value, KEY_AGENT_RUN_ID, NRPRG(app)->agent_run_id);
nr_csec_php_add_assoc_string_const(return_value, KEY_ACCOUNT_ID, NRPRG(app)->account_id);
nr_csec_php_add_assoc_string_const(return_value, KEY_PLICENSE, NRPRG(app)->plicense);
int high_security = NRPRG(app)->info.high_security;
add_assoc_long(return_value, KEY_HIGH_SECURITY, (long)high_security);
}

}
12 changes: 12 additions & 0 deletions agent/csec_metadata.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#include "php_agent.h"
#include "util_hashmap.h"

#define KEY_ENTITY_NAME "entity.name"
#define KEY_ENTITY_TYPE "entity.type"
#define KEY_ENTITY_GUID "entity.guid"
#define KEY_HOSTNAME "hostname"
#define KEY_AGENT_RUN_ID "agent.run.id"
#define KEY_ACCOUNT_ID "account.id"
#define KEY_LICENSE "license"
#define KEY_PLICENSE "plicense"
#define KEY_HIGH_SECURITY "high_security"
Comment on lines +4 to +12
Copy link
Member

@lavarou lavarou Jun 26, 2024

Choose a reason for hiding this comment

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

Are all these values really required by csec-agent? Why? When does the csec-agent going to use these values? Some of these values can change within a request if PHP code calls newrelic_set_appname

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the backend (Security Engine) uses these values to correctly map all the information for application, incidents, etc. csec-agent sends this information with the events.
If the value changes, as mentioned in the documentation for given method, same impact will be there. But we will not be missing any vulnerabilities due to this.

Copy link
Member

@lavarou lavarou Jun 27, 2024

Choose a reason for hiding this comment

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

Yes, the backend (Security Engine) uses these values to correctly map all the information for application, incidents, etc. csec-agent sends this information with the events.

I really doubt that backend will use plicense to map anything. Why is this particular value needed?

If the value changes, as mentioned in the documentation for given method, same impact will be there. But we will not be missing any vulnerabilities due to this.

I do not understand your answer. If csec-agent is going to retrieve this information before newrelic_set_appname is called in PHP code, and send the 'event' to Security Engine after newrelic_set_appname is called, then the 'event' will not be correctly mapped. When does csec-agent retrieve the metadata and when does csec-agent generate the events sent to the backend? This is very important design consideration that needs to be taken into account.

Copy link
Author

@anmol-ap anmol-ap Jun 27, 2024

Choose a reason for hiding this comment

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

We are logging some of these values, and that includes the license as well. Logging for license in particular has to be masked, and hence the reason for plicense. Unmasked license logging has previously being flagged at some agent.

At any point of time, when the apm agent is connected, entity.name is going to have a value along with entity.guid. When the newrelic_set_appname is called, csec-agent will be using that value on next reconnect cycle to the server. So, yes as soon as newrelic_set_appname is called, entity.name and entity.guid will not change for the csec-agent but on the next reconnect (this is planned at SE, the backend), the events will be mapped to new entity.name and entity.guid, this would reset the values for all the running and connected processes.

csec-agent tries to retrieve the metadata at the beginning of each request until it has data. It will wait for next reconnect cycle with server to see if anything has changed.

csec-agent generates the events based on the hooks and send the data immediately to the backend.

As has been detailed the intent of using newrelic_set_appname as early as possible in code, taking runtime values from APM agent would not affect the app's data at backend.

Copy link
Member

Choose a reason for hiding this comment

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

csec-agent tries to retrieve the metadata at the beginning of each request until it has data.

This is exactly why php-agent team has a problem with this. Not only there's potential for incorrect mapping of an event to an app, but also this depends on wether php-agent has been initialized before csec-agent, which we cannot see how it can be guaranteed. Why doesn't csec-agent retrieve the metadata only at the time it is needed, in the 'hook' you mention here:

csec-agent generates the events based on the hooks and send the data immediately to the backend.

Copy link
Author

@anmol-ap anmol-ap Jun 27, 2024

Choose a reason for hiding this comment

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

csec-agent takes the connection information from the apm agent only. It does not generate any data until the apm agent has been connected. csec-agent is completely reliant on APM agent for entity.guid.

Copy link
Author

Choose a reason for hiding this comment

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

And without entity.guid, it does not connect to the backend

2 changes: 2 additions & 0 deletions agent/php_api_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
extern PHP_FUNCTION(newrelic_get_request_metadata);

extern PHP_FUNCTION(newrelic_get_security_metadata);

#ifdef ENABLE_TESTING_API

/*
Expand Down
2 changes: 2 additions & 0 deletions agent/php_globals.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ typedef struct _nrphpglobals_t {
int instrument_internal; /* newrelic.transaction_tracer.internal_functions_enabled
*/
int high_security; /* newrelic.high_security */
bool nr_security_agent_enabled; /* newrelic.security.agent.enabled */
bool nr_security_enabled; /* newrelic.security.enabled */

int apache_major; /* Apache major version */
int apache_minor; /* Apache minor version */
Expand Down
7 changes: 7 additions & 0 deletions agent/php_minit.c
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,13 @@ PHP_MINIT_FUNCTION(newrelic) {
nr_wordpress_minit();
nr_php_set_opcode_handlers();

if (!NR_PHP_PROCESS_GLOBALS(nr_security_agent_enabled) || !NR_PHP_PROCESS_GLOBALS(nr_security_enabled) || NR_PHP_PROCESS_GLOBALS(high_security)) {
nrl_info(NRL_INIT, "New Relic Security is completely disabled by one of the user provided config `newrelic.security.enabled`, `newrelic.security.agent.enabled` or `newrelic.high_security`. Not loading security capabilities.");
nrl_debug(NRL_INIT, "newrelic.security.agent.enabled : %s", NR_PHP_PROCESS_GLOBALS(nr_security_enabled) ? "true" : "false");
nrl_debug(NRL_INIT, "newrelic.security.enabled : %s", NR_PHP_PROCESS_GLOBALS(nr_security_agent_enabled) ? "true" : "false");
nrl_debug(NRL_INIT, "newrelic.high_security : %s", NR_PHP_PROCESS_GLOBALS(high_security) ? "true" : "false");
}

nrl_debug(NRL_INIT, "MINIT processing done");
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP 7.4+ */
NR_PHP_PROCESS_GLOBALS(zend_offset) = zend_get_resource_handle(dummy);
Expand Down
2 changes: 2 additions & 0 deletions agent/php_newrelic.c
Original file line number Diff line number Diff line change
Expand Up @@ -343,9 +343,11 @@ static zend_function_entry newrelic_functions[] = {
#ifdef PHP8
PHP_FE(newrelic_get_linking_metadata, newrelic_arginfo_void)
PHP_FE(newrelic_get_trace_metadata, newrelic_arginfo_void)
PHP_FE(newrelic_get_security_metadata, newrelic_arginfo_void)
#else
PHP_FE(newrelic_get_linking_metadata, 0)
PHP_FE(newrelic_get_trace_metadata, 0)
PHP_FE(newrelic_get_security_metadata, 0)
#endif /* PHP 8 */
/*
* Integration test helpers
Expand Down
64 changes: 64 additions & 0 deletions agent/php_nrini.c
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,58 @@ static PHP_INI_MH(nr_high_security_mh) {
return SUCCESS;
}

static PHP_INI_MH(nr_security_enabled_mh) {
int val;

(void)entry;
(void)NEW_VALUE_LEN;
(void)mh_arg1;
(void)mh_arg2;
(void)mh_arg3;
(void)stage;
NR_UNUSED_TSRMLS;

val = nr_bool_from_str(NEW_VALUE);

if (-1 == val) {
return FAILURE;
}

if (val) {
NR_PHP_PROCESS_GLOBALS(nr_security_enabled) = true;
} else {
NR_PHP_PROCESS_GLOBALS(nr_security_enabled) = false;
}

return SUCCESS;
}

static PHP_INI_MH(nr_security_agent_enabled_mh) {
int val;

(void)entry;
(void)NEW_VALUE_LEN;
(void)mh_arg1;
(void)mh_arg2;
(void)mh_arg3;
(void)stage;
NR_UNUSED_TSRMLS;

val = nr_bool_from_str(NEW_VALUE);

if (-1 == val) {
return FAILURE;
}

if (val) {
NR_PHP_PROCESS_GLOBALS(nr_security_agent_enabled) = true;
} else {
NR_PHP_PROCESS_GLOBALS(nr_security_agent_enabled) = false;
}

return SUCCESS;
}

static PHP_INI_MH(nr_preload_framework_library_detection_mh) {
int val;

Expand Down Expand Up @@ -2009,6 +2061,18 @@ PHP_INI_ENTRY_EX("newrelic.high_security",
nr_high_security_mh,
0)

PHP_INI_ENTRY_EX("newrelic.security.agent.enabled",
"0",
NR_PHP_SYSTEM,
nr_security_agent_enabled_mh,
0)

PHP_INI_ENTRY_EX("newrelic.security.enabled",
"0",
NR_PHP_SYSTEM,
nr_security_enabled_mh,
0)

Comment on lines +2064 to +2075
Copy link
Member

Choose a reason for hiding this comment

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

Does the newrelic-php-agent need these settings? They don't seem to be used anywhere...

Copy link
Author

Choose a reason for hiding this comment

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

newrelic.security.agent.enabled and newrelic.security.enabled settings are logged in the APM log file if these settings are disabled in the minit function

Copy link
Member

Choose a reason for hiding this comment

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

Will the values of these settings be used by the security agent as well?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, these values are to be used by the security agent as well as mentioned here in comment.

/*
* Feature flag handling.
*/
Expand Down
83 changes: 83 additions & 0 deletions agent/scripts/newrelic.ini.template
Original file line number Diff line number Diff line change
Expand Up @@ -1332,3 +1332,86 @@ newrelic.daemon.logfile = "/var/log/newrelic/newrelic-daemon.log"
; for vulnerability management.
;
;newrelic.vulnerability_management.package_detection.enabled = true

; Setting: newrelic.security.enabled
; Type : boolean
; Scope : system
; Default: false
; Info : Indicates if attack detection security module is to be enabled
;
newrelic.security.enabled = true
anmol-ap marked this conversation as resolved.
Show resolved Hide resolved

; Setting: newrelic.security.mode
; Type : string
; Scope : system
; Default: "IAST"
; Info : Security module provides two modes "IAST" or "RASP"
; See documentation for more details
;
;newrelic.security.mode = "IAST"

; Setting: newrelic.security.validator_service_endpoint_url
; Type : string
; Scope : system
; Default: "wss://csec.nr-data.net"
; Info : New Relic<E2><80><99>s security module SaaS connection URLs
;
;newrelic.security.validator_service_url = "wss://csec.nr-data.net"

; Setting: newrelic.security.agent.enabled
; Type : boolean
; Scope : system
; Default: false
; Info : Used to enable security module, default false is equivalent to
; security module not even loaded. If this setting is set to true,
; then only security module is loaded and to enable it, a restart
; of application is required. This is different than
; newrelic.security.enabled, in terms that security.enabled decides
; runtime behavior of security module but security.agent.enabled
; would not even load the security module when set to false
;
;newrelic.security.agent.enabled = false

; Setting: newrelic.security.detection.rci.enabled
; Type : boolean
; Scope : system
; Default: true
; Info : Indicates if detection of remote code injection attack category is to be enabled
;
;newrelic.security.detection.rci.enabled = true

; Setting: newrelic.security.detection.rxss.enabled
; Type : boolean
; Scope : system
; Default: true
; Info : Indicates if detection of reflected xss attack category is to be enabled
;
;newrelic.security.detection.rxss.enabled = true

; Setting: newrelic.security.detection.deserialization.enabled
; Type : boolean
; Scope : system
; Default: true
; Info : Indicates if detection of deserialization attack category is to be enabled
;
;newrelic.security.detection.deserialization.enabled = true

; Setting: newrelic.security.request.body_limit
; Type : unsigned integer
; Scope : system
; Default: 300
; Info : Sets the maximum limit of the request body to be read in kb.
;
;newrelic.security.request.body_limit = 300

; Setting: newrelic.security.validator.client_ssl_cert_filepath
; Type : string
; Scope : system
; Default: none
; Info : Sets the full path of the client certificate in PEM
; format. When set, this certificate will be used to
; authenticate the New Relic IAST Security Engine's url. If
; not set, the default certificate will be used. Make
; sure the file permissions are correct.
;
;newrelic.security.validator.client_ssl_cert_filepath = ""
Copy link
Member

Choose a reason for hiding this comment

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

Why these settings are added? newrelic-php-agent is neither parsing nor using any of these settings.

Copy link
Author

Choose a reason for hiding this comment

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

newrelic-php-agent will be parsing newrelic.security.agent.enabled and newrelic.security.enabled settings and logging in the log file if these settings are disabled. That is consistent across other language agents also. And the other settings will be read by the Security agent and we do not want to introduce another config file.

Copy link
Member

Choose a reason for hiding this comment

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

newrelic-php-agent will be parsing newrelic.security.agent.enabled and newrelic.security.enabled settings and logging in the log file if these settings are disabled. That is consistent across other language agents also.

What is the meaning of these two flags? What is the difference between them?

And the other settings will be read by the Security agent and we do not want to introduce another config file.

Since newrelic-php-agent is not parsing any of those, I don't think they belong to the agent/scripts/newrelic.ini.template, which serves as a quick-access documentation of newrelic-php-agent settings. These settings must be properly documented in official documentation available on docs.newrelic.com. If Security agent wants to also provide a quick-access documentation for ini settings controlling its behavior, it should include its own ini template in the distribution.

Copy link
Author

Choose a reason for hiding this comment

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

What is the meaning of these two flags? What is the difference between them?

The newrelic.security.agent.enabled is a flag for not even loading the agent. But since php security agent is not a dependency like in other language agents, this flag tells the security agent to not perform instrumentation. A complete bypass of the agent. But this has been kept to keep the config consistent across all language agents.

The newrelic.security.enabled flag tells the php security agent to perform instrumentation and all the tasks for the security agent.

Since newrelic-php-agent is not parsing any of those, I don't think they belong to the

I understand this but IAST security agent is not a different agent for the customer. To the customer, there is a single agent which supports both APM and IAST. Therefore, we do not want a separate ini file for the security agent. Plus 2 settings mentioned above are already read and logged in APM agent.

15 changes: 15 additions & 0 deletions axiom/cmd_appinfo_transmit.c
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ nr_status_t nr_cmd_appinfo_process_reply(const uint8_t* data,
int reply_len;
const char* reply_json;
const char* entity_guid;
const char* account_id; /* Csec : Added for extracting account_id */

if ((NULL == data) || (0 == len)) {
return NR_FAILURE;
Expand Down Expand Up @@ -384,6 +385,20 @@ nr_status_t nr_cmd_appinfo_process_reply(const uint8_t* data,
app->entity_guid = NULL;
}

/*
* Csec : Added for extracting account_id
*/
nr_free(app->account_id);
account_id = nro_get_hash_string(app->connect_reply, "account_id", NULL);
if (NULL != account_id) {
app->account_id = nr_strdup(account_id);
} else {
app->account_id = NULL;
}
/*
* Csec : Added for extracting account_id
*/

nrl_debug(NRL_ACCT, "APPINFO reply full app='%.*s' agent_run_id=%s",
NRP_APPNAME(app->info.appname), app->agent_run_id);

Expand Down
1 change: 1 addition & 0 deletions axiom/nr_app.c
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ void nr_app_destroy(nrapp_t** app_ptr) {
nr_free(app->host_name);
nr_free(app->entity_guid);
nr_free(app->entity_name);
nr_free(app->account_id);
nr_rules_destroy(&app->url_rules);
nr_rules_destroy(&app->txn_rules);
nr_segment_terms_destroy(&app->segment_terms);
Expand Down
1 change: 1 addition & 0 deletions axiom/nr_app.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ typedef struct _nrapp_t {
char* host_name; /* Local host name reported to the daemon */
char* entity_name; /* Entity name related to this application */
char* entity_guid; /* Entity guid related to this application */
char* account_id; /* Security : Added for getting account id */
time_t last_daemon_query; /* Used by agent: Last time we queried daemon about
this app */
int failed_daemon_query_count; /* Used by agent: Number of times daemon query
Expand Down