Fix uncontrolled command line in V4L2

[odaysec]

motioneye/motioneye/controls/v4l2ctl.py

Line 98 in 5158ddc

cmd = f"v4l2-ctl -d {quote(device)} --list-formats-ext | grep -vi stepwise | grep -oE '[0-9]+x[0-9]+' || true"

motioneye/motioneye/utils/__init__.py

Line 664 in 5158ddc

args,

Fix the issue need to ensure that user-provided input is sanitized or validated before being passed to subprocess.run. The best approach is to implement an allowlist of acceptable commands or arguments and reject any input that does not match the allowlist. Additionally, we should avoid using shell=True wherever possible, as it significantly increases the risk of command injection.

Steps to fix:

1. Introduce an allowlist of valid commands or arguments in motioneye/controls/v4l2ctl.py for the list_resolutions function.
2. Validate the device parameter against this allowlist before constructing the cmd string.
3. Modify call_subprocess in motioneye/utils/__init__.py to reject unsafe inputs or enforce stricter validation.
4. Ensure that all paths leading to call_subprocess sanitize or validate user input.

Code that passes user input directly to exec, eval, or some other library routine that executes a command, allows the user to execute malicious code. The following shows two functions. The first is unsafe as it takes a shell script that can be changed by a user, and passes it straight to subprocess.call() without examining it first. The second is safe as it selects the command from a predefined allowlist.

urlpatterns = [
    # Route to command_execution
    url(r'^command-ex1$', command_execution_unsafe, name='command-execution-unsafe'),
    url(r'^command-ex2$', command_execution_safe, name='command-execution-safe')
]

COMMANDS = {
    "list" :"ls",
    "stat" : "stat"
}

def command_execution_unsafe(request):
    if request.method == 'POST':
        action = request.POST.get('action', '')
        #BAD -- No sanitizing of input
        subprocess.call(["application", action])

def command_execution_safe(request):
    if request.method == 'POST':
        action = request.POST.get('action', '')
        #GOOD -- Use an allowlist
        subprocess.call(["application", COMMANDS[action]])

[MichaIng]

IMO there is no point to sanitize call_subprocess. It really is just a wrapper for subprocess.run, changing some defaults. Any harmful code could just call subprocess.run directly, bypassing the sanity check.

But it generally makes sense to sanitize any kind of user input, including output of external commands. I am not 100% sure, but AFAIK v4l2-ctl --list-devices is supposed to list /dev/video[0-9]+ device paths only, so we could do a sanity check with regex, instead of a hardcoded allow list. Regarding security, at least, thanks to shlex.quote(), no matter that the device name is, any unintended expansion or code execution is ruled out, and the whole input is assured to be taken literally as device name argument for the command. Worst that can happen is that v4l2-ctl complains about the device not being a valid. Quite possible that this error is even better than what we could achieve with a sanity check.

I'll turn this into a draft, since with hardcoded example allow losts, it is not ready to be tested, but a discussion basis.

---

Regarding shell=True: yes, we should ban it entirely. Where used, we should convert any logic done in the shell into logic done in Python. We are happily accepting any PR, whether it replaces all or just a single shell=True.