Update Builder (or create_helper) to check that the supplied prefix and suffix don't contain /

[cipriancraciun]

As the title says, currently the code doesn't make any validations if the prefix or suffix are valid path component values (i.e. they don't contain a slash or \0).

(If needed I can provide the patch.)

[Stebalien]

So, filtering \0 shouldn't be necessary as the OS will do that. Really, we can't reasonably build a blacklist for every OS so we might as well delegate.

In retrospect, we probably should have checked for path separators. However, the documentation currently explicitly allows them.

[cipriancraciun]

@Stebalien As explained in one of the previous issues, my main usage for this library is in a runtime for a (shell) scripting language, and thus many of the parameters sent to tempfile could be under the direct control of the "user" (which sometimes can be an "attacker")...

Now I understand that all developers should do their best in sanitizing the user inputs, before passing these values to the underlying libraries. However this imposes extra burden on those developers (which is the first reason they use a library and not write their own). Moreover in order to "perfectly" sanitize the inputs, one must have in-depth knowledge of how said library would use these inputs, which again adds development overhead.

Imagine one uses Rust and the tempfile library to create an application that has the set UUD bit set (i.e. runs as root), and fails to sanitize some prefix or suffix (which are exposed in one way or another as arguments) not to contain any slashes. Now an "attacker" could just try to miss-use these arguments to create files outside the "temporary" directory (and target for example the /etc/cron.d folders). (There are countless CVE's that follow this pattern in which an application that happens to run as root fails to sanitize inputs, and thus exposes the system to all kinds of issues.)

Therefore, especially since tempfile is so popular, I think the library should do its best to enforce some basic "sanity checkups".

[Stebalien]

I agree but Builder::new().prefix("foo/bar").tempfile()? is valid and potentially useful so I'm worried someone is already using it. However, I really do agree that we shouldn't allow that and users should just use tempfile_in.

@KodrAus thoughts? I'm really tempted to fix this as a security oversight.

[KodrAus]

@Stebalien Personally, I think changing the behaviour here to prevent the prefix and suffix from altering the path is reasonable. I wouldn't necessarily expect .prefix("foo/bar") to do that or try use it that way myself in any case.

[marcospb19]

... thus many of the parameters sent to tempfile could be under the direct control of the "user" (which sometimes can be an "attacker")...

I usually check all paths before passing to functions.

This one was never a pain point to me, but I also never relied on this behavior.

---

Non-breaking alternate solution:

fn prefix_checked(...) -> Option<_>
fn suffix_checked(...) -> Option<_>

Having _checked communicates that there is a check to be done in prefix and suffix, but only users who are re-reading the docs will notice.

Another one: deprecate .prefix() and .suffix(), add .with_prefix() and .with_suffix() to Builder.

[Stebalien]

Hm. I like that second option actually...

[marcospb19]

🤔 hmmm, one problem tho, people might expect all .with_prefix and .with_prefix_in functions to be consistent on whether they perform this check or not.

And it looks like the existing with_prefixs don't, so this one would be the only checked one.

(Same for suffix.)

[Stebalien]

Dammit! The ones I just added.