Add advisory for temporary

[5225225]

No description provided.

[KamilaBorowska]

Curiously, this was reported to an author four year ago: stainless-steel/temporary#1.

Author's response was as follows:

Thanks for the feedback! The crate is primarily used for testing purposes, and it serves well in those scenarios. It is also a part of a family of crates relying on random. I agree that, when security is a concern, it might not be appropriate; I recommend switching to tempdir.

[5225225]

Fixed version of the crate has been released, so this crate is now sound. It does have one instance of unsafe, as a from_utf8_unchecked, but that looks sound to me.

The seeding is still very predictable and not using system entropy, but it has a retry mechanism, and will keep on trying until it passes. Removed mention of the predictable names, since that's no longer due to memory unsafety.

[pinkforest]

The downloads are all over the place on older versions of this so people are still downloading the unsound versions.

Could we ask the author perhaps to voluntarily yank the older versions ?

Entropy from a tempfile is interesting catch indeed -

I would wonder a scenario where something is spitting the temporary directory entropy to malicious user unless you can trigger debug / logging out e.g. in a file upload that is often used for temporary files.

There is no indication in the documentation that this should be only limited / used for testing purposes.

Whilst 30k downloads is low I would still encourage people to use sound versions of any tempfile handling.

Nonetheless it would be controversial to merge an advisory without giving the maintainer a heads up / voice specifically about a potential security advisory considering all the time that has passed -

I would prefer hearing the author's response on filing advisory with the recommendation and perhaps see if the maintainer is keen to yank the unsound versions from crates.io

I also would prefer maintainer response re: predictability and perhaps documenting this limitation in the library nonetheless I don't think advisory for that predictability part is not necessarily justified - as was neither proposed here.

@amousset what is your take on advisory on this?

[5225225]

Should I also bump the date forward to today since it's been a while?

[pinkforest]

Date is when the maintainer was made aware of it - not necessarily when the advisory was raised

I believe that date was 22 Aug 2018 so I think this should be used if you could change this that would be great thanks :)

[5225225]

I thought date was used for ordering, wouldn't that cause an advisory to show up backdated 4 years? The README.md says

# Disclosure date of the advisory as an RFC 3339 date (mandatory)
date = "2021-01-31"

It's unclear if that's the disclosure of the issue to the author, or the date we published the advisory.

[pinkforest]

We've specified the date here:
https://github.com/rustsec/advisory-db/blob/main/MAINTAINERS_GUIDE.md

@Shnatsel does backdating like that cause any issues anywhere per @5225225 #1196 (comment)?

[pinkforest]

@5225225 this probably may be also best as informational / unsound ? I am seeking response from the maintainer

[pinkforest]

I've asked for the maintainer to give some feedback re: whether this should be framed as informational
stainless-steel/temporary#3

[5225225]

Well, it's non-contrived code that does (if mem::uninitialized didn't do 0x01 filling) likely leak the contents of memory by creating file names derived from the memory.

Not quite heartbleed, but rather dangerous IMO. I can reasonably see code patterns where this is an issue.

However, it's mitigated against by the fact that mem::uninitialized now does 0x01 filling, so the contents are completely deterministic on newer compilers, and it seemed like compilers just did the computation as if the value was zero, in release mode. (So it wasn't very random, but it wasn't leaking memory contents).

So given that it's not really an issue in newer compilers, or in release mode, an informational seems fair enough. I don't think the advisory should mention the 0x01 filling since that is an implementation detail of mem::uninitialized, and it is free to be removed at any time.

The 0x01 filling was added rust-lang/rust#99182, for reference.

I'll leave it as-is until we hear back about it.

[Shnatsel]

Re backdating: creating advisories with old dates (years ago) doesn't cause any issues with RustSec tooling. They might cause issues for someone somewhere, but I've never heard any complaints about it. So I'm going to assume it's fine.

[pinkforest]

I think as the maintainer has publicly indicated that the crate should not be used beyond testing if the security is concern and since we are not hearing from the maintainer (again) I would tend to veer towards proposing to merge this as-is security issue.

@5225225 could we please add the recommendation per maintainer words to switch to tempdir or other alternative(s) as actionable fix ? We can still keep the patched here too.

[5225225]

Okay, added a few lines at the bottom and linked to tempfile (since tempdir says it's deprecated and replaced with tempfile). Also squashed my commits.

[pinkforest]

Thanks!

[amousset]

Re backdating: creating advisories with old dates (years ago) doesn't cause any issues with RustSec tooling. They might cause issues for someone somewhere, but I've never heard any complaints about it. So I'm going to assume it's fine.

The only issue I currently see is that the atom feed uses it as publication date, so new advisories appear with the old date in the feed readers. We should probably use the publication date there, but it's a bit tricky as currently we would need to resort to getting the info from git.

[pinkforest]

we should have separate published / modified date but that would be API addition as would changing how we've defined date that would be breaking change afaik