[sonic-py-common] Add getstatusoutput_noshell() functions to general module #12065

maipbui · 2022-09-13T22:57:18Z

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

Why I did it

getstatusoutput() function from subprocess module has shell injection issue because it includes shell=True in the implementation
Eliminate duplicate code

How I did it

Reimplement getstatusoutput_noshell() and getstatusoutput_noshell_pipe() functions with shell=False
Add check_output_pipe() function

How to verify it

Pass UT

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

Description for the changelog

Link to config_db schema for YANG module changes

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

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

qiluo-msft · 2022-09-14T18:31:17Z

src/sonic-py-common/sonic_py_common/general.py

+            output = p2.communicate()[0]
+            p1.wait()
+            if p1.returncode != 0 and p2.returncode != 0:
+                status = 1


status = 1

This is a general function, so hide information into status 0/1 is not recommended. Suggest return an array of status as the first element in the returned pair. #Closed

qiluo-msft · 2022-09-14T18:31:52Z

src/sonic-py-common/sonic_py_common/general.py

@@ -23,3 +24,33 @@ def load_module_from_source(module_name, file_path):
    sys.modules[module_name] = module

    return module
+
+
+def getstatusoutput_noshell(cmd):


getstatusoutput_noshell

Please add unit tests to cover new functions. #Closed

Sure, working on UT

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

qiluo-msft · 2022-09-15T20:45:26Z

src/sonic-py-common/sonic_py_common/general.py

+            with Popen(cmd2, universal_newlines=True, stdin=p1.stdout, stdout=PIPE) as p2:
+                output = p2.communicate()[0]
+                p1.wait()
+                if p1.returncode != 0 and p2.returncode != 0:


and

or? #Closed

qiluo-msft · 2022-09-15T21:00:40Z

src/sonic-py-common/sonic_py_common/general.py

+        with Popen(cmd2, universal_newlines=True, stdin=p1.stdout, stdout=PIPE) as p2:
+            output = p2.communicate()[0]
+            p1.wait()
+            if p1.returncode != 0 and p2.returncode != 0:


and

and -> or #Closed

qiluo-msft · 2022-09-15T21:00:54Z

src/sonic-py-common/sonic_py_common/general.py

+            output = p2.communicate()[0]
+            p1.wait()
+            if p1.returncode != 0 and p2.returncode != 0:
+                returncode = p1.returncode if p2.returncode == 0 else p2.returncode


p1

check p1 return code first, if not zero, throw based on p1 info. otherwise, check p2 return code, and throw based on p2 info. #Closed

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

qiluo-msft · 2022-09-16T00:27:54Z

src/sonic-py-common/sonic_py_common/general.py

+            output = p2.communicate()[0]
+            p1.wait()
+            if p1.returncode != 0 or p2.returncode != 0:
+                return ([p1.returncode, p2.returncode], output)


You do not need to specially handle this case, and fall through below code, and return [p1.returncode, p2.returncode], output #Closed

qiluo-msft · 2022-09-16T00:37:21Z

src/sonic-py-common/sonic_py_common/general.py

+            p1.wait()
+            if p1.returncode != 0 or p2.returncode != 0:
+                if p1.returncode != 0:
+                    raise CalledProcessError(returncode=p1.returncode, cmd=cmd, output=output)


cmd

cmd1 #Closed

qiluo-msft · 2022-09-16T00:37:56Z

src/sonic-py-common/sonic_py_common/general.py

+            p1.wait()
+            if p1.returncode != 0 or p2.returncode != 0:
+                if p1.returncode != 0:
+                    raise CalledProcessError(returncode=p1.returncode, cmd=cmd, output=output)


output

Could you use output of cmd1? #Closed

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

qiluo-msft · 2022-09-17T22:57:34Z

src/sonic-py-common/tests/test_general.py

+    assert exitcode != 0
+
+def test_getstatusoutput_noshell_pipes():
+    exitcode, output = getstatusoutput_noshell_pipes(dict(args=['echo', 'sonic']), dict(args=['awk', '{print $1}']))


Suggested change

exitcode, output = getstatusoutput_noshell_pipes(dict(args=['echo', 'sonic']), dict(args=['awk', '{print $1}']))

exitcode, output = getstatusoutput_noshell_pipes(['echo', 'sonic'], ['awk', '{print $1}'])

You can still call the function in traditional way if function parameter is *args.
#Closed

qiluo-msft · 2022-09-17T23:05:23Z

src/sonic-py-common/sonic_py_common/general.py

+        raise ValueError("Needs at least 2 processes")
+    # Set up more arguments in every processes
+    for arg in args:
+        arg["stdout"] = PIPE


arg["stdout"]

If caller provide an arg with stdout, you will override? Suggest not to reuse parameter var. #Closed

qiluo-msft · 2022-09-17T23:06:56Z

src/sonic-py-common/sonic_py_common/general.py

+    return (exitcodes, output)
+
+
+def check_output_pipes(*args):


check_output_pipes

Maybe you can keep the name check_output_pipe #Closed

qiluo-msft · 2022-09-17T23:09:28Z

src/sonic-py-common/sonic_py_common/general.py

+    return (exitcodes, output)
+
+
+def check_output_pipes(*args):


Suggested change

def check_output_pipes(*args):

def check_output_pipes(cmd0, *args):

If you will treat the first element of args differently, it's better to define it as normal parameter. I guess your implementation will also works for only 1 cmd0, then you do not need to check len(args) at all. #Closed

qiluo-msft · 2022-09-17T23:10:06Z

src/sonic-py-common/sonic_py_common/general.py

+def check_output_pipes(*args):
+    """
+    This function implements check_output API from subprocess module.
+    Input command includes two or more pipe command.


pipe command -> commands connected by shell pipe(s) #Closed

qiluo-msft · 2022-09-17T23:10:53Z

src/sonic-py-common/sonic_py_common/general.py

+    # to create the pipeline. Closes stdouts to avoid deadlocks.
+    # Ref: https://docs.python.org/2/library/subprocess.html#replacing-shell-pipeline
+    popens = [Popen(**args[0])]
+    for i in range(1,len(args)):


range

range -> xrange #Closed

qiluo-msft · 2022-09-17T23:12:58Z

src/sonic-py-common/sonic_py_common/general.py

+    return (exitcodes, output)
+
+
+def check_output_pipes(*args):


check_output_pipes

you can still call it check_output_pipe, since one pipe is possible. Not a big deal. #Closed

…12108) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it 1. `getstatusoutput` is used without a static string and it uses `shell=True` 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it 1. use `getstatusoutput` without shell=True 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess`

Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it `shell=True` is dangerous because this call will spawn the command using a shell process `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it `os` - use with `subprocess` Use `shell=False` with shell features - redirection: [https://stackoverflow.com/questions/4965159/how-to-redirect-output-with-subprocess-in-python/6482200#6482200?newreg=53afb91b3ebd47c5930be627fcdf2930](https://stackoverflow.com/questions/4965159/how-to-redirect-output-with-subprocess-in-python/6482200#6482200?newreg=53afb91b3ebd47c5930be627fcdf2930) - `|` operator: [https://docs.python.org/2/library/subprocess.html#replacing-shell-pipeline](https://docs.python.org/2/library/subprocess.html#replacing-shell-pipeline)

Related work items: sonic-net#2151, sonic-net#2194, sonic-net#2224, sonic-net#2237, sonic-net#2264, sonic-net#2281, sonic-net#2286, sonic-net#2297, sonic-net#2299, sonic-net#2305, sonic-net#2325, sonic-net#2335, sonic-net#2338, sonic-net#2341, sonic-net#2343, sonic-net#2347, sonic-net#2350, sonic-net#2355, sonic-net#2356, sonic-net#2358, sonic-net#2360, sonic-net#2363, sonic-net#2367, sonic-net#2368, sonic-net#2370, sonic-net#2374, sonic-net#2392, sonic-net#2398, sonic-net#2408, sonic-net#2414, sonic-net#2415, sonic-net#2419, sonic-net#2421, sonic-net#2422, sonic-net#2423, sonic-net#2426, sonic-net#2427, sonic-net#2430, sonic-net#2431, sonic-net#2433, sonic-net#2434, sonic-net#2436, sonic-net#2437, sonic-net#2441, sonic-net#2444, sonic-net#2445, sonic-net#2446, sonic-net#2456, sonic-net#2458, sonic-net#2460, sonic-net#2461, sonic-net#2463, sonic-net#2472, sonic-net#2475, sonic-net#11877, sonic-net#12024, sonic-net#12065, sonic-net#12097, sonic-net#12130, sonic-net#12209, sonic-net#12217, sonic-net#12244, sonic-net#12251, sonic-net#12255, sonic-net#12276, sonic-net#12284

…module (sonic-net#12065) Signed-off-by: maipbui <maibui@microsoft.com> #### Why I did it `getstatusoutput()` function from `subprocess` module has shell injection issue because it includes `shell=True` in the implementation Eliminate duplicate code #### How I did it Reimplement `getstatusoutput_noshell()` and `getstatusoutput_noshell_pipe()` functions with `shell=False` Add `check_output_pipe()` function #### How to verify it Pass UT

Signed-off-by: maipbui maibui@microsoft.com Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it `commands` module is not secure command injection in `getstatusoutput` being used without a static string #### How I did it Eliminate `commands` module, use `subprocess` module only Convert Python 2 to Python 3

) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it 1. `getstatusoutput` is used without a static string and it uses `shell=True` 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it 1. use `getstatusoutput` without shell=True 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess`

Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it `subprocess.Popen()` and `subprocess.run()` 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 `getstatusoutput` is dangerous because it contains `shell=True` in the implementation #### How I did it Replace `os` by `subprocess`, use with `shell=False` Remove unused functions

Signed-off-by: maipbui maibui@microsoft.com Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it `commands` module is not secure command injection in `getstatusoutput` being used without a static string #### How I did it Eliminate `commands` module, use `subprocess` module only Convert Python 2 to Python 3

…ic-net#12107) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it 1. `getstatusoutput` is used without a static string and it uses `shell=True` 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it 1. use `getstatusoutput` without shell=True 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess`

…1740) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [PR (#12065)](#12065) needs to merge first. #### Why I did it 1. `eval()` - not secure against maliciously constructed input, can be dangerous if used to evaluate dynamic content. This may be a code injection vulnerability. 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. 4. `is` operator - string comparison should not be used with reference equality. 5. `globals()` - extremely dangerous because it may allow an attacker to execute arbitrary code on the system #### How I did it 1. `eval()` - use `literal_eval()` 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess` 4. `is` - replace by `==` operator for value equality 5. `globals()` - avoid the use of globals()

Signed-off-by: maipbui maibui@microsoft.com Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it `commands` module is not secure command injection in `getstatusoutput` being used without a static string #### How I did it Eliminate `commands` module, use `subprocess` module only Convert Python 2 to Python 3

) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it 1. `getstatusoutput` is used without a static string and it uses `shell=True` 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it 1. use `getstatusoutput` without shell=True 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess`

Dependency: [PR (#12065)](#12065) needs to merge first. #### Why I did it `commands` module is not protected against malicious input `getstatusoutput` is detected without a static string, uses `shell=True` #### How I did it Eliminate the use of `commands` Use `subprocess.run()`, commands in `subprorcess.run()` are totally static Fix indentation #### How to verify it Tested on DUT [dell_log.txt](https://github.com/sonic-net/sonic-buildimage/files/9561332/dell_log.txt)

xumia · 2023-07-04T06:15:14Z

@maipbui , we need to add the request tag for old release, right?

qiluo-msft · 2023-07-07T20:52:52Z

@xumia No need to backport. Please ping me if you see any blocking issues.

Add getstatusoutput_noshell functions

6a6dd70

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

maipbui mentioned this pull request Sep 13, 2022

[device/dell] Mitigation for security vulnerability #11875

Merged

7 tasks

maipbui requested a review from qiluo-msft September 13, 2022 23:00

maipbui mentioned this pull request Sep 13, 2022

[device/marvell] Mitigation for security vulnerability #11876

Merged

7 tasks

qiluo-msft reviewed Sep 14, 2022

View reviewed changes

Fix getstatusoutput and add check_output_pipe

f7d90d0

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

This was referenced Sep 15, 2022

[device/mellanox] Mitigation for security vulnerability #11877

Merged

[device/celestica] Mitigation for command injection vulnerability #11740

Merged

qiluo-msft reviewed Sep 15, 2022

View reviewed changes

maipbui added 2 commits September 15, 2022 23:58

Fix PR commends and add UT

0e8f4ce

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

Add ref

8e3b376

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

qiluo-msft reviewed Sep 16, 2022

View reviewed changes

maipbui added 3 commits September 16, 2022 03:28

Modify code for python 2

5b256df

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

Modify UT to pass py2

bfd3862

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

Return p1.stdout in exception 1

38ca258

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

qiluo-msft approved these changes Sep 16, 2022

View reviewed changes

Extend 2 functions to handle more pipe commands

fbe43bf

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

maipbui requested a review from qiluo-msft September 17, 2022 21:33

qiluo-msft reviewed Sep 17, 2022

View reviewed changes

qiluo-msft removed Request for 202106 Branch Request for 202111 Branch For PRs being requested for 202111 branch Request for 202205 Branch labels Oct 12, 2022

maipbui mentioned this pull request Oct 29, 2022

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

Merged

7 tasks

xumia added Request for 202205 Branch Request for 202211 Branch Request for 202305 Branch labels Jul 5, 2023

qiluo-msft removed Request for 202205 Branch Request for 202211 Branch Request for 202305 Branch labels Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sonic-py-common] Add getstatusoutput_noshell() functions to general module #12065

[sonic-py-common] Add getstatusoutput_noshell() functions to general module #12065

maipbui commented Sep 13, 2022 •

edited by xumia

Loading

qiluo-msft Sep 14, 2022 •

edited

Loading

qiluo-msft Sep 14, 2022 •

edited

Loading

maipbui Sep 15, 2022

qiluo-msft Sep 15, 2022 •

edited

Loading

qiluo-msft Sep 15, 2022 •

edited

Loading

qiluo-msft Sep 15, 2022 •

edited

Loading

qiluo-msft Sep 16, 2022 •

edited

Loading

qiluo-msft Sep 16, 2022 •

edited

Loading

qiluo-msft Sep 16, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

xumia commented Jul 4, 2023

qiluo-msft commented Jul 7, 2023

	exitcode, output = getstatusoutput_noshell_pipes(dict(args=['echo', 'sonic']), dict(args=['awk', '{print $1}']))
	exitcode, output = getstatusoutput_noshell_pipes(['echo', 'sonic'], ['awk', '{print $1}'])

	def check_output_pipes(*args):
	def check_output_pipes(cmd0, *args):

[sonic-py-common] Add getstatusoutput_noshell() functions to general module #12065

[sonic-py-common] Add getstatusoutput_noshell() functions to general module #12065

Conversation

maipbui commented Sep 13, 2022 • edited by xumia Loading

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

Link to config_db schema for YANG module changes

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

qiluo-msft Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

maipbui Sep 15, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

xumia commented Jul 4, 2023

qiluo-msft commented Jul 7, 2023

maipbui commented Sep 13, 2022 •

edited by xumia

Loading

qiluo-msft Sep 14, 2022 •

edited

Loading

qiluo-msft Sep 14, 2022 •

edited

Loading

qiluo-msft Sep 15, 2022 •

edited

Loading

qiluo-msft Sep 15, 2022 •

edited

Loading

qiluo-msft Sep 15, 2022 •

edited

Loading

qiluo-msft Sep 16, 2022 •

edited

Loading

qiluo-msft Sep 16, 2022 •

edited

Loading

qiluo-msft Sep 16, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading

qiluo-msft Sep 17, 2022 •

edited

Loading