Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[security] parse quoted strings for possible commands #147, #148, #149 #153

Merged
merged 1 commit into from Aug 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
[security] parse quoted strings for possible commands (Closes #148, C…
…loses #147, Closes #149)

Both issues #148 and #147 use the same vulnerability in the parser,
that ignored the quoted strings. Parsing only the rest of the line
for security issues. This is a major security bug.

This commits also corrects a previous ommited correction regarding the
control charaters, that permitted to escape from lshell.

Thank you Proskurin Kirill (@Oloremo) and Vladislav Yarmak (@Snawoot)
for reporting this!!
  • Loading branch information
Ignace Mouzannar committed Aug 25, 2016
commit a686f71732a3d0f16df52ef46ab8a49ee0083c68
9 changes: 3 additions & 6 deletions lshell/sec.py
Expand Up @@ -174,12 +174,9 @@ def check_secure(line, conf, strict=None, ssh=None):
ret_check_path, conf = check_path(item, conf, strict=strict)
returncode += ret_check_path

# ignore quoted text
line = re.sub(r'\"(.+?)\"', '', line)
line = re.sub(r'\'(.+?)\'', '', line)

if re.findall('[:cntrl:].*\n', line):
ret, conf = warn_count('syntax',
# parse command line for control characters, and warn user
if re.findall(r'[\x01-\x1F\x7F]', oline):
ret, conf = warn_count('control char',
oline,
conf,
strict=strict,
Expand Down
24 changes: 21 additions & 3 deletions test/test_functional.py
Expand Up @@ -384,14 +384,14 @@ def test_27_checksecure_awk(self):

self.assertEqual(expected, result)

def test_28_catch_lnext_terminal_ctrl(self):
""" F25 | test ctrl-v ctrl-j then command, forbidden/security """
def test_28_catch_terminal_ctrl_j(self):
""" F28 | test ctrl-v ctrl-j then command, forbidden/security """
self.child = pexpect.spawn('%s/bin/lshell '
'--config %s/etc/lshell.conf '
% (TOPDIR, TOPDIR))
self.child.expect('%s:~\$' % self.user)

expected = u'*** forbidden syntax: echo\r'
expected = u'*** forbidden control char: echo\r'
self.child.send('echo')
self.child.sendcontrol('v')
self.child.sendcontrol('j')
Expand All @@ -402,5 +402,23 @@ def test_28_catch_lnext_terminal_ctrl(self):

self.assertIn(expected, result)

def test_29_catch_terminal_ctrl_k(self):
""" F29 | test ctrl-v ctrl-k then command, forbidden/security """
self.child = pexpect.spawn('%s/bin/lshell '
'--config %s/etc/lshell.conf '
% (TOPDIR, TOPDIR))
self.child.expect('%s:~\$' % self.user)

expected = u'*** forbidden control char: echo\x0b() bash && echo\r'
self.child.send('echo')
self.child.sendcontrol('v')
self.child.sendcontrol('k')
self.child.sendline('() bash && echo')
self.child.expect('%s:~\$' % self.user)

result = self.child.before.decode('utf8').split('\n')[1]

self.assertIn(expected, result)

if __name__ == '__main__':
unittest.main()
25 changes: 15 additions & 10 deletions test/test_unit.py
Expand Up @@ -18,16 +18,6 @@ class TestFunctions(unittest.TestCase):
userconf = CheckConfig(args).returnconf()
shell = ShellCmd(userconf, args)

def test_01_checksecure_doublequote(self):
""" U01 | quoted text should not be forbidden """
INPUT = 'ls -E "1|2" tmp/test'
return self.assertEqual(sec.check_secure(INPUT, self.userconf)[0], 0)

def test_02_checksecure_simplequote(self):
""" U02 | quoted text should not be forbidden """
INPUT = "ls -E '1|2' tmp/test"
return self.assertEqual(sec.check_secure(INPUT, self.userconf)[0], 0)

def test_03_checksecure_doublepipe(self):
""" U03 | double pipes should be allowed, even if pipe is forbidden """
args = self.args + ["--forbidden=['|']"]
Expand Down Expand Up @@ -221,5 +211,20 @@ def test_25_disable_ld_preload(self):
# verify that no alias was created containing LD_PRELOAD
return self.assertNotIn('echo', userconf['aliases'])

def test_26_checksecure_quoted_command(self):
""" U26 | quoted command should be parsed """
INPUT = 'echo 1 && "bash"'
return self.assertEqual(sec.check_secure(INPUT, self.userconf)[0], 1)

def test_27_checksecure_quoted_command(self):
""" U27 | quoted command should be parsed """
INPUT = '"bash" && echo 1'
return self.assertEqual(sec.check_secure(INPUT, self.userconf)[0], 1)

def test_28_checksecure_quoted_command(self):
""" U28 | quoted command should be parsed """
INPUT = "echo'/1.sh'"
return self.assertEqual(sec.check_secure(INPUT, self.userconf)[0], 1)

if __name__ == "__main__":
unittest.main()