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

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

@@ -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,

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()

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()