Nyre221 / Kiview

GNU General Public License v3.0
50 stars 1 forks source link

Dangerous call to bash shell allows command injection #5

Closed n303p4 closed 3 months ago

n303p4 commented 3 months ago

https://github.com/Nyre221/Kiview/blob/c065ebeba10aab0fbfc017f4101cee9636b19479/src/documentviewer.cpp#L42-L47

bash is called here and there's no input sanitization done on the file path.

As an example, if a filename includes the string ' & (cd && rm -rf *) & ', and an unwitting user previews it in Kiview, it'll wipe out the user's entire home directory without warning. Tested this in a VM.

Could avoid this in a number of ways, e.g. by not calling bash. I'm not familiar with QProcess usage, so I don't know what the "proper" way to handle command arguments here is, but as a rule of thumb, the file path itself should be treated as a singular argument, so that it can't accidentally be interpreted as a series of arguments that mean something totally different. You could probably also use LibreOfficeKit to avoid making an external command call in the first place.

Attached is a video illustrating a less damaging version:

Nyre221 commented 3 months ago

Really interesting, I'll see what I can do.

In any case, I don't think it's the call to libreoffice that's the problem. I also use bash to try to figure out the file type if there is no extension present and that is probably the problem in this case.

I will check every part of the code where I used bash and try to fix it.

For now use the flatpak version.

Thank you very much for pointing out this error to me.

Nyre221 commented 3 months ago

@n303p4
Thank you so much once again for reporting the issue.

I eliminated bash -c and gave the arguments directly to the exec and that allowed me to pass the path separately.

In dolphinbridge.cpp I put a check on the presence of some characters so as to avoid using xclip or wl-copy to restore the contents of the clipboard (This is necessary because KIview uses the clipboard to take the path from dolphin).