vlachoudis / bCNC

GRBL CNC command sender, autoleveler and g-code editor
GNU General Public License v2.0
1.54k stars 528 forks source link

Use Subprocess instead of os #1740

Open bosd opened 1 year ago

bosd commented 1 year ago

An quick attempt to silence B605: start_process_with_a_shell.

This rule looks for the spawning of a subprocess using a command shell. This type of subprocess invocation is dangerous as it is vulnerable to various shell injection attacks. Great care should be taken to sanitize all input in order to mitigate this risk. Calls of this type are identified by the use of certain commands which are known to use shells.
bosd commented 1 year ago

Just tested this one on python3.10 on windows 10. Working ok.

Harvie commented 1 year ago

Can you please explain how is this "safer" ?

bosd commented 1 year ago

Sorry, I cannot. I'm not very knowledgable on the subject. Basically this pr is to silence a "critical" codefactor.io complaint. I'm utiliting the suggested fix from codefactor.io.

I'll paste here the complaint and explanation.

This rule looks for the spawning of a subprocess using a command shell. This type of subprocess invocation is dangerous as it is vulnerable to various shell injection attacks. Great care should be taken to sanitize all input in order to mitigate this risk. Calls of this type are identified by the use of certain commands which are known to use shells.

Example of insecure code:

import os

os.system('/bin/echo suspicious code')

The subprocess module provides more powerful facilities for spawning new processes and retrieving their results; using that module is preferable to using this function.

Example of secure code:

import subprocess

subprocess.call('/bin/echo suspicious code', shell=False)

Further Reading

The Python Standard Library - subprocess - Frequently Used Arguments

Bandit - B605: start_process_with_a_shell

bosd commented 1 year ago

@Harvie Can you merge this?

bosd commented 1 year ago

Ping @Harvie

bosd commented 1 year ago

Ping @Harvie

Harvie commented 1 year ago

Yeah, i am still not sure whether this is actualy making something safer or just removing actual useful feature, because some automated tool thinks it might be unsafe without understanding context...

bosd commented 1 year ago

Did'nt notice any feature removal.. if so, we an always revert.