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

Update insecure dependencies #22

Closed
piegamesde opened this issue Mar 5, 2022 · 3 comments · Fixed by #23
Closed

Update insecure dependencies #22

piegamesde opened this issue Mar 5, 2022 · 3 comments · Fixed by #23
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@piegamesde
Copy link
Contributor

This crate currently depends on nix 0.18 and transitively on nix 0.17. Apparently, those are potentially insecure: nix-rust/nix#1541 https://rustsec.org/advisories/RUSTSEC-2021-0119 . According to the advisory, this can be fixed by simply updating the affected dependencies.

@YaLTeR YaLTeR added enhancement New feature or request good first issue Good for newcomers labels Mar 5, 2022
@piegamesde
Copy link
Contributor Author

I gave this a try myself, and it got hairy rather quickly :D

The bump in nix made the fork call unsafe, which is used in utils.rs. This is problematic because this library declares deny(unsafe_code). Moreover, the safety requirements for that method call are not met: we need to switch from execvp to execv. See git/git@e3a4344 and https://man7.org/linux/man-pages/man7/signal-safety.7.html for more details.

I did not want to do a PATH lookup, so I tried calling /usr/bin/env cat instead. Unfortunately this made the copy_multi_test test fail even more than before (at the moment, it already fails on my machine with latest master). I suppose that this may have to do with the file descriptor redirects the code is doing, or that I just know too little about Unix and this code base.

@YaLTeR
Copy link
Owner

YaLTeR commented Mar 6, 2022

The bump in nix made the fork call unsafe, which is used in utils.rs. This is problematic because this library declares deny(unsafe_code).

Oh yeah, right. I probably postponed updating nix for that reason. I think it's okay to introduce unsafe for that part.

Moreover, the safety requirements for that method call are not met: we need to switch from execvp to execv.

Uh oh, that sounds like my mistake when I was writing the fork code.

I did not want to do a PATH lookup, so I tried calling /usr/bin/env cat instead.

I guess just calling /usr/bin/cat should be enough? If we're assuming env is in /usr/bin then probably cat is there too.

Unfortunately this made the copy_multi_test test fail even more than before (at the moment, it already fails on my machine with latest master).

Yeah, unfortunately those tests are pretty flaky (#1). Perhaps the new wayland-rs will eventually allow making them more robust; I haven't looked into it yet.

@piegamesde
Copy link
Contributor Author

I suppose that this may have to do with the file descriptor redirects the code is doing, or that I just know too little about Unix and this code base.

It was the latter. I managed to make it work with env; on my first attempt I had forgotten about the zeroth command argument.

I guess just calling /usr/bin/cat should be enough? If we're assuming env is in /usr/bin then probably cat is there too.

No, distributions vary rather wildly in their locations for binaries. Hardcoding absolute paths should be avoided. /usr/bin/env is a notable exception, because it is required for bootstrapping in cases like this one.

@YaLTeR YaLTeR closed this as completed in #23 Mar 6, 2022
piegamesde added a commit to piegamesde/cli-clipboard that referenced this issue Mar 16, 2022
piegamesde added a commit to piegamesde/cli-clipboard that referenced this issue Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants