Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(security): avoid shell injection in open-tilix plugin #2155

Merged
merged 2 commits into from Nov 6, 2023

Conversation

taoky
Copy link
Contributor

@taoky taoky commented Mar 2, 2023

I first reported this issue in https://bugs.archlinux.org/task/77698.

Currently tilix's open-tilix plugin for nautilus uses subprocess.call() with shell=True. However it fails to sanitize input data (filename) correctly, thus causing possible shell injection when filename contains " or `, etc.

This PR tries to solve this issue by using subprocess.Popen() and avoiding invoking shell:

  • Popen constructor recommends to use shutil.which() to get actual path of executable (requires Python 3.3+).
  • For REMOTE_URI_SCHEME part, subprocess security considerations recommends to use shlex.quote() to escape path (also requires Python 3.3+, and it may still have security issue when the shell is not POSIX-compliant).

@ximion
Copy link
Collaborator

ximion commented Apr 20, 2023

Can you adjust the patch so it works on top of the Nautilus plugin updates in master? Thanks for looking into this and creating this patch!

@taoky
Copy link
Contributor Author

taoky commented Apr 20, 2023

Can you adjust the patch so it works on top of the Nautilus plugin updates in master? Thanks for looking into this and creating this patch!

OK, the conflict has been resolved.

@ximion
Copy link
Collaborator

ximion commented Nov 6, 2023

Are you sure? It appears there's still a conflict and this PR can't be merged right now...

@taoky
Copy link
Contributor Author

taoky commented Nov 6, 2023

Are you sure? It appears there's still a conflict and this PR can't be merged right now...

This PR page shows "This branch has no conflicts with the base branch". I don't know where the "conflict" is.

image

And the "incomplete" label is incorrect - this PR is a complete work and is just waiting for reviewing & merging.

@ximion
Copy link
Collaborator

ximion commented Nov 6, 2023

Ah, the problem is that you have a merge commit in thie PR, so rebase does not work. But I can squash down the PR and merge it that way.

@ximion ximion merged commit f253b84 into gnunn1:master Nov 6, 2023
@ximion
Copy link
Collaborator

ximion commented Nov 6, 2023

Thank you for the patch! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants