Agent: Change trap command signal to TERM #1440

ilija-lazoroski · 2021-09-01T16:32:05Z

What does this PR do?

Fixes #1406 .

Trap command was showing no output when run from Island because the kill command never sends it.
As the technique allows for any kind of interrupt signals ( check this and this ) I have changed the INT with TERM.

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?

Testing Checklist

~~Added relevant unit tests?~~
Have you successfully tested your changes locally? Elaborate:

Tested by {Running the Monkey locally with relevant config/running Island/...}
If applicable, add screenshots or log transcripts of the feature working

Explain Changes

codecov · 2021-09-01T16:40:00Z

Codecov Report

Merging #1440 (9dfbab8) into develop (86fd735) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #1440      +/-   ##
===========================================
- Coverage    40.31%   40.26%   -0.05%     
===========================================
  Files          464      466       +2     
  Lines        13899    14023     +124     
===========================================
+ Hits          5603     5647      +44     
- Misses        8296     8376      +80

Impacted Files	Coverage Δ
...ion_monkey/post_breach/actions/use_trap_command.py	`0.00% <0.00%> (ø)`
...s/config_schema/definitions/post_breach_actions.py	`100.00% <ø> (ø)`
...infection_monkey/exploit/powershell_utils/utils.py	`91.11% <0.00%> (-8.89%)`	⬇️
monkey/infection_monkey/exploit/powershell.py	`0.00% <0.00%> (ø)`
.../exploit/powershell_utils/credential_generation.py	`100.00% <0.00%> (ø)`
...on_monkey/exploit/powershell_utils/auth_options.py	`100.00% <0.00%> (ø)`
monkey/infection_monkey/utils/windows/users.py	`29.67% <0.00%> (+5.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86fd735...9dfbab8. Read the comment docs.

mssalvatore · 2021-09-02T00:20:40Z

CHANGELOG.md

@@ -62,8 +62,7 @@ Changelog](https://keepachangelog.com/en/1.0.0/).
  instead of $HOME. #1143
 - Put environment config options in `server_config.json` into a separate
  section named "environment". #1161
- Automatically register if BlackBox tests are run on a fresh installation.
-  #1180
+- Automatically register if BlackBox tests are run on a fresh installation. #1180


I've been trying to keep the lines in the changelog <= 80 characters.

This was done by black I think. Cause the previous one looked like 1180 is a header.

Black shouldn't be formatting markdown files.

mssalvatore · 2021-09-02T00:21:59Z

CHANGELOG.md

@@ -77,6 +76,7 @@ Changelog](https://keepachangelog.com/en/1.0.0/).
 - Improve runtime of some unit tests. #1125
 - Run curl OR wget (not both) when attempting to communicate as a new user on
  Linux. #1407
+- Changed trap command signal from INT to TERM. #1406


You've put this under v1.11.0. This needs to go under "Unreleased".

In addition, put it under "fixed", and just say we fixed the trap post-breach action not producing any output. The specific signal we're using is too low level. Users don't care.

mssalvatore · 2021-09-02T00:22:39Z

monkey/infection_monkey/post_breach/trap_command/linux_trap_command.py

-        "trap 'echo \"Successfully used trap command\"' INT && kill -2 $$ ;",
-        "trap - INT",  # untrap SIGINT
+        # trap and send SIGTERM signal
+        "trap 'echo \"Successfully used trap command\"' TERM && kill -15 $$ ; ",


Is the extra space after the ; at the end of this string necessary?

VakarisZ

I don't understand. So why does the signal get sent from the manual run but not from island?

VakarisZ

Approved, but fix Mike's comments

VakarisZ · 2021-09-02T07:23:33Z

CHANGELOG.md

@@ -62,8 +62,7 @@ Changelog](https://keepachangelog.com/en/1.0.0/).
  instead of $HOME. #1143
 - Put environment config options in `server_config.json` into a separate
  section named "environment". #1161
- Automatically register if BlackBox tests are run on a fresh installation.
-  #1180
+- Automatically register if BlackBox tests are run on a fresh installation. #1180


Suggested change

- Automatically register if BlackBox tests are run on a fresh installation. #1180

- Automatically register if BlackBox tests are run on a fresh

installation. #1180

shreyamalviya

Same question as Vakaris.
Tested with binaries?

ilija-lazoroski · 2021-09-02T11:43:41Z

The reason which I think that the trap command was not working from the island is the following: When running from the Island we create a child process in which we then check the output of the trap command which sends SIGINT or INT and the response is empty because for some reason kill -2 $$ (make INT signal so the trap can caught it) is never called. The reason why is it not caught is not well documented, so a little research showed that a child process can't make INT signal, but it can send SIGTERM signal. Again there is not much of documentation for this so if anyone thinks that this is not okay, I will find another solution.

ilija-lazoroski requested review from mssalvatore, VakarisZ and shreyamalviya September 1, 2021 16:32

mssalvatore requested changes Sep 2, 2021

View reviewed changes

VakarisZ reviewed Sep 2, 2021

View reviewed changes

VakarisZ approved these changes Sep 2, 2021

View reviewed changes

VakarisZ reviewed Sep 2, 2021

View reviewed changes

Agent: Change trap command signal to TERM

9dfbab8

ilija-lazoroski force-pushed the trap-command-rework branch from 8ec8545 to 9dfbab8 Compare September 2, 2021 07:32

shreyamalviya reviewed Sep 2, 2021

View reviewed changes

mssalvatore merged commit cd9d5b4 into develop Sep 2, 2021

mssalvatore deleted the trap-command-rework branch September 2, 2021 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Agent: Change trap command signal to TERM #1440

Agent: Change trap command signal to TERM #1440

Uh oh!

ilija-lazoroski commented Sep 1, 2021

Uh oh!

codecov bot commented Sep 1, 2021 •

edited

Loading

Uh oh!

mssalvatore Sep 2, 2021

Uh oh!

ilija-lazoroski Sep 2, 2021

Uh oh!

mssalvatore Sep 2, 2021

Uh oh!

mssalvatore Sep 2, 2021

Uh oh!

mssalvatore Sep 2, 2021

Uh oh!

VakarisZ left a comment

Uh oh!

VakarisZ left a comment

Uh oh!

VakarisZ Sep 2, 2021

Uh oh!

shreyamalviya left a comment

Uh oh!

ilija-lazoroski commented Sep 2, 2021

Uh oh!

Uh oh!

	- Automatically register if BlackBox tests are run on a fresh installation. #1180
	- Automatically register if BlackBox tests are run on a fresh
	installation. #1180

Agent: Change trap command signal to TERM #1440

Agent: Change trap command signal to TERM #1440

Uh oh!

Conversation

ilija-lazoroski commented Sep 1, 2021

What does this PR do?

PR Checklist

Testing Checklist

Explain Changes

Uh oh!

codecov bot commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mssalvatore Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

ilija-lazoroski Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

mssalvatore Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

mssalvatore Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

mssalvatore Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

VakarisZ left a comment

Choose a reason for hiding this comment

Uh oh!

VakarisZ left a comment

Choose a reason for hiding this comment

Uh oh!

VakarisZ Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

shreyamalviya left a comment

Choose a reason for hiding this comment

Uh oh!

ilija-lazoroski commented Sep 2, 2021

Uh oh!

Uh oh!

codecov bot commented Sep 1, 2021 •

edited

Loading