Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Modified Regex pattern to accept commands besides full executable path. #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

feldrim
Copy link

@feldrim feldrim commented Dec 26, 2018

Although it's the default behavior, the regex pattern added with commit 4948bd6 does not accept commands from environment paths, as I have mentioned on issue #6 . So I changed the pattern to accept commands like cmd. Tests are working but they are not ideal solutions, just workarounds. It needs some refactoring yet still capable of esting the condition.

@CLAassistant
Copy link

CLAassistant commented Dec 26, 2018

CLA assistant check
All committers have signed the CLA.

@andrewmd5
Copy link
Member

This will take some time to review due to the holidays. @healingbrew check when you have some time.

@yretenai
Copy link
Contributor

yretenai commented Dec 27, 2018

The regex seems to return C for C:\Windows\System32\cmd.exe

This is your regex:

"cmd"                                         = cmd
"garbled cmd garbled"                         = garbled
"cmd.exe"                                     = cmd
"garbled cmd.exe garbled"                     = garbled
"C:\cmd.exe"                                  = C
"garbled C:\cmd.exe garbled"                  = garbled
"C:\Windows\system32\cmd.exe"                 = C
"garbled C:\Windows\system32\cmd.exe garbled" = garbled
"\\server\cmd.exe"                            = \
"garbled \\server\cmd.exe garbled"            = garbled
""warden dot dll.exe""                        = "warden
"garbled "warden dot dll.exe" garbled"        = garbled

This is the current implementation

"cmd"                                         = NULL
"garbled cmd garbled"                         = NULL
"cmd.exe"                                     = NULL
"garbled cmd.exe garbled"                     = NULL
"C:\cmd.exe"                                  = C:\cmd.exe
"garbled C:\cmd.exe garbled"                  = C:\cmd.exe
"C:\Windows\system32\cmd.exe"                 = C:\Windows\system32\cmd.exe
"garbled C:\Windows\system32\cmd.exe garbled" = C:\Windows\system32\cmd.exe
"\\server\cmd.exe"                            = \\server\cmd.exe
"garbled \\server\cmd.exe garbled"            = \\server\cmd.exe
""warden dot dll.exe""                        = NULL
"garbled "warden dot dll.exe" garbled"        = NULL

This is our fault for not writing the tests, but this PR cannot be merged until this is fixed.
The entire regex is hugely inefficient and complicated.
It can be trimmed down to
((([\w])?[:%\\\/]|").*(\.[\w:]+|"|%))|([\w\d.\\:]*(\s|$)) (add path-safe characters like _-()&)
But that yields some quirks as well

"cmd"                                         = cmd
"garbled cmd garbled"                         = garbled 
"cmd.exe"                                     = cmd.exe
"garbled cmd.exe garbled"                     = garbled 
"C:\cmd.exe"                                  = C:\cmd.exe
"garbled C:\cmd.exe garbled"                  = garbled 
"C:\Windows\system32\cmd.exe"                 = C:\Windows\system32\cmd.exe
"garbled C:\Windows\system32\cmd.exe garbled" = garbled 
"\\server\cmd.exe"                            = \\server\cmd.exe
"garbled \\server\cmd.exe garbled"            = garbled 
""warden dot dll.exe""                        = "warden dot dll.exe"
"garbled "warden dot dll.exe" garbled"        = garbled

So you can either split it up into two regexes (((([\w])?[:%\\\/]|").*(\.[\w:]+|"|%)) and ([\w\d.\\:]*(\s|$)) as a fallback which is easier to maintain I guess) or figure prioritization out.

@yretenai yretenai self-requested a review December 27, 2018 07:58
Copy link
Contributor

@yretenai yretenai left a comment

Choose a reason for hiding this comment

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

Regex fails to sanitize garbled path arguments such as lorem ipsum cmd.exe dolor and lorem ipsum C:\cmd.exe dolor, or any path starting with or containing a non-alphanumeric character.

@feldrim
Copy link
Author

feldrim commented Dec 27, 2018

Oh, thanks. I was totally focused on the "cmd" on tutorial. The cases are very detailed and I'll work on unit tests with them. Thank you.

@feldrim
Copy link
Author

feldrim commented Dec 28, 2018

I actually couldn't get the format except garbled in "garbled "warden dot dll.exe" garbled". We want commands in quotations? Then what's the issue with .exe. Can you please explain the format and expected result?

@yretenai
Copy link
Contributor

Out of those examples, the expected outcome would be

"cmd"                                         = cmd
"garbled cmd garbled"                         = garbled 
"cmd.exe"                                     = cmd.exe
"garbled cmd.exe garbled"                     = cmd.exe 
"C:\cmd.exe"                                  = C:\cmd.exe
"garbled C:\cmd.exe garbled"                  = C:\cmd.exe 
"C:\Windows\system32\cmd.exe"                 = C:\Windows\system32\cmd.exe
"garbled C:\Windows\system32\cmd.exe garbled" = C:\Windows\system32\cmd.exe 
"\\server\cmd.exe"                            = \\server\cmd.exe
"garbled \\server\cmd.exe garbled"            = \\server\cmd.exe 
""warden dot dll.exe""                        = warden dot dll.exe
"garbled "warden dot dll.exe" garbled"        = warden dot dll.exe

@feldrim
Copy link
Author

feldrim commented Dec 28, 2018

As of now, the tests pass but the part in quotation marks are accepted with any text inside. I believe it should be something like "command command filename.exe" or if it's specific to warden, "warden command filename.exe". Yet, it's incomplete although all tests pass.

@zbalkan
Copy link

zbalkan commented Jan 15, 2019

@healingbrew Hi. I have a question. Let's say I typed something like cd "c:\Users\Some User\Desktop\Some Path Containing Spaces". Then the implementation would only care the quoted string. It might be incorrect by design.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants