[sonic-config-engine] Replace os.system, replace yaml.load, remove subprocess with shell=True by maipbui · Pull Request #12533 · sonic-net/sonic-buildimage

maipbui · 2022-10-27T18:52:11Z

Signed-off-by: maipbui [email protected]

Why I did it

subprocess is used with shell=True, which is very dangerous for shell injection.
os - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content
yaml.load can create arbitrary Python objects

How I did it

Replace os by subprocess, remove shell=True
Use yaml.safe_load()

How to verify it

Pass UT

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: maipbui <[email protected]>

qiluo-msft · 2022-10-28T18:49:09Z

src/sonic-config-engine/tests/common_utils.py

            self.yang_parser.loadYangModel()
            self.test_dir = os.path.dirname(os.path.realpath(__file__))
-            self.script_file = PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+            self.script_file = [PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


Suggested change

self.script_file = [PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]

self.script_file = [PYTHON_INTERPRETTER, os.path.join(self.test_dir, '..', 'sonic-cfggen')]

``` #Closed

All are fixed.

qiluo-msft · 2022-10-28T18:49:45Z

src/sonic-config-engine/tests/common_utils.py

-            args, unknown = parser.parse_known_args(shlex.split(argument))
+            parser.add_argument("-o", "--output-file", help="Output file", nargs='?', const=None)
+            args, unknown = parser.parse_known_args(argument)
+            print(args)


print

Remove debug code. #Closed

qiluo-msft · 2022-10-28T19:08:26Z

src/sonic-config-engine/tests/common_utils.py

            parser.add_argument("-S", "--hwsku-config", help="hwsku config file, used with -p and -m or -k", nargs='?', const=None)
            parser.add_argument("-j", "--json", help="additional json file input, used with -p, -S and -m or -k", nargs='?', const=None)
-            args, unknown = parser.parse_known_args(shlex.split(argument))
+            parser.add_argument("-o", "--output-file", help="Output file", nargs='?', const=None)


add_argument

Why change behavior? #Closed

Because there're some commands that have output redirection.
Original command:

sonic-buildimage/src/sonic-config-engine/tests/test_j2files.py

Line 98 in f34ca2b

argument = '-m ' + self.t0_minigraph_nomgmt + ' -p ' + self.t0_port_config_tiny + ' -j ' + self.ztp + ' -j ' + self.port_data + ' -a \'{\"hwaddr\":\"e4:1d:2d:a5:f3:ad\"}\' -t ' + interfaces_template + '> ' + self.output_file

Changed command

sonic-buildimage/src/sonic-config-engine/tests/test_j2files.py

Line 109 in ce3e075

argument = ['-m', self.t0_minigraph_nomgmt, '-p', self.t0_port_config_tiny, '-j', self.ztp, '-j', self.port_data, '-a', '{\"hwaddr\":\"e4:1d:2d:a5:f3:ad\"}', '-t', interfaces_template, '-o', self.output_file]

Implementation of new behavior:
https://github.com/sonic-net/sonic-buildimage/blob/ce3e0759543cd00a5a515b31ffb3e87de48c2e76/src/sonic-config-engine/tests/test_j2files.py#L46-#L59

I prefer following subprocess functions, adding stdout=None, stderr=None to this function parameter list.

qiluo-msft · 2022-10-28T19:09:10Z

src/sonic-config-engine/tests/test_cfggen.py

        self.yang = utils.YangWrapper()
        self.test_dir = os.path.dirname(os.path.realpath(__file__))
-        self.script_file = utils.PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+        self.script_file = [utils.PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


] + [

The same. #Closed

qiluo-msft · 2022-10-28T19:09:48Z

src/sonic-config-engine/tests/test_cfggen.py


    def run_script(self, argument, check_stderr=False, verbose=False):
-        print('\n    Running sonic-cfggen ' + argument)
+        print('\n    Running sonic-cfggen ', argument)


,

Does semgrep alert on this line? #WontFix

No, but argument variable is a list, cannot be concatenated with string in the print statement.

Updated to shlex.join

I notice it is not available in python 3.7. Then + is good enough in test code.

qiluo-msft · 2022-10-28T19:13:10Z

src/sonic-config-engine/tests/test_cfggen_platformJson.py

    def setUp(self):
        self.test_dir = os.path.dirname(os.path.realpath(__file__))
-        self.script_file = utils.PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+        self.script_file = [utils.PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


] + [

The same. #Closed

qiluo-msft · 2022-10-28T19:13:28Z

src/sonic-config-engine/tests/test_cfggen_t2_chassis_fe.py

    def setUp(self):
        self.test_dir = os.path.dirname(os.path.realpath(__file__))
-        self.script_file = utils.PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+        self.script_file = [utils.PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


] + [

The same #Closed

qiluo-msft · 2022-10-28T19:13:40Z

src/sonic-config-engine/tests/test_cfggen_t2_chassis_fe.py

        self.t2_chassis_fe_port_config = os.path.join(self.test_dir, 't2-chassis-fe-port-config.ini')

    def run_script(self, argument, check_stderr=False):
-        print('\n    Running sonic-cfggen ' + argument)


semgrep alert on this line? #Closed

qiluo-msft · 2022-10-28T19:15:54Z

src/sonic-config-engine/tests/test_j2files_t2_chassis_fe.py

    def setUp(self):
        self.test_dir = os.path.dirname(os.path.realpath(__file__))
-        self.script_file = utils.PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+        self.script_file = [utils.PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


] + [

The same. #Closed

qiluo-msft · 2022-10-28T19:16:06Z

src/sonic-config-engine/tests/test_j2files_t2_chassis_fe.py

    def run_script(self, argument):
-        print('CMD: sonic-cfggen ' + argument)
-        output = subprocess.check_output(self.script_file + ' ' + argument, shell=True)
+        print('CMD: sonic-cfggen ', argument)


,

Semgrep alert on this line? #Closed

qiluo-msft · 2022-10-28T19:16:22Z

src/sonic-config-engine/tests/test_minigraph_case.py

        self.yang = utils.YangWrapper()
        self.test_dir = os.path.dirname(os.path.realpath(__file__))
-        self.script_file = utils.PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+        self.script_file = [utils.PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


] + [

The same #Closed

qiluo-msft · 2022-10-28T19:16:28Z

src/sonic-config-engine/tests/test_minigraph_case.py

        self.port_config = os.path.join(self.test_dir, 't0-sample-port-config.ini')

    def run_script(self, argument, check_stderr=False):
-        print('\n    Running sonic-cfggen ' + argument)


The same. #Closed

qiluo-msft · 2022-10-28T19:17:23Z

src/sonic-config-engine/tests/test_multinpu_cfggen.py

        self.test_dir = os.path.dirname(os.path.realpath(__file__))
        self.test_data_dir = os.path.join(self.test_dir,  'multi_npu_data')
-        self.script_file = utils.PYTHON_INTERPRETTER + ' ' + os.path.join(self.test_dir, '..', 'sonic-cfggen')
+        self.script_file = [utils.PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]


] + [

The same #Closed

qiluo-msft · 2022-10-28T19:17:28Z

src/sonic-config-engine/tests/test_multinpu_cfggen.py

        os.environ["CFGGEN_UNIT_TESTING"] = "2"

    def run_script(self, argument, check_stderr=False):
-        print('\n    Running sonic-cfggen ' + argument)


The same #Closed

Signed-off-by: maipbui <[email protected]>

ganglyu · 2022-10-29T01:28:59Z

src/sonic-config-engine/tests/test_frr.py

+        write_output = False
+        if '-o' in argument:
+            write_output = True
+            output_file = argument[-1]


How to guarantee '-o' is the last argument?

I changed to different solution.

ganglyu · 2022-10-29T01:32:40Z

src/sonic-config-engine/tests/test_j2files.py

-            os.system(echo_cmd)
+                subprocess.call(['sudo', 'mkdir', '/host'])
+            subprocess.call(['sudo', 'touch', '/host/machine.conf'])
+            getstatusoutput_noshell_pipe(echo_cmd1, echo_cmd2)


Can you explain these commands?

Please check this PR #12065

ganglyu · 2022-10-29T01:35:22Z

src/sonic-config-engine/tests/test_j2files.py

    def remove_machine_conf(self, file_exist, dir_exist):
        if not file_exist:
-            os.system('sudo rm -f /host/machine.conf')
+            subprocess.call(['sudo', 'rm', '-f', '/host/machine.conf'])


there's no user input in 'sudo rm -f /host/machine.conf', why is it a risk?

It's better for extra security when using subprocess and with array of strings. https://semgrep.dev/docs/cheat-sheets/python-command-injection/#1c-using-os-module-to-execute-commands

Signed-off-by: maipbui <[email protected]>

…emove subprocess with shell=True (#12533)" This reverts commit 934871c.

…emove subprocess with shell=True (#12533)" (#12616) This reverts commit 934871c. Unblocking sync from github to internal

….load, remove subprocess with shell=True (sonic-net#12533)" (sonic-net#12616)" This reverts commit 661c467.

…ubprocess with shell=True (#12607) Signed-off-by: maipbui <[email protected]> #### Why I did it Missing import statement in PR #12533 #### How I did it Revert [PR 12646](#12616) Add import statement 1. https://github.com/sonic-net/sonic-buildimage/blob/31f7afa92e8d1e353ef2f3b004711dd18c0d94f6/src/sonic-config-engine/tests/test_j2files_t2_chassis_fe.py#L8 2. https://github.com/sonic-net/sonic-buildimage/blob/31f7afa92e8d1e353ef2f3b004711dd18c0d94f6/src/sonic-config-engine/tests/test_j2files.py#L8 3. https://github.com/sonic-net/sonic-buildimage/blob/31f7afa92e8d1e353ef2f3b004711dd18c0d94f6/src/sonic-config-engine/tests/test_multinpu_cfggen.py#L11 #### How to verify it Pass UT

Replace unsafe functions

a5b2779

Signed-off-by: maipbui <[email protected]>

maipbui requested a review from qiluo-msft October 27, 2022 20:09

qiluo-msft reviewed Oct 28, 2022

View reviewed changes

qiluo-msft requested review from ganglyu and qiluo-msft October 28, 2022 19:19

maipbui added 2 commits October 28, 2022 20:23

Resolve PR comments

ce3e075

Signed-off-by: maipbui <[email protected]>

Resolve PR comments

4944fda

Signed-off-by: maipbui <[email protected]>

ganglyu reviewed Oct 29, 2022

View reviewed changes

maipbui added 2 commits October 29, 2022 03:42

Resolve PR comments

8198a29

Signed-off-by: maipbui <[email protected]>

Fix build

36f1870

Signed-off-by: maipbui <[email protected]>

ganglyu approved these changes Oct 29, 2022

View reviewed changes

qiluo-msft approved these changes Oct 31, 2022

View reviewed changes

maipbui marked this pull request as ready for review October 31, 2022 14:41

maipbui changed the title ~~[sonic-config-engine] Replace os.system and remove subprocess with shell=True~~ [sonic-config-engine] Replace os.system, replace yaml.load, remove subprocess with shell=True Oct 31, 2022

maipbui merged commit 934871c into sonic-net:master Oct 31, 2022

maipbui deleted the bui/config-engine-pysec branch October 31, 2022 14:43

maipbui mentioned this pull request Nov 4, 2022

[sonic-config-engine] Replace os.system, replace yaml.load, remove subprocess with shell=True #12607

Merged

7 tasks

StormLiangMS added a commit that referenced this pull request Nov 7, 2022

Revert "[sonic-config-engine] Replace os.system, replace yaml.load, r…

e9f446e

…emove subprocess with shell=True (#12533)" This reverts commit 934871c.

StormLiangMS mentioned this pull request Nov 7, 2022

Revert "[sonic-config-engine] Replace os.system, replace yaml.load, remove subprocess with shell=True" #12616

Merged

StormLiangMS added a commit that referenced this pull request Nov 7, 2022

Revert "[sonic-config-engine] Replace os.system, replace yaml.load, r…

661c467

…emove subprocess with shell=True (#12533)" (#12616) This reverts commit 934871c. Unblocking sync from github to internal

maipbui added a commit to maipbui/sonic-buildimage that referenced this pull request Nov 7, 2022

Revert "Revert "[sonic-config-engine] Replace os.system, replace yaml…

e449603

….load, remove subprocess with shell=True (sonic-net#12533)" (sonic-net#12616)" This reverts commit 661c467.

maipbui added Static Code Analysis and removed Static Code Analysis labels Nov 23, 2022

	self.script_file = [PYTHON_INTERPRETTER] + [os.path.join(self.test_dir, '..', 'sonic-cfggen')]
	self.script_file = [PYTHON_INTERPRETTER, os.path.join(self.test_dir, '..', 'sonic-cfggen')]
	``` #Closed

Conversation

maipbui commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maipbui commented Oct 27, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading

qiluo-msft Oct 28, 2022 •

edited

Loading