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

[Security] Vulnerability report #1773

Closed
Ry0taK opened this issue Jan 3, 2021 · 8 comments
Closed

[Security] Vulnerability report #1773

Ry0taK opened this issue Jan 3, 2021 · 8 comments
Labels
bug A bug.

Comments

@Ry0taK
Copy link

Ry0taK commented Jan 3, 2021

I've sent an email that contains the detail of the vulnerability to . (As I couldn't find proper contact information to report a vulnerability, I used the email from https://blog.burntsushi.net/about/)
Since I believe this vulnerability can be exploited, I'm not publishing the detail here.

@BurntSushi Can you check the inbox, please?

@BurntSushi
Copy link
Owner

Thanks! That email is fine. I'm investigating it, but since it only happens on Windows, it will take me some time to get to it.

@BurntSushi BurntSushi added the bug A bug. label Jan 4, 2021
@BurntSushi
Copy link
Owner

Also fixed by 229d1a8.

@Ry0taK
Copy link
Author

Ry0taK commented May 29, 2021

@BurntSushi Thank you so much for fixing this issue ;)

@xfbs
Copy link

xfbs commented May 29, 2021

This almost sounds like a bug in Windows or std::process::Command to me. Other people that are used to *nix might find this behaviour surprising and introduce similar vulnerabilities. Should this be fixed in the stdlib maybe? Maybe have some method on a Command that optionally resolves the path to an executable like your resolve_binary, overriding the OS's search path (skipping the current working directory)? Just some food for thought (duplicated here because comments on commits might not be the best place).

@BurntSushi
Copy link
Owner

I meant a file a bug against std. The library team did have a brief private conversation about it a couple months ago, and there does seem to be some appetite toward doing something here.

I haven't gotten around to filing an issue, but we are okay doing it publicly. So anyone can create that issue. (And I would invite someone to do it.)

See also: golang/go#38736

@BurntSushi
Copy link
Owner

Note also that I created a CVE for this: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3013

(I sent mail to them to publish the CVE today, so that's pending.)

@Shnatsel
Copy link

Does this also affect ripgrep being used as a crate? If so, it's probably a good idea to also file an advisory at https://github.com/RustSec/advisory-db - that way people relying on a vulnerable version would be notified through cargo audit, cargo deny, etc.

@BurntSushi
Copy link
Owner

@Shnatsel Good idea. You're correct!

Done: rustsec/advisory-db#939

Shnatsel added a commit to rustsec/advisory-db that referenced this issue Jun 14, 2021
…ws (#939)

* crates/grep-cli: add advisory for arbitrary binary execution on Windows

Ref BurntSushi/ripgrep#1773

* drop commented out field

* crates/grep-cli: add more details about mitigation

Instead of dancing around it, we just say it: the main issue is that
std::process::Command will resolve relative binary names with respect to
the CWD first, because it just uses the Windows API for this.

More specifically, we call out the two particular mitigations that are
now in place.

Co-authored-by: Sergey "Shnatsel" Davidoff <shnatsel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug.
Projects
None yet
Development

No branches or pull requests

4 participants