Make visudo into a forbid(unsafe_code) module.

[squell]

visudo uses unsafe code at two spots n the fn create_temporary_dir. While visudo isn't part of the security critical boundary, it would be nice to remove them (or move them into the system module).

[dd-dreams]

Is https://docs.rs/tempfile/latest/tempfile/fn.tempdir.html a good replacement? It's a small crate.
Otherwise, we could move the function to system module, or we can remove unsafe completely by just implementing a temporary directory pretty easily using env::temp_dir and creating a struct with custom drop.

[pvdrz]

I'd lean to use an std function instead (I"m not sure if https://doc.rust-lang.org/std/env/fn.temp_dir.html does what we want). Adding a dependency that has unsafe code for a single function feels like putting the issue under the rug.

[dd-dreams]

I have mis-copied before, I meant the link above I provided. You can use a std function, though I didn't find a way to use mkdtemp in std lib. You can create a temp dir manually.

[pvdrz]

yeah we would have to come up with a random name for the directory ourselves and I'm not sure what would be the proper requirements for that. Maybe we could "just" move the mktemp wrapper to sudo and expose it so visudo can use it 🤔

[squell]

I agree that just incorporating an extra dependency that uses the same kind of code would just be "sweeping it under the rug". One easy way would be to move it to system. Or we could investigate if fn.temp_dir suffices (but probably it doesn't or we would have already used that?)

[pvdrz]

the issue with temp_dir is that it gives us the path to the temporary directory (/tmp/ in linux) but mktemp creates one random directory under the temporary directory.

So we could, in principle, call temp_dir and then mkdir a new directory with a random name under the temporary directory, but it could be a possible source of vulns.

I think moving this wrapper to system is the best solution.

[dd-dreams]

temp_dir could suffice also because I'm not seeing any real danger implementing it ourselves, but moving it to system is also a good solution.

[pvdrz]

The advantage of just moving it to system is that we have the peace of mind that sudo has been using the same function
without issues for years.

[dd-dreams]

Don't think its a good argument, but it's up to the maintainer.

[pvdrz]

why?