From a686f71732a3d0f16df52ef46ab8a49ee0083c68 Mon Sep 17 00:00:00 2001
From: Ignace Mouzannar <ignace@redhat.com>
Date: Tue, 23 Aug 2016 17:08:22 -0400
Subject: [PATCH] [security] parse quoted strings for possible commands (Closes
 #148, Closes #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!!
---
 lshell/sec.py           |  9 +++------
 test/test_functional.py | 24 +++++++++++++++++++++---
 test/test_unit.py       | 25 +++++++++++++++----------
 3 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/lshell/sec.py b/lshell/sec.py
index ea3561f..ff9557c 100644
--- a/lshell/sec.py
+++ b/lshell/sec.py
@@ -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,
diff --git a/test/test_functional.py b/test/test_functional.py
index d139e7e..5d3b1d5 100644
--- a/test/test_functional.py
+++ b/test/test_functional.py
@@ -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')
@@ -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()
diff --git a/test/test_unit.py b/test/test_unit.py
index 137f551..d5ed417 100644
--- a/test/test_unit.py
+++ b/test/test_unit.py
@@ -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=['|']"]
@@ -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()