CVE-2023-39018: Assumed code injection vulnerability of `net.bramp.ffmpeg.FFmpeg.<constructor>`

[LetianYuan]

Affected Version
The latest version 0.7.0 and below.

Describe the vulnerability
net.bramp.ffmpeg.FFmpeg.<constructor> is designed to create an FFmpeg object. However, passing an unchecked argument to this API can lead to the execution of arbitrary codes. For instance, following codes can lead to the execution of malicious program:

new FFmpeg("C:/Windows/System32/calc.exe");

To Reproduce
Just execute above codes would reproduce it.

Fix Suggestion
Check the parameter of FFmpeg.<constructor> strictly.

[bramp]

It is by design that a user can provide the path to the binary they wish to call for ffmpeg/avconv

What would you suggest instead? As-in what is a concrete suggestion for more strict checking?

[LetianYuan]

It is by design that a user can provide the path to the binary they wish to call for ffmpeg/avconv

What would you suggest instead? As-in what is a concrete suggestion for more strict checking?

Much thanks for your reply. Actually, we've sent a mail months ago to discuss this problem, but we got no reply.

I'll close this issure right now.

[StayPirate]

CVE-2023-39018 was assigned to this bug. Now that @bramp confirmed this is not a bug, but instead a design choice, someone should ask MITRE to withdraw this CVE ID. I can file the request for that, any objection?

[lzher]

Hi, thanks for your opinion! @bramp @StayPirate

I'm working with @LetianYuan . We have done some research on these kinds of bugs, and we believe it is a bug for the following reasons.

1. This API (FFmpeg.<constructor>) violates the principle of least privilege. The purpose of the API is to instantiate an FFmpeg object for further use. However, it is not necessary to actually run the provided application at the stage of instantiation, because none of the FFmpeg functionalities is required at this point. Thus, the version() call should be omitted from the constructor.
2. Since the API invokes the given application directly, users should do security checks before passing parameters to this API. However, the users can hardly be aware that this API will execute given command right away, since the API name and the javadoc don't give any information about it. As a result, untrusted inputs can be passed to the API in some circumstances, leading to remote command execution.
3. We believe that this kind of implementation obscures the responsibility of security checks between the developers and the users of libraries, which has been confirmed by some developers. For example, the Google Jib library fixed the unnecessary command executions with a file existence check (cf. CVE-2022-25914, and GoogleContainerTools/jib@67fa40b ). Besides, the well-known log4j vulnerability is also introduced because of the unawareness of library users.

Because of the reasons above, I suggest that the version() call in the constructor can be removed or replaced with an execution permission check.

I'd be very happy if you could share your further opinions with me. Thank you!

[bramp]

Thanks for the followup @lzher

1. That sounds reasonable. The main reason it was checked that, is because you only construct the FFmpeg object, when you actually want to invoke it. e.g

   FFmpegBuilder builder =
       new FFmpegBuilder()
           ...
           .done();
   
   FFmpeg ffmpeg = new FFmpeg("/path/to/ffmpeg");
   FFmpegExecutor executor = new FFmpegExecutor(ffmpeg, ffprobe);

   But you are right it could be deferred until the FFmpegExecutor executes it.

2. The user here is the developer. I never expect them to let the user give them a random path to a binary. But you are right, I can document this behaviour, and/or slightly modify it.

3. I think it's pretty clear to the developer that new FFmpeg("/path/to/ffmpeg"); will eventually invoke /path/to/ffmpeg, unlike log4j where a option the developer never saw caused the problem. So I don't think these are remotely similar. This is more akin to saying "Be careful running bash /your/binary because Bash is going to invoke your binary.

Anyways, next steps

1. Clearer documentation
2. Considering removing the version() call from the constructor.

[LetianYuan]

Thanks for your kind reply.

We agree the attack surface is limited. But in some cases developers can allow their users to control a part of the binary path. For example, if a developer allows users to choose different versions of FFmpeg, the version input from untrusted users can be directly concatenate into the path, resulting in an path injection and then arbitrary command execution.

We cannot predict how developers use your APl, so it is important to make sure the API satisfy the principle of least privilege and are well documented.

Thank you again for sharing your opinion with us!

[AlexanderSchuetz97]

Is there any action done to unflag this from the CVE databases?
I am aware that this CVE is smoke and not real...

It still shows up in the vulnerabilty databases...
This may eventually make some people (that only use automated scanning tools and dont care to look into the details) nervous.

[Euklios]

@AlexanderSchuetz97

I suspected this issue...
Is there a CVE for this library, or is it a pattern match? (The CVE mentioned above is for the GoogleContainerTool)

[AlexanderSchuetz97]

https://www.cve.org/CVERecord?id=CVE-2023-39018

IntelliJ is also showing this. Its also shown for v 0.8.0.
whoever submitted this CVE gave a "*" for a version.

Ive read all comments in this issue. By @LetianYuan logic the class "ProcessBuilder" is a vulnerability. I am assumeing that LetianYuan submitted this CVE, if this is not the case then the following is not directed at you but rather whoever submitted it to mitre.

Also the reason why I am assumeing this is or was bad faith is because most vuln trackers flag this as critical and easily exploitable with 0 user interaction. This is really not the case at all. Its almost as if someone malicoisly fed the CVE all the things so it would show up this way.

Could you please remove this CVE before countless engineering hours are wasted explaining to non tech savy people that this CVE is completly harmless?

Comparing a framework that 90% of people intended to write logs to a file where someone considered it smart to parse said log and load random classes from a url if the input matched, to this library where its sole purpose is to run a elf/pe binary with complex arguments is beyond me. It is completely obvieus to everyone immedeatly what this library does and that it at some point will run the provided binary.

99% of users of this library probably extract or download ffmpeg and write it to %TEMP% and then pass this path directly to the library. Anyone who lets the user choose this path arbitrarily does so full well knowing what he is doing.

[Euklios]

@AlexanderSchuetz97 Thanks for the analysis.
@bramp I just contacted Mitre about this; I'll update...

[Euklios]

For all coming here due to the CVE, this was filed in bad faith, and we're trying to work with Mitre to resolve it.
For this to impact you meaningfully, you must instantiate the FFmpeg object without using the instance.

Additionally, you would have to let the user override the application you referenced to inject your executable or allow the user to upload his executable into a directory with higher priority on the PATH.
This means:

• The attacker has access to the service user and can't escape it (no problem)
• The attacker is root (You have a problem, but it's not this library)
• You allowed users to override arbitrary system files (You have a problem, but it's not this library)

You will have to execute shell commands to use this library meaningfully (and the FFmpeg class in particular).
Saying this is a thread is the same as saying sudo bash -c "rm -rf /*" is one.

Last but not least, neither the CVE description nor the version is correct.