BAN-B602 · Detected subprocess `popen` call with shell equals `True`

Detected subprocess popen call with shell equals True BAN-B602

Security

Major

a year ago — a year old

subprocess call with shell=True identified, security issue.

1539            title = ctx.message.attachments[0].filename
1540        # Tries to put it in queue, if it cannot parse the duration
1541        try:
1542            await ctx.voice_state.songs.put({"url": "local@" + filename, "title": title, "user": ctx.author, "duration": int(float(subprocess.check_output(f"ffprobe -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 \"{filename}\"", shell=True).decode("ascii").replace("\r", "").replace("\n", "")))})1543        except:
1544            return await self.respond(ctx.ctx, loc["messages"]["cannot_add_invalid_file"], color=0xff0000)
1545        # Displaying filename with _ will cause discord to format the text, replace them with \_ to avoid this problem

subprocess call with shell=True identified, security issue.

cogs/commands/misc/music.py

1408                        if "duration" in data:
1409                            await ctx.voice_state.songs.put({"url": search, "title": title, "user": ctx.author, "duration": int(float(data["duration"]))})
1410                        else:
1411                            await ctx.voice_state.songs.put({"url": search, "title": title, "user": ctx.author, "duration": int(float(subprocess.check_output(f"ffprobe -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 \"{search}\"", shell=True).decode("ascii").replace("\r", "").replace("\n", "")))})1412                    except:
1413                        return await self.respond(ctx.ctx, loc["messages"]["cannot_add_invalid_file"], color=0xff0000)
1414                    await self.respond(ctx.ctx, loc["messages"]["added_song"].format(title.replace("_", "\\_")), color=0x1eff00)

subprocess call with shell=True identified, security issue.

cogs/commands/misc/music.py

 447            url = url[6:]
 448            try:
 449                # Try to get the duration of the uploaded file
 450                duration = str(int(float(subprocess.check_output(f"ffprobe -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 \"{url}\"", shell=True).decode("ascii").replace("\r", "").replace("\n", "")))) 451            except:
 452                return "error"
 453            # Return the song object with ffmpeg

Description

Using shell=True can expose you to security risks if someone crafts input to issue different commands than the ones you intended.

Python possesses many mechanisms to invoke an external executable. However, doing so may present a security issue if appropriate care is not taken to sanitize any user-provided or variable input. Subprocess invocation using a command shell is dangerous as it is vulnerable to various shell injection attacks. It is possible for an attacker to craft inputs to issue different commands than the ones you intended, for example: removing a file.

Great care should be taken to sanitize all input in order to mitigate this risk. Calls of this type are identified by a parameter of shell=True being given.

It is recommended to use functions that don't spawn a shell. If you must use them, use shlex.quote to sanitize the input.

Bad practice

import subprocess

subprocess.Popen(cmd, shell=True)  # Sensitive, shell spawned

References:

subprocess.popen
Subprocess security considerations
shlex
Command Injection
OWASP Top 10 2021 Category A03 - Injection
SANS Top 25
CWE-78

junepark678 / GIRRewrite

Bad practice

Recommended

References: