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

Add advisory tempfile #1358

Closed
wants to merge 3 commits into from
Closed

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Aug 14, 2022

Since this crate also advertises itself as being secure and portable it might be feasible to remove recommendations and / or flag an advisory either as informational or regular where the crate makes these guarantees and may not provide them.

Stebalien/tempfile#178

We previously recommended to use tempfile at temporary here: #1196

https://owasp.org/www-community/vulnerabilities/Insecure_Temporary_File

Proposal

Removing the recommendation to use tempfile from the earlier temporary advisory
Also removes advertised reference from tempdir

Should we also file an advisory in addition not recommending it ?

There is no real PoC or anything but 🤷‍♀️

Perhaps we should use the informatonal = "notice" category finally ?

@5225225 and @vks could you review this please ?

@pinkforest pinkforest requested a review from amousset August 14, 2022 08:34
@5225225
Copy link
Contributor

5225225 commented Aug 14, 2022

From what I can tell, the only possible issue is a denial of service. As in, tempfile acts like mkstemp. The advisory is rather vague, if we do go ahead with this, i'd say "it's not secure against denial of service attacks if an attacker can create files in the directory that std::env::temp_dir returns, because an attacker can predict the names going to be created and create files in the temporary directory"

@amousset
Copy link
Member

I don't think we should file an advisory for this one, at least for now. It looks like the security expectations for temporary files creation methods are not clear-cut, and I don't think we should publish an non-actionable advisory for a hypothetical issue.

We could continue discussing possible improvements with the maintainer (as @5225225 just asked in Stebalien/tempfile#178 (comment)).

@5225225
Copy link
Contributor

5225225 commented Aug 14, 2022

Yeah, "don't do it", or at the very least "do it, but only as a "hey we fixed it in this newer version you should all go update"" seems like the best plan.

@tarcieri
Copy link
Member

Yeah, it seems to me like this would be a very noisy advisory with relatively low impact.

Agreed it's fine as encouragement to upgrade. information = "notice" seems OK although several people run with --deny warnings so it would still be high-impact there.

@pinkforest
Copy link
Contributor Author

Yeah agreed, I'll just close and we can re-hash later if needed. As positive note at least we got some upstream fix going.

Thanks all 💜

@pinkforest pinkforest closed this Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants