godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.9k stars 20.16k forks source link

Possibly security issues found by FlawFinder(Mitre CWE) #30452

Open qarmin opened 5 years ago

qarmin commented 5 years ago

Godot version: 3.2.dev.custom_build. 56269e2db

Issue description: I recently checked Godot with FlawFinder and it found a lot of possible security issues. Some can be a false positives or informational, but I think that is good to look at it.

Command(go to Godot folder first) flawfinder -S -H * > /home/rafal/flawfinder.html

Report in HTML(just delete txt from file name) - flawfinder2.html.txt and QT Creator tasks(also delete txt from file name) - flawfinder.tasks.txt

To properly configure and QT Creator tasks, change each /home/rafal/Pulpit/godot/ in tasks file to your own Godot destination and open QT Creator with parameter qtcreator filawfinder.tasks (extension must be exactly tasks)

Some of CWE reports

vfprintf: If format strings can be influenced by an attacker, they can be exploited (CWE-134). Use a constant for the format specification.

vsnprintf: If format strings can be influenced by an attacker, they can be exploited, and note that sprintf variations do not always \0-terminate (CWE-134). Use a constant for the format specification.

sprintf: Does not check for buffer overflows (CWE-120). Use sprintf_s, snprintf, or vsnprintf.

readlink: This accepts filename arguments; if an attacker can move those files or change the link content, a race condition results. Also, it does not terminate with ASCII NUL. (CWE-362, CWE-20). Reconsider approach.

creikey commented 5 years ago

Almost all of these issues relate to Godot passing around files as strings, then it reporting that in a multi-threaded situation the actual file content that the strings are pointed to could've been replaced by an attacker. I feel as if this is only relevant if said attacker had control over the internals of a Godot game, and therefore executable access, so these issues don't even matter in a case where the attacker is trying to elevate himself to running customized code with a higher privilege. As long as the networking code does not allow for remotely executing gdscript code, then there are still no remote vulnerabilities I believe.

Tazshelby2016 commented 4 years ago

Now that the security tab is functional in GitHub, it may be better to move this there.

KoBeWi commented 3 years ago

What is the status of this? I guess the report is still valid, but is it relevant? (See 2 comments above)