Validate ransomware target directories #1288

shreyamalviya · 2021-07-01T08:14:08Z

What does this PR do?

#1239

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:

Ran and tested on Island (see video below).
If applicable, add screenshots or log transcripts of the feature working

ransomware-dir-validator.mp4

ghost · 2021-07-01T08:14:18Z

DeepCode's analysis on #4bec95 found:

⚠️ 1 warning 👇

Top issues

Description	Example fixes
Two subsequent branches of an if statement have duplicate bodies (here and here). This may be caused by a copy-paste error. If this usage is intentional, consider merging the branches to avoid code duplication. Occurrences: ValidationErrorMessages.js:15	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

codecov · 2021-07-01T08:16:05Z

Codecov Report

Merging #1288 (4bec957) into develop (f698c88) will decrease coverage by 1.33%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1288      +/-   ##
===========================================
- Coverage    30.68%   29.34%   -1.34%     
===========================================
  Files          449      452       +3     
  Lines        13470    14286     +816     
===========================================
+ Hits          4133     4192      +59     
- Misses        9337    10094     +757

Impacted Files	Coverage Δ
monkey/common/common_consts/validation_formats.py	`100.00% <100.00%> (ø)`
...key_island/cc/services/config_schema/ransomware.py	`100.00% <100.00%> (ø)`
monkey/infection_monkey/exploit/tools/helpers.py	`32.00% <0.00%> (-4.37%)`	⬇️
monkey/infection_monkey/exploit/wmiexec.py	`23.63% <0.00%> (-3.77%)`	⬇️
monkey/monkey_island/cc/app.py	`0.00% <0.00%> (ø)`
monkey/infection_monkey/dropper.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/cc/server_setup.py	`0.00% <0.00%> (ø)`
monkey/infection_monkey/exploit/hadoop.py	`0.00% <0.00%> (ø)`
monkey/infection_monkey/exploit/vsftpd.py	`0.00% <0.00%> (ø)`
monkey/infection_monkey/model/__init__.py	`100.00% <0.00%> (ø)`
... and 13 more

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 f698c88...4bec957. Read the comment docs.

VakarisZ

Having invalid defaults makes no sense. Empty fields should probably be valid for no encryption (especially if we're planning to remove the checkmarks in the future)

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

VakarisZ · 2021-07-02T08:12:05Z

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

@@ -1,13 +1,22 @@
 const ipRegex = '((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)'
 const cidrNotationRegex = '([0-9]|1[0-9]|2[0-9]|3[0-2])'
 const hostnameRegex = '^([A-Za-z0-9]*[A-Za-z]+[A-Za-z0-9]*.?)*([A-Za-z0-9]*[A-Za-z]+[A-Za-z0-9]*)$'
+// path starts with `/` OR `$`
+const linuxDirRegex = '^/|\\$'


afdbfasd$ is a valid absolute path according to this regex. Maybe change to ^/|^\\$?

VakarisZ · 2021-07-02T08:51:54Z

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

+  [VALID_DIR_LINUX]: buildValidDirLinuxRegex(),
+  [VALID_DIR_WINDOWS]: buildValidDirWindowsRegex()


This doesn't check if it's a valid directory. Doesn't even check if it's a directory, the regex would match a file as well. Maybe rename to getRegexWindowsStartsLikeAbsolutePath or getRegexWindowsIsAbsolutePath something?

VakarisZ · 2021-07-02T08:53:28Z

monkey/common/common_consts/validation_formats.py

+VALID_DIR_LINUX = "valid-directory-linux"
+VALID_DIR_WINDOWS = "valid-directory-windows"


Same as the other comment, the name is misleading as this validator doesn't check if the directory is valid, it only checks if the start of the path looks like the start of an absolute path.

VakarisZ

The regex bug and invalid-input-by-default are a must fixes, everything else optional

…y paths

… rename related stuff to accurately describe it

VakarisZ · 2021-07-02T12:45:11Z

This doesn't make sense as a valid ABS path, the whitespace should be either ignored or invalid as a starting character:

shreyamalviya · 2021-07-02T16:21:37Z

@VakarisZ shows me invalid

mssalvatore · 2021-07-05T11:38:18Z

monkey/infection_monkey/ransomware/ransomware_payload.py

@@ -32,7 +32,7 @@ def __init__(self, config: dict, telemetry_messenger: ITelemetryMessenger):
            target_directories["windows_target_dir"]
            if is_windows_os()
            else target_directories["linux_target_dir"]
-        )
+        ).strip()


At least on Linux, it is valid, if uncommon, to have a space at the end of a directory.

ubuntu@appimage:~/testdir$ ls ubuntu@appimage:~/testdir$ mkdir "space_test " ubuntu@appimage:~/testdir$ cd space_test -su: cd: space_test: No such file or directory ubuntu@appimage:~/testdir$ ls 'space_test ' ubuntu@appimage:~/testdir$ cd "space_test " ubuntu@appimage:~/testdir/space_test $ pwd /home/ubuntu/testdir/space_test

We shouldn't be preventing valid directories from being used, no matter how unusual they might be. What was the motivation for this change?

If the user accidentally leaves a space before or after the path, the payload won't run at all. But since, like you said, we shouldn't be preventing valid directories from being used, no matter how unusual they might be, how about we add a check in the UI and show a warning message if there are stray spaces?

A space before the path will be flagged, since this isn't an absolute path. A space after is trickier, since we don't really have a mechanism to distinguish a warning from something invalid. I say we leave it as is. After all, we don't spell check the directory names to check for typos. We can revisit if we get support requests or complaints from users.

Windows doesn't care about space after.

mssalvatore · 2021-07-05T12:10:06Z

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

+const linuxPathStartsWithTilde = '^~' // path starts with `~`
+
+const windowsAbsolutePathRegex = '^([A-Za-z]:(\\\\|\\/))' // path starts like `C:\` OR `C:/`
+const windowsPathStartsWithEnvVariableRegex = '^\\$|^(%\\w*\\d*\\s*%)' // path starts like `$` OR `%abc%`


These are the rules I found regarding the naming of Windows environment variables:

Variables have a percent sign on both sides: %ThisIsAVariable% The variable name can include spaces, punctuation and mixed case: %_Another Ex.ample% (This is unlike Parameter variables which only have one % sign and are always one character long: %A ) A variable name may include any of the following characters: A-Z, a-z, 0-9, # $ ' ( ) * + , - . ? @ [ ] _ ` { } ~ The first character of the name must not be numeric.

https://ss64.com/nt/syntax-variables.html

So we need something more like

Suggested change

const windowsPathStartsWithEnvVariableRegex = '^\\$|^(%\\w*\\d*\\s*%)' // path starts like `$` OR `%abc%`

const windowsEnvVarNonNumeric = '^%[A-Za-z0-9#\\$\'\$\$\\*\\+,-\\.\\?@\\[ \\]_`{}~]+ '

const windowsPathStartsWithEnvVariableRegex = `^\\\$|^(%${windowsEnvVarNonNumeric}+($windowsEnvVarNonNumeric|[0-9])*%)` // path starts like `$` OR `%abc%`

Note: The above is untested.

Fix committed: 6edca9a

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

mssalvatore · 2021-07-05T12:27:42Z

monkey/tests/unit_tests/infection_monkey/ransomware/test_ransomware_payload.py

+    ] = path_with_whitespaces
+
+    assert (
+        RansomwarePayload(ransomware_payload_config, telemetry_messenger_spy)._target_dir


We should avoid testing private variables and methods. Unit tests should test the interface, not the inner workings.

This may be moot, however, as the path on line 52 is valid:

ubuntu@appimage:~/testdir/space_test $ mkdir -p " t1/t2/t3 " ubuntu@appimage:~/testdir/space_test $ cd !$ cd " t1/t2/t3 " ubuntu@appimage:~/testdir/space_test / t1/t2/t3 $

We shouldn't be preventing the user from using valid paths, no matter how obnoxiously they've used spaces..

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

shreyamalviya · 2021-07-06T07:17:15Z

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

-const whitespacesOnlyRegex = '^\\s*$'
+
+const windowsAbsolutePathRegex = /^([A-Za-z]:(\\|\/))/ // path starts like `C:\` OR `C:/`
+const windowsEnvVarNonNumeric = '[A-Za-z#\\$\'\\(\\)\\*\\+,-\\.\\?@\\[\\]_`\\{\\}~+ ]'


What does the + in the end do? We're already checking for + with \\+.

shreyamalviya · 2021-07-06T07:44:58Z

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js

-const whitespacesOnlyRegex = '^\\s*$'
+
+const windowsAbsolutePathRegex = /^([A-Za-z]:(\\|\/))/ // path starts like `C:\` OR `C:/`
+const windowsEnvVarNonNumeric = '[A-Za-z#\\$\'\\(\\)\\*\\+,-\\.\\?@\\[\\]_`\\{\\}~+ ]'


Is windowsEnvVarNonNumeric the best name for this? Can file/directory names also contain the same characters? If yes, let's rename this.

"?" characters are not allowed in file/directory names, but are allowed in environment variable names.

…t directory

Refactored because the escape characters were cumbersome and difficult to read when regexes were defined as strings. Also allow special characters in Windows environment variable names as per https://ss64.com/nt/syntax-variables.html

As far as I can tell, environment variables in Windows look like %NAME%. Variables in powershell begin with $, but file explorer doesn't recognize paths beginning with $ as valid.

Co-authored-by: Shreya Malviya <[email protected]>

shreyamalviya added 4 commits July 1, 2021 13:30

common: Add validator constants for valid ransomware directory paths

0a1782a

island: Add ransomware directory path validators to ransomware schema

73c61eb

cc: Add ransomware directory path validation error messages

8af93c4

cc: Add regex validators for ransomware directory path validation

3d48a11

shreyamalviya requested review from mssalvatore and VakarisZ July 1, 2021 08:14

shreyamalviya requested a review from ilija-lazoroski July 1, 2021 08:14

VakarisZ reviewed Jul 2, 2021

View reviewed changes

monkey/monkey_island/cc/ui/src/components/configuration-components/ValidationFormats.js Outdated Show resolved Hide resolved

VakarisZ reviewed Jul 2, 2021

View reviewed changes

VakarisZ requested changes Jul 2, 2021

View reviewed changes

shreyamalviya added 3 commits July 2, 2021 16:04

cc: Fix regex bug when validating ransomware target directories

1768c0c

cc: Make whitespace-only a valid input for ransomware target director…

54072b6

…y paths

cc, common: Split ransomware dir path validator regex expressions and…

3496c71

… rename related stuff to accurately describe it

shreyamalviya requested a review from VakarisZ July 2, 2021 11:16

mssalvatore requested changes Jul 5, 2021

View reviewed changes

mssalvatore force-pushed the ransomware-target-dir-validators branch from 845d7f6 to b1d8d7d Compare July 5, 2021 18:00

shreyamalviya commented Jul 6, 2021

View reviewed changes

shreyamalviya and others added 5 commits July 6, 2021 09:38

cc: Add validation format (starts wih ~) for ransomware linux targe…

dc305d8

…t directory

Island: Do not allow Windows ransomware target paths beginning with "$"

9d4ee88

As far as I can tell, environment variables in Windows look like %NAME%. Variables in powershell begin with $, but file explorer doesn't recognize paths beginning with $ as valid.

Island: Allow Windows ransomware target paths to be UNC paths

d2dda45

Island: Escape '-' character in environment variable regex

638db3d

Co-authored-by: Shreya Malviya <[email protected]>

Island: Remove extra + from windows environment variable regex

4bec957

mssalvatore force-pushed the ransomware-target-dir-validators branch from 2dcf2f7 to 4bec957 Compare July 6, 2021 13:38

mssalvatore approved these changes Jul 6, 2021

View reviewed changes

mssalvatore merged commit b1ab252 into develop Jul 6, 2021

mssalvatore deleted the ransomware-target-dir-validators branch July 29, 2021 14:14

shreyamalviya mentioned this pull request Feb 9, 2023

Configuration form validators don't work #2958

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate ransomware target directories #1288

Validate ransomware target directories #1288

shreyamalviya commented Jul 1, 2021 •

edited

Loading

ghost commented Jul 1, 2021 •

edited by ghost

Loading

codecov bot commented Jul 1, 2021 •

edited

Loading

VakarisZ left a comment

VakarisZ Jul 2, 2021

VakarisZ Jul 2, 2021

VakarisZ Jul 2, 2021

VakarisZ left a comment •

edited

Loading

VakarisZ commented Jul 2, 2021

shreyamalviya commented Jul 2, 2021

mssalvatore Jul 5, 2021

shreyamalviya Jul 6, 2021

mssalvatore Jul 6, 2021 •

edited

Loading

ilija-lazoroski Jul 6, 2021

mssalvatore Jul 5, 2021

mssalvatore Jul 5, 2021

mssalvatore Jul 5, 2021

shreyamalviya Jul 6, 2021

shreyamalviya Jul 6, 2021

mssalvatore Jul 6, 2021

		[VALID_DIR_LINUX]: buildValidDirLinuxRegex(),
		[VALID_DIR_WINDOWS]: buildValidDirWindowsRegex()

		VALID_DIR_LINUX = "valid-directory-linux"
		VALID_DIR_WINDOWS = "valid-directory-windows"

	const windowsPathStartsWithEnvVariableRegex = '^\\$\|^(%\\w\\d\\s*%)' // path starts like `$` OR `%abc%`
	const windowsEnvVarNonNumeric = '^%[A-Za-z0-9#\\$\'\\(\\)\\*\\+,-\\.\\?@\\[ \\]_`{}~]+ '
	const windowsPathStartsWithEnvVariableRegex = `^\\\$\|^(%${windowsEnvVarNonNumeric}+($windowsEnvVarNonNumeric\|[0-9])*%)` // path starts like `$` OR `%abc%`

Validate ransomware target directories #1288

Validate ransomware target directories #1288

Conversation

shreyamalviya commented Jul 1, 2021 • edited Loading

What does this PR do?

PR Checklist

Testing Checklist

ghost commented Jul 1, 2021 • edited by ghost Loading

DeepCode's analysis on #4bec95 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

codecov bot commented Jul 1, 2021 • edited Loading

Codecov Report

VakarisZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VakarisZ left a comment • edited Loading

Choose a reason for hiding this comment

VakarisZ commented Jul 2, 2021

shreyamalviya commented Jul 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssalvatore Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyamalviya commented Jul 1, 2021 •

edited

Loading

ghost commented Jul 1, 2021 •

edited by ghost

Loading

codecov bot commented Jul 1, 2021 •

edited

Loading

VakarisZ left a comment •

edited

Loading

mssalvatore Jul 6, 2021 •

edited

Loading