From 20dfbbc26abd6a95db953ca04b85adcada172130 Mon Sep 17 00:00:00 2001 From: Yehuda Rosenberg <90599084+RosenbergYehuda@users.noreply.github.com> Date: Tue, 12 Mar 2024 21:52:47 +0200 Subject: [PATCH] Hide sensitive info from logs (#33224) * Revert "Sanitize Curl Logs (#31702)" This reverts commit 60a9393197da60176432c452015c687df669eb67. * init * more * try * try * fixes * add tests * more * RN * RN * fix * pre commit * fix tests * fix * fixes * fixes * add test * pre commit * python 2 * Python 2 #2 * Update Packs/Base/Scripts/CommonServerPython/CommonServerPython_test.py Co-authored-by: Jasmine Beilin <71636766+JasBeilin@users.noreply.github.com> * add description * pre commit --------- Co-authored-by: Jasmine Beilin <71636766+JasBeilin@users.noreply.github.com> --- Packs/Base/ReleaseNotes/1_33_39.md | 6 ++ .../CommonServerPython/CommonServerPython.py | 63 +++++++++++---- .../CommonServerPython_test.py | 77 +++++++++++++++++-- Packs/Base/pack_metadata.json | 2 +- 4 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 Packs/Base/ReleaseNotes/1_33_39.md diff --git a/Packs/Base/ReleaseNotes/1_33_39.md b/Packs/Base/ReleaseNotes/1_33_39.md new file mode 100644 index 000000000000..8f44f477aee6 --- /dev/null +++ b/Packs/Base/ReleaseNotes/1_33_39.md @@ -0,0 +1,6 @@ + +#### Scripts + +##### CommonServerPython + +Improved implementation to prevent sensitive information from being logged. diff --git a/Packs/Base/Scripts/CommonServerPython/CommonServerPython.py b/Packs/Base/Scripts/CommonServerPython/CommonServerPython.py index b6c6f28fa2fa..f6ab7aa775b5 100644 --- a/Packs/Base/Scripts/CommonServerPython/CommonServerPython.py +++ b/Packs/Base/Scripts/CommonServerPython/CommonServerPython.py @@ -49,8 +49,8 @@ def __line__(): ASSETS = "assets" EVENTS = "events" DATA_TYPES = [EVENTS, ASSETS] - -SECRET_REPLACEMENT_STRING = '' +MASK = '' +SEND_PREFIX = "send: b'" def register_module_line(module_name, start_end, line, wrapper=0): @@ -1632,7 +1632,7 @@ def encode(self, message): else: res = "Failed encoding message with error: {}".format(exception) for s in self.replace_strs: - res = res.replace(s, SECRET_REPLACEMENT_STRING) + res = res.replace(s, MASK) return res def __call__(self, message): @@ -1701,7 +1701,7 @@ def build_curl(self, text): :rtype: ``None`` """ http_methods = ['GET', 'POST', 'PUT', 'DELETE', 'PATCH'] - data = text.split("send: b'")[1] + data = text.split(SEND_PREFIX)[1] if data and data[0] in {'{', '<'}: # it is the request url query params/post body - will always come after we already have the url and headers # `<` is for xml body @@ -1711,7 +1711,6 @@ def build_curl(self, text): url = '' headers = [] headers_to_skip = ['Content-Length', 'User-Agent', 'Accept-Encoding', 'Connection'] - headers_to_sanitize = ['Authorization', 'Cookie'] request_parts = repr(data).split('\\\\r\\\\n') # splitting lines on repr since data is a bytes-string for line, part in enumerate(request_parts): if line == 0: @@ -1723,9 +1722,6 @@ def build_curl(self, text): else: if any(header_to_skip in part for header_to_skip in headers_to_skip): continue - if any(header_to_sanitize in part for header_to_sanitize in headers_to_sanitize): - headers.append(part.split(' ')[0] + " " + SECRET_REPLACEMENT_STRING) - continue headers.append(part) curl_headers = '' for header in headers: @@ -1757,12 +1753,18 @@ def write(self, msg): if self.buffering: self.messages.append(text) else: + if is_debug_mode(): + if text.startswith(('send:', 'header:')): + try: + text = censor_request_logs(text) + except Exception as e: # should fail silently + demisto.debug('Failed censoring request logs - {}'.format(str(e))) + if text.startswith('send:'): + try: + self.build_curl(text) + except Exception as e: # should fail silently + demisto.debug('Failed generating curl - {}'.format(str(e))) demisto.info(text) - if is_debug_mode() and text.startswith('send:'): - try: - self.build_curl(text) - except Exception as e: # should fail silently - demisto.debug('Failed generating curl - {}'.format(str(e))) self.write_buf = [] def print_override(self, *args, **kwargs): @@ -8387,6 +8389,41 @@ def emit(self, record): pass +def censor_request_logs(request_log): + """ + Censors the request logs generated from the urllib library directly by replacing sensitive information such as tokens and cookies with a mask. + In most cases, the sensitive value is the first word after the keyword, but in some cases, it is the second one. + :param request_log: The request log to censor + :type request_log: ``str`` + + :return: The censored request log + :rtype: ``str`` + """ + keywords_to_censor = ['Authorization:', 'Cookie', "Token"] + lower_keywords_to_censor = [word.lower() for word in keywords_to_censor] + + trimed_request_log = request_log.lstrip(SEND_PREFIX) + request_log_with_spaces = trimed_request_log.replace("\\r\\n", " \\r\\n") + request_log_lst = request_log_with_spaces.split() + + for i, word in enumerate(request_log_lst): + # Check if the word is a keyword or contains a keyword (e.g "Cookies" containes "Cookie") + if any(keyword in word.lower() for keyword in lower_keywords_to_censor): + next_word = request_log_lst[i + 1] if i + 1 < len(request_log_lst) else None + if next_word: + # If the next word is "Bearer" or "Basic" then we replace the word after it since thats the token + if next_word.lower() in ["bearer", "basic"] and i + 2 < len(request_log_lst): + request_log_lst[i + 2] = MASK + else: + request_log_lst[i + 1] = MASK + + # Rebuild the request log so that the only change is the masked information. + censored_string = SEND_PREFIX + \ + ' '.join(request_log_lst) if request_log.startswith(SEND_PREFIX) else ' '.join(request_log_lst) + censored_string = censored_string.replace(" \\r\\n", "\\r\\n") + return censored_string + + class DebugLogger(object): """ Wrapper to initiate logging at logging.DEBUG level. diff --git a/Packs/Base/Scripts/CommonServerPython/CommonServerPython_test.py b/Packs/Base/Scripts/CommonServerPython/CommonServerPython_test.py index 5786abb9feb8..032fb604efba 100644 --- a/Packs/Base/Scripts/CommonServerPython/CommonServerPython_test.py +++ b/Packs/Base/Scripts/CommonServerPython/CommonServerPython_test.py @@ -28,7 +28,7 @@ url_to_clickable_markdown, WarningsHandler, DemistoException, SmartGetDict, JsonTransformer, \ remove_duplicates_from_list_arg, DBotScoreType, DBotScoreReliability, Common, send_events_to_xsiam, ExecutionMetrics, \ response_to_context, is_integration_command_execution, is_xsiam_or_xsoar_saas, is_xsoar, is_xsoar_on_prem, \ - is_xsoar_hosted, is_xsoar_saas, is_xsiam, send_data_to_xsiam + is_xsoar_hosted, is_xsoar_saas, is_xsiam, send_data_to_xsiam, censor_request_logs, censor_request_logs EVENTS_LOG_ERROR = \ """Error sending new events into XSIAM. @@ -1452,7 +1452,7 @@ def test_build_curl_post_noproxy(): "Content-Type: application/json\\r\\n\\r\\n'") ilog.build_curl("send: b'{\"data\": \"value\"}'") assert ilog.curl == [ - 'curl -X POST https://demisto.com/api -H "Authorization: " -H "Content-Type: application/json" ' + 'curl -X POST https://demisto.com/api -H "Authorization: TOKEN" -H "Content-Type: application/json" ' '--noproxy "*" -d \'{"data": "value"}\'' ] @@ -1479,7 +1479,7 @@ def test_build_curl_post_xml(): "Content-Type: application/json\\r\\n\\r\\n'") ilog.build_curl("send: b''") assert ilog.curl == [ - 'curl -X POST https://demisto.com/api -H "Authorization: " -H "Content-Type: application/json" ' + 'curl -X POST https://demisto.com/api -H "Authorization: TOKEN" -H "Content-Type: application/json" ' '--noproxy "*" -d \'\'' ] @@ -1511,7 +1511,7 @@ def test_build_curl_get_withproxy(mocker): "Content-Type: application/json\\r\\n\\r\\n'") ilog.build_curl("send: b'{\"data\": \"value\"}'") assert ilog.curl == [ - 'curl -X GET https://demisto.com/api -H "Authorization: " -H "Content-Type: application/json" ' + 'curl -X GET https://demisto.com/api -H "Authorization: TOKEN" -H "Content-Type: application/json" ' '--proxy http://proxy -k -d \'{"data": "value"}\'' ] @@ -1548,9 +1548,9 @@ def test_build_curl_multiple_queries(): "Content-Type: application/json\\r\\n\\r\\n'") ilog.build_curl("send: b'{\"getdata\": \"value\"}'") assert ilog.curl == [ - 'curl -X POST https://demisto.com/api/post -H "Authorization: " -H "Content-Type: application/json" ' + 'curl -X POST https://demisto.com/api/post -H "Authorization: TOKEN" -H "Content-Type: application/json" ' '--noproxy "*" -d \'{"postdata": "value"}\'', - 'curl -X GET https://demisto.com/api/get -H "Authorization: " -H "Content-Type: application/json" ' + 'curl -X GET https://demisto.com/api/get -H "Authorization: TOKEN" -H "Content-Type: application/json" ' '--noproxy "*" -d \'{"getdata": "value"}\'' ] @@ -9554,3 +9554,68 @@ def test_create_clickable_test_wrong_text_value(): assert e.type == AssertionError assert 'The URL list and the text list must be the same length.' in e.value.args + + +@pytest.mark.parametrize("request_log, expected_output", [ + ( + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nmy_authorization: Bearer token123\\r\\n'", + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nmy_authorization: Bearer \\r\\n'" + ), + ( + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nSet_Cookie: session_id=123\\r\\n'", + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nSet_Cookie: \\r\\n'" + ), + ( + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nAuthorization: token123\\r\\n'", + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nAuthorization: \\r\\n'" + ), + ( + "GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nAuthorization: Bearer token123\\r\\n", + "GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\nAuthorization: Bearer \\r\\n" + ), + ( + "send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\n'", + str("send: b'GET /api/v1/users HTTP/1.1\\r\\nHost: example.com\\r\\n'") + ), +]) +def test_censor_request_logs(request_log, expected_output): + """ + Given: + A request log. + case 1: A request log with a sensitive data under the 'Authorization' header, but the 'Authorization' is not capitalized and within a string. + case 2: A request log with a sensitive data under the 'Cookie' header, but with a 'Set_Cookie' prefix. + case 3: A request log with a sensitive data under the 'Authorization' header, but with no 'Bearer' prefix. + case 4: A request log with a sensitive data under the 'Authorization' header, but with no 'send b' prefix at the beginning. + case 5: A request log with no sensitive data. + When: + Running censor_request_logs function. + Then: + Assert the function returns the exactly same log with the sensitive data masked. + """ + assert censor_request_logs(request_log) == expected_output + + +@pytest.mark.parametrize("request_log", [ + ('send: hello\n'), + ('header: Authorization\n') +]) +def test_logger_write__censor_request_logs_has_been_called(mocker, request_log): + """ + Given: + A request log that starts with 'send' or 'header' that may contains sensitive data. + When: + Running logger.write function when using debug-mode. + Then: + Assert the censor_request_logs function has been called. + """ + mocker.patch.object(demisto, 'params', return_value={ + 'credentials': {'password': 'my_password'}, + }) + mocker.patch.object(demisto, 'info') + mocker.patch('CommonServerPython.is_debug_mode', return_value=True) + mock_censor = mocker.patch('CommonServerPython.censor_request_logs') + mocker.patch('CommonServerPython.IntegrationLogger.build_curl') + ilog = IntegrationLogger() + ilog.set_buffering(False) + ilog.write(request_log) + assert mock_censor.call_count == 1 diff --git a/Packs/Base/pack_metadata.json b/Packs/Base/pack_metadata.json index 48ac238dc3e5..9f69790f7c3b 100644 --- a/Packs/Base/pack_metadata.json +++ b/Packs/Base/pack_metadata.json @@ -2,7 +2,7 @@ "name": "Base", "description": "The base pack for Cortex XSOAR.", "support": "xsoar", - "currentVersion": "1.33.38", + "currentVersion": "1.33.39", "author": "Cortex XSOAR", "serverMinVersion": "6.0.0", "url": "https://www.paloaltonetworks.com/cortex",