Don't leak OTP in logs #3123

shreyamalviya · 2023-03-17T13:50:40Z

What does this PR do?

Fixes a part of #3077

PR Checklist

Have you added an explanation of what your changes do and why you'd like to include them?
Is the TravisCI build passing?
~~Was the CHANGELOG.md updated to reflect the changes?~~
~~Was the documentation framework updated to reflect the changes?~~
Have you checked that you haven't introduced any duplicate code?

Testing Checklist

~~Added relevant unit tests?~~
Do all unit tests pass?
~~Do all end-to-end tests pass?~~
Any other testing performed?

Added dummy logs and checked that they were correctly formatted
~~If applicable, add screenshots or log transcripts of the feature working~~

codecov · 2023-03-17T13:56:16Z

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (fa7e27e) 71.66% compared to head (ccf0aba) 71.65%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3123      +/-   ##
===========================================
- Coverage    71.66%   71.65%   -0.01%     
===========================================
  Files          449      452       +3     
  Lines        12815    12850      +35     
===========================================
+ Hits          9184     9208      +24     
- Misses        3631     3642      +11

see 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mssalvatore

Cool idea! My concern, however, is there's no simple way to verify this automatically. Small changes could cause OTPs to leak and we might not notice for quite a while.

I think we can keep this in place for "defense-in-depth" purposes, but I think we need another approach as well.

Maybe commands should only be logged by command builder functions (which can be reused by different exploiters). The command builder function could log the command without the OTP redacted but return the entire command back to the caller.

Another option would be for command builders to return an object whose __str__() method redacts the OTP but you can call a specific method on the object to get back the full command. I kind of like this option. It adds more structure to the command and could be useful in the future.

VakarisZ · 2023-03-20T08:51:24Z

This solution seems a bit fragile. IF the OTP is logged in an error stack trace (without AGENT_OTP_ENVIRONMENT_VARIABLE_NAME) it will get logged plain text? The best solution I came up on top of my mind is to create a "ISecretVariable" interface. A specific implementation of that interface could use SecretStr. If we store the OTP in this type there's no chance it will get logged.

VakarisZ · 2023-03-21T13:27:52Z

monkey/common/utils/secret_variable.py

+from common.utils.i_secret_variable import ISecretVariable
+
+
+class SecretVariable(ISecretVariable):


Theoretically this should be called something like PydanticSecretVariable, but It's unlikely that we'll need multiple implementations of ISecretVariable, so I kept the name simple

If it's unlikely that we'll need multiple implementations, why do we have an interface at all?
If we do decide to keep it, I say we rename this to PydanticSecretVariable.

It somewhat achieves dependency inversion, which has all kinds of benefits and some drawbacks. The most trivial benefit is for example, that we would be able to create a test MockSecretVariable and pass it wherever ISecretVariable is expected. If there's no interface, we would have to mock the actual SecretVariable. You are right, we don't need the interface ATM, but I think it's better to have it and not need it, than to need it and not have it.

Fair enough. I think we should rename it then.

VakarisZ · 2023-03-21T13:39:03Z

monkey/infection_monkey/island_api_client/http_island_api_client.py

+            # Custom error message to avoid leaking the OTP
+            raise IslandAPIAuthenticationError("Failed to retrieve the authentication token")


We have no control over what happens in the HTTPClient, but we can suppress the error message

VakarisZ · 2023-03-21T13:41:28Z

monkey/infection_monkey/main.py

+    def format(self, record):
+        otp_regex = re.compile(f"{AGENT_OTP_ENVIRONMENT_VARIABLE}=[a-zA-Z0-9]*")
+        otp_replacement = f"{AGENT_OTP_ENVIRONMENT_VARIABLE}={'*' * 6}"


Do we still need this? I guess it's unlikely that f"{AGENT_OTP_ENVIRONMENT_VARIABLE}=[a-zA-Z0-9]*" will appear in the logs (because that's not the __repr__ of the OTP)

It provides us with defense-in-depth. Unless it's causing issues, I vote we leave it.

That's what I'm saying, it looks like the "depth" we gain is not worth the code we want to add. We are always running against the clock, maintaining the "depth" is not always worth it.

mssalvatore

Looks good, but rebase on develop.

monkey/common/utils/secret_variable.py

…n logs

This secret variable is a utility to gide output of variables into logs

Casting OTP and using it as a SecretVariable means that OTP won't get logged

Theoretically SecretVariable should support any data type, intergace should reflect it

mssalvatore requested changes Mar 17, 2023

View reviewed changes

VakarisZ reviewed Mar 21, 2023

View reviewed changes

VakarisZ force-pushed the 3077-do-not-leak-otp-in-logs branch from 30ab2fe to 20300c0 Compare March 21, 2023 13:29

VakarisZ reviewed Mar 21, 2023

View reviewed changes

mssalvatore approved these changes Mar 21, 2023

View reviewed changes

shreyamalviya commented Mar 22, 2023

View reviewed changes

monkey/common/utils/secret_variable.py Show resolved Hide resolved

shreyamalviya and others added 8 commits March 22, 2023 11:07

Agent: Add OTPFormatter class in main.py

f1b132e

Agent: Modify logger to use OTPFormatter so that OTPs aren't leaked i…

75f9d58

…n logs

Common: Add secret variable

797d677

This secret variable is a utility to gide output of variables into logs

Common: Add OTP type

56428a4

Agent: Cast OTP to a SecretVariable

2c3b7cc

Casting OTP and using it as a SecretVariable means that OTP won't get logged

Common: Change "get_secret_value" return type to Any

9185d73

Theoretically SecretVariable should support any data type, intergace should reflect it

UT: Remove unnecessary comments in test_secret_variable.py

3c737db

Common: Change the OTP typehint to an interface

d7e6869

VakarisZ force-pushed the 3077-do-not-leak-otp-in-logs branch from 3a7300a to d7e6869 Compare March 22, 2023 09:17

Agent: Catch all authentication errors to not leak the OTP

ccf0aba

VakarisZ force-pushed the 3077-do-not-leak-otp-in-logs branch from 8b0b79c to ccf0aba Compare March 22, 2023 10:20

VakarisZ merged commit 6fd780d into develop Mar 22, 2023

VakarisZ deleted the 3077-do-not-leak-otp-in-logs branch March 22, 2023 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't leak OTP in logs #3123

Don't leak OTP in logs #3123

shreyamalviya commented Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

mssalvatore left a comment

VakarisZ commented Mar 20, 2023

VakarisZ Mar 21, 2023 •

edited

Loading

shreyamalviya Mar 22, 2023

VakarisZ Mar 22, 2023

shreyamalviya Mar 22, 2023

VakarisZ Mar 21, 2023

VakarisZ Mar 21, 2023

mssalvatore Mar 21, 2023

VakarisZ Mar 22, 2023

mssalvatore left a comment

		from common.utils.i_secret_variable import ISecretVariable


		class SecretVariable(ISecretVariable):

		# Custom error message to avoid leaking the OTP
		raise IslandAPIAuthenticationError("Failed to retrieve the authentication token")

Don't leak OTP in logs #3123

Don't leak OTP in logs #3123

Conversation

shreyamalviya commented Mar 17, 2023 • edited Loading

What does this PR do?

PR Checklist

Testing Checklist

codecov bot commented Mar 17, 2023 • edited Loading

Codecov Report

mssalvatore left a comment

Choose a reason for hiding this comment

VakarisZ commented Mar 20, 2023

VakarisZ Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

shreyamalviya Mar 22, 2023

Choose a reason for hiding this comment

VakarisZ Mar 22, 2023

Choose a reason for hiding this comment

shreyamalviya Mar 22, 2023

Choose a reason for hiding this comment

VakarisZ Mar 21, 2023

Choose a reason for hiding this comment

VakarisZ Mar 21, 2023

Choose a reason for hiding this comment

mssalvatore Mar 21, 2023

Choose a reason for hiding this comment

VakarisZ Mar 22, 2023

Choose a reason for hiding this comment

mssalvatore left a comment

Choose a reason for hiding this comment

shreyamalviya commented Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

VakarisZ Mar 21, 2023 •

edited

Loading