Remove subprocess with shell=True #24

maipbui · 2022-11-02T20:50:50Z

Signed-off-by: maipbui maibui@microsoft.com

Why I did it

subprocess is used with shell=True, which is very dangerous for shell injection.

How I did it

remove shell=True, use shell=False

How to verify it

Pass UT
Manual test

Signed-off-by: maipbui <maibui@microsoft.com>

lgtm-com · 2022-11-02T21:06:32Z

This pull request introduces 1 alert when merging c1251e9 into bc8698d - view on LGTM.com

new alerts:

1 for Unused import

Signed-off-by: maipbui <maibui@microsoft.com>

qiluo-msft · 2022-11-02T23:19:15Z

scripts/hostcfgd

+        if raise_exception:
+            raise
+
+def run_cmd_pipe(cmd0, cmd1, cmd2, log_err=True, raise_exception=False):


run_cmd_pipe

Similar function already implemented. Could you reuse in the scope of this repo? #Closed

which function are you referring to?

You implemented twice run_commands_pipe in this PR. I understand they are different a little bit. Could you try unify and reuse?

Unified duplicate code to one function.

qiluo-msft · 2022-11-02T23:22:46Z

scripts/hostcfgd

-            cmd = "sed -e {0} {1} > {1}.new; mv -f {1} {1}.old; mv -f {1}.new {1}".format(' -e '.join(operations), filename)
-            os.system(cmd)
+            e_operations_str = ' -e '.join(operations)
+            e_operations_list = split(e_operations_str)


e_operations_list

operations is already a list, no need to join and then split. #Closed

I think the list is joined by ' -e ', not whitespace?

I got it. Better to implement the transformation on a list directly.

qiluo-msft · 2022-11-02T23:23:17Z

scripts/hostcfgd

-            os.system(cmd)
+            e_operations_str = ' -e '.join(operations)
+            e_operations_list = split(e_operations_str)
+            with open(filename+'new', 'w') as f:


new

you miss . ? #Closed

qiluo-msft · 2022-11-02T23:24:32Z

scripts/hostcfgd

@@ -971,9 +987,11 @@ class PasswHardening(object):

    def modify_single_file_inplace(self, filename, operations=None):
        if operations:
-            cmd = "sed -i {0} {1}".format(' -i '.join(operations), filename)
+            i_operations_str = ' -i '.join(operations)
+            i_operations_list = split(i_operations_str)


split

The same. #Closed

qiluo-msft · 2022-11-02T23:56:09Z

scripts/procdockerstatsd

+        cmd0 = ["ps", "-eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd", "--sort", "-%cpu"]
+        cmd1 = ["head", "-1024"]
+        exitcode, data = getstatusoutput_noshell_pipe(cmd0, cmd1)
+        if exitcode != [0, 0]:


exitcode

If you want to check all array elements are zero, you can use not exitcode.any() #Closed

Signed-off-by: maipbui <maibui@microsoft.com>

ganglyu · 2022-11-03T02:42:18Z

tests/caclmgrd/caclmgrd_bfd_test.py

@@ -44,6 +44,8 @@ def test_caclmgrd_bfd(self, test_name, test_data, fs):
            call_rc = test_data["call_rc"]
            mocked_subprocess.call.return_value = call_rc

+            self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
+            self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()


Why do you add mock? And why can original unit test work without mock?

Without mock, it throws this error:

scripts/caclmgrd:202: in run_commands_pipe exitcodes, stdout = getstatusoutput_noshell_pipe(cmd0, cmd1, cmd2, cmd3) /usr/local/lib/python3.9/dist-packages/sonic_py_common/general.py:52: in getstatusoutput_noshell_pipe popens = [Popen(cmd0, stdout=PIPE, universal_newlines=True)] [OSError]: [Error 9] Bad file descriptor in the fake filesystem: '11'

I think it is due to

If close_fds is true, all file descriptors except 0, 1 and 2 will be closed before the child process is executed. Otherwise when close_fds is false, file descriptors obey their inheritable flag as described in Inheritance of File Descriptors.

in https://docs.python.org/3/library/subprocess.html#popen-constructor
Not sure how original UT works w/o mock

You have changed the behavior of this unit test, and we might have better solution.
https://github.com/sonic-net/sonic-host-services/blob/master/tests/caclmgrd/caclmgrd_bfd_test.py#L37
Above line doesn't work with your commit, and I think you should fix this mock and keep the original behavior.

I fixed the mock and revert to original behavior. Could you help review again this UT?

Other file is still using mock for get_namespace_mgmt_ip and get_namespace_mgmt_ipv6

Fixed. And thanks so much for your suggestion!

ganglyu · 2022-11-03T02:47:03Z

scripts/caclmgrd

@@ -178,7 +182,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase):
            commands: List of strings, each string is a shell command


It should be List of List?

ganglyu · 2022-11-03T02:48:18Z

scripts/caclmgrd

+        """
+        Given a list of shell commands, run them in order
+        Args:
+            commands: List of strings, each string is a shell command


Still List of strings?

Signed-off-by: maipbui <maibui@microsoft.com>

fastiuk · 2022-11-03T21:35:44Z

scripts/hostcfgd

+        subprocess.check_call(cmd)
+    except Exception as err:
+        if log_err:
+            syslog.syslog(syslog.LOG_ERR, "{} - failed: return code - {}, output:\n{}"
+                  .format(err.cmd, err.returncode, err.output))
+        if raise_exception:
+            raise
+
+def run_cmd_pipe(cmd0, cmd1, cmd2, log_err=True, raise_exception=False):


Why use that code and not use the python 'sh' library?
It supports everything you need here: shell lexicography and pipe.
It is just much easier to use it.
Link to docs

any updates?

We want to have a more secure codebase and aim to fix potentially dangerous function calls that could introduce security vulnerabilities if used incorrectly. I fixed the functions based on Semgrep recommendations (a static analysis tool to find security issues, some refs: https://semgrep.dev/docs/cheat-sheets/python-command-injection/, https://github.com/sonic-net/sonic-buildimage/actions/runs/3412786426/jobs/5678731076). Not sure if we want to use third-party library and the 'sh' library docs does not mention anything about security.

Signed-off-by: maipbui <maibui@microsoft.com>

ganglyu · 2022-11-07T03:38:08Z

tests/caclmgrd/caclmgrd_dhcp_test.py

@@ -44,6 +44,8 @@ def test_caclmgrd_dhcp(self, test_name, test_data, fs):

            mark = test_data["mark"]

+            self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
+            self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()


ganglyu · 2022-11-07T03:38:17Z

tests/caclmgrd/caclmgrd_feature_test.py

@@ -44,6 +44,8 @@ def test_feature_present(self, test_name, test_data, fs):
            call_rc = test_data["call_rc"]
            mocked_subprocess.call.return_value = call_rc

+            self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ip = mock.MagicMock()
+            self.caclmgrd.ControlPlaneAclManager.get_namespace_mgmt_ipv6 = mock.MagicMock()


Signed-off-by: maipbui <maibui@microsoft.com>

qiluo-msft · 2023-01-04T00:54:57Z

scripts/caclmgrd

+        ipv6_address_cmd3 = ['head', '-1']
+        return self.run_commands_pipe(ipv6_address_cmd0, ipv6_address_cmd1, ipv6_address_cmd2, ipv6_address_cmd3)
+
+    def log_output(self, cmd, exitcodes, stdout):


log_output

Add an explicit return None at the function end, since you are expecting the behavior. #Closed

Signed-off-by: maipbui <maibui@microsoft.com>

linux-foundation-easycla · 2023-01-04T04:15:31Z

The committers listed above are authorized under a signed CLA.

✅ login: maipbui / name: Mai Bui (865fd1d, c1251e9, ae4acf8, 6fe4056, 4285463, ff626f7, 4efddb6, 16c12cb, 3ba0aae, e1bb857, bbf121b, d02382e, 592079c, a9d0945, 7401e30, a42cf3a, 8e76910, e9269b4, e9fe5fa, 61c0e1e, 709a418, 215745e, 5f9c9c5)

Signed-off-by: maipbui <maibui@microsoft.com>

qiluo-msft · 2023-01-04T18:39:22Z

scripts/procdockerstatsd

@@ -135,7 +136,13 @@ class ProcDockerStats(daemon_base.DaemonBase):
        return True

    def update_processstats_command(self):
-        data = self.run_command("ps -eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd --sort -%cpu | head -1024")
+        cmd = "ps -eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd --sort -%cpu | head -1024"


cmd = "ps -eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd --sort -%cpu | head -1024"

Assume someone fix cmd0, but forget to change cmd, the log_error message will be wrong. #Closed

qiluo-msft · 2023-01-04T18:50:58Z

scripts/hostcfgd

                if ret == 0:
                    syslog.syslog(syslog.LOG_INFO, "{} rule exists in {}".format(ip, chain))
                else:
                    # Modify command from Check to Append
-                    iptables_cmds.append(cmd.replace("check", "append"))
+                    iptables_cmds.append([word if word != "--check" else "--append" for word in cmd])


cmd

Strictly speaking, you changed behavior.
Suggest loop through cmd, replace in each element. #Closed

Signed-off-by: maipbui <maibui@microsoft.com>

This reverts commit 77b9c77.

This reverts commit 1b0d54c.

qiluo-msft · 2023-01-12T22:18:53Z

tests/mock_connector.py

@@ -22,3 +22,10 @@ def keys(self, db_id, pattern):

    def get_all(self, db_id, key):
        return MockConnector.data[key]
+
+    def delete_all_by_pattern(self, db_id, pattern):


delete_all_by_pattern

This mock function only work on special use case, not a general implementation. Please consider copy the logic from codebase, and maybe implement a mock delete() function.

This reverts commit 409ecc3.

…nic-net#33)" This reverts commit 28afb55.

StormLiangMS · 2023-04-20T13:47:27Z

@dgsudharsan included the #34, which is newer version, I think we don't need this PR.

maipbui added 2 commits November 2, 2022 20:46

Replace unsafe functions

865fd1d

Signed-off-by: maipbui <maibui@microsoft.com>

Merge branch 'master' into pysec

c1251e9

Fix lgtm and UT

ae4acf8

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui requested a review from qiluo-msft November 2, 2022 22:42

qiluo-msft requested review from abdosi, fastiuk, ZhaohuiS and ganglyu November 2, 2022 22:51

qiluo-msft reviewed Nov 2, 2022

View reviewed changes

Resolve PR comments

6fe4056

Signed-off-by: maipbui <maibui@microsoft.com>

ganglyu reviewed Nov 3, 2022

View reviewed changes

Resolve PR comments

4285463

Signed-off-by: maipbui <maibui@microsoft.com>

fastiuk reviewed Nov 3, 2022

View reviewed changes

maipbui added 2 commits November 4, 2022 13:46

Merge branch 'master' into pysec

ff626f7

Fix UT

4efddb6

Signed-off-by: maipbui <maibui@microsoft.com>

ganglyu approved these changes Nov 7, 2022

View reviewed changes

ganglyu self-requested a review November 7, 2022 03:37

ganglyu reviewed Nov 7, 2022

View reviewed changes

maipbui added 2 commits November 7, 2022 03:40

Fix UT

16c12cb

Signed-off-by: maipbui <maibui@microsoft.com>

Fix UT

3ba0aae

Signed-off-by: maipbui <maibui@microsoft.com>

ganglyu approved these changes Nov 7, 2022

View reviewed changes

Resolve PR comments

e1bb857

Signed-off-by: maipbui <maibui@microsoft.com>

qiluo-msft reviewed Jan 4, 2023

View reviewed changes

Address PR comment

709a418

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui added 2 commits January 4, 2023 15:16

Merge remote-tracking branch 'upstream/master' into pysec

215745e

Signed-off-by: maipbui <maibui@microsoft.com>

Fix build

5f9c9c5

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui force-pushed the pysec branch from 23cd674 to 5f9c9c5 Compare January 4, 2023 15:22

qiluo-msft reviewed Jan 4, 2023

View reviewed changes

Address PR comments

e39e72e

Signed-off-by: maipbui <maibui@microsoft.com>

qiluo-msft approved these changes Jan 7, 2023

View reviewed changes

Improve coverage

4f220ae

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui force-pushed the pysec branch from 6a9162f to 4f220ae Compare January 12, 2023 18:12

maipbui added 4 commits January 12, 2023 18:30

Ignore findings

1b0d54c

Signed-off-by: maipbui <maibui@microsoft.com>

Ignore findings

77b9c77

Signed-off-by: maipbui <maibui@microsoft.com>

Revert "Ignore findings"

f1f9011

This reverts commit 77b9c77.

Revert "Ignore findings"

9ea3367

This reverts commit 1b0d54c.

maipbui marked this pull request as ready for review January 12, 2023 20:23

maipbui merged commit 409ecc3 into sonic-net:master Jan 12, 2023

maipbui deleted the pysec branch January 12, 2023 20:46

qiluo-msft reviewed Jan 12, 2023

View reviewed changes

maipbui added a commit to maipbui/sonic-host-services that referenced this pull request Jan 12, 2023

Revert "Remove subprocess with shell=True (sonic-net#24)"

4ee8033

This reverts commit 409ecc3.

maipbui mentioned this pull request Jan 12, 2023

Revert "Remove subprocess with shell=True (#24)" #33

Merged

qiluo-msft pushed a commit that referenced this pull request Jan 12, 2023

Revert "Remove subprocess with shell=True (#24)" (#33)

28afb55

This reverts commit 409ecc3.

maipbui added a commit to maipbui/sonic-host-services that referenced this pull request Jan 13, 2023

Revert "Revert "Remove subprocess with shell=True (sonic-net#24)" (so…

6b8e7ad

…nic-net#33)" This reverts commit 28afb55.

dgsudharsan added the Request for 202211 Branch label Apr 14, 2023

dgsudharsan mentioned this pull request Apr 17, 2023

[202211][caclmgrd]Allow Vxlan udp port on receiving vxlan tunnel configuration #54

Closed

StormLiangMS added Included in 202211 and removed Included in 202211 labels Apr 20, 2023

StormLiangMS removed the Request for 202211 Branch label Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove subprocess with shell=True #24

Remove subprocess with shell=True #24

maipbui commented Nov 2, 2022 •

edited

Loading

lgtm-com bot commented Nov 2, 2022

qiluo-msft Nov 2, 2022 •

edited

Loading

maipbui Nov 3, 2022

qiluo-msft Nov 3, 2022

maipbui Nov 7, 2022

qiluo-msft Nov 2, 2022 •

edited

Loading

maipbui Nov 3, 2022

qiluo-msft Nov 3, 2022

qiluo-msft Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

ganglyu Nov 3, 2022

maipbui Nov 3, 2022 •

edited

Loading

ganglyu Nov 3, 2022

maipbui Nov 7, 2022

ganglyu Nov 7, 2022

maipbui Nov 7, 2022

ganglyu Nov 3, 2022

ganglyu Nov 3, 2022

fastiuk Nov 3, 2022

fastiuk Nov 7, 2022

maipbui Nov 7, 2022

ganglyu Nov 7, 2022

ganglyu Nov 7, 2022

qiluo-msft Jan 4, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jan 4, 2023 •

edited

Loading

qiluo-msft Jan 4, 2023 •

edited

Loading

qiluo-msft Jan 4, 2023 •

edited

Loading

qiluo-msft Jan 12, 2023

StormLiangMS commented Apr 20, 2023

		@@ -178,7 +182,7 @@ class ControlPlaneAclManager(daemon_base.DaemonBase):
		commands: List of strings, each string is a shell command

Remove subprocess with shell=True #24

Remove subprocess with shell=True #24

Conversation

maipbui commented Nov 2, 2022 • edited Loading

Why I did it

How I did it

How to verify it

lgtm-com bot commented Nov 2, 2022

qiluo-msft Nov 2, 2022 • 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

qiluo-msft Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maipbui Nov 3, 2022 • 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

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

qiluo-msft Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jan 4, 2023 • edited Loading

qiluo-msft Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StormLiangMS commented Apr 20, 2023

maipbui commented Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

qiluo-msft Nov 2, 2022 •

edited

Loading

maipbui Nov 3, 2022 •

edited

Loading

qiluo-msft Jan 4, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jan 4, 2023 •

edited

Loading

qiluo-msft Jan 4, 2023 •

edited

Loading

qiluo-msft Jan 4, 2023 •

edited

Loading