jdm / tinyfiledialogs-rs

A Rust binding for the tinyfiledialogs library.
http://sourceforge.net/projects/tinyfiledialogs/
55 stars 22 forks source link

Add security warning to README and lib.rs doc #40

Closed Marcono1234 closed 1 year ago

Marcono1234 commented 1 year ago

Due to tinyfiledialogs trying to support many operating systems and environments, and due to how it is implemented (generating command line strings, generating Python code, ...) it seems to be inherently prone to command injection vulnerabilities, see for example #19.

What do you therefore think about adding a security warning to the README of this project to the documentation of the lib.rs file?

For example something like this:

...

Security Warning

tinyfiledialogs should only be used with trusted input. Using it with untrusted input for example as dialog title or message can in the worst case lead to execution of arbitrary commands.

My intention here is not fearmongering, but instead to indicate to users for which use cases tinyfiledialogs is suitable, and for which it might not.

(CC @vareille; is this information correct? Should there maybe also be a warning in the README of the upstream project?)

jdm commented 1 year ago

I would accept a pull request that added a warning.

vareille commented 1 year ago

Hi tinyfiledialogs rejects ' " ` and generate a warning accordingly. You should update to the latest version 3.15 on sourceforge.

edit: I have just also added a warning at the beginning of all the files in v3.15.1

Marcono1234 commented 1 year ago

Have created #41

Hi tinyfiledialogs rejects ' " ` and generate a warning accordingly.

Ah you are right. But the function quoteDetectedW in the upstream version only checks for " and '; is that intended or an oversight?

But I am not sure if there might be other characters which can also cause problems. Maybe also related is https://github.com/native-toolkit/libtinyfiledialogs/issues/5 (not sure if that is still relevant though).

edit: I have just also added a warning at the beginning of all the files in v3.15.1

Thanks!

vareille commented 1 year ago

quoteDetectedW is only on windows and ` has only an effect in Unix shells. The issue https://github.com/native-toolkit/libtinyfiledialogs/issues/5 with < is a zenity behavior (bug ?). I think they know about it and don't seem to care.

Marcono1234 commented 1 year ago

Also @jdm and @vareille, sorry if this GitHub issue made you feel pressured to add such a security notice. That was not my intention.

To me it seemed tinyfiledialogs hadn't been designed to handle arbitrary input and that as seen by the other issues it does not handle some input very well, but that this just hadn't been explicitly mentioned in the README before. And therefore directly providing potentially untrusted input as dialog message isn't an intended or supported use case of tinyfiledialogs.

If my assessment was incorrect or if you think the security warning added by my pull request is misleading or too alarmist, feel free to adjust it or let me know how I should adjust it.

My intention wasn't to harm the reputation of the Rust bindings project here or tinyfiledialogs in general.