Open LetianYuan opened 1 year ago
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?
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.
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?
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.
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.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!
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
```java
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.
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!
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.
@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)
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.
@AlexanderSchuetz97 Thanks for the analysis. @bramp I just contacted Mitre about this; I'll update...
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:
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.
Minor update: The CVE is now marked as disputed. From the CVE entry: This is disputed by multiple third parties because there are no realistic use cases in which FFmpeg.java uses untrusted input for the path of the executable file.
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:To Reproduce Just execute above codes would reproduce it.
Fix Suggestion Check the parameter of
FFmpeg.<constructor>
strictly.