search

YaLTeR / wl-clipboard-rs

A safe Rust crate for working with the Wayland clipboard.
Apache License 2.0
223 stars 16 forks source link

Update insecure dependencies #22

Closed piegamesde closed 2 years ago

piegamesde commented 2 years ago

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

piegamesde commented 2 years ago

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 https://github.com/git/git/commit/e3a434468fecca7c14a6bef32050dfa60534fde6 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 commented 2 years ago

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 (https://github.com/YaLTeR/wl-clipboard-rs/issues/1). Perhaps the new wayland-rs will eventually allow making them more robust; I haven't looked into it yet.

piegamesde commented 2 years ago

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.