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

icmd: replace all usages of os/exec with golang.org/x/sys/execabs #218

Merged
merged 2 commits into from
May 15, 2021

Conversation

thaJeztah
Copy link
Contributor

@thaJeztah thaJeztah commented Mar 14, 2021

this is a follow-up to #217, opening as draft because the first 3 commits are from that pull request; I thought it was good to open this change as a separate PR for visibility.

go.mod: golang.org/x/tools v0.1.0

Project now started to do releases; this one was related to a
security issue on Windows, replacing all usages of os/exec with
golang.org/x/sys/execabs.

icmd: replace all usages of os/exec with golang.org/x/sys/execabs

Following the changes in Go, and golang.org/x/tools themselves, this change
ensures that packages using exec.LookPath or exec.Command to find or run
binaries do not accidentally run programs from the current directory when
they mean to run programs from the system PATH instead.

@thaJeztah thaJeztah marked this pull request as draft March 14, 2021 11:52
@thaJeztah thaJeztah force-pushed the secure_exec branch 2 times, most recently from 0bc7e7e to 762f31e Compare March 14, 2021 11:59
@dnephin
Copy link
Member

dnephin commented Mar 14, 2021

https://blog.golang.org/path-security is interesting!

#219 should fix the test-windows job.

This is marked as a draft, but it seems to be working. Anything left to do?

Project now started to do releases; this one was related to a
security issue on Windows, replacing all usages of os/exec with
golang.org/x/sys/execabs.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Contributor Author

Anything left to do?

Nope! I just rebased to get rid of the other commits (well, doesn't really change, but it's confusing on GitHub otherwise); moving it out of draft

@thaJeztah thaJeztah marked this pull request as ready for review March 14, 2021 19:00
@thaJeztah
Copy link
Contributor Author

Blogpost is indeed interesting; I was aware of similar things with dll's on Windows, but guess this one is a typical case of "common knowledge that we forgot about"; thought it wouldn't hurt to follow their recommendations (IMO, os/exec could just as well be updated, but probably counts as a "breaking change", so 🤷‍♂️ )

@thaJeztah
Copy link
Contributor Author

oops, sorry, looks like I pushed too many times, and you're out of credits 😬, hope that's only "for the day", and not for the rest of the month 😅

Screenshot 2021-03-14 at 20 03 24

Following the changes in Go, and golang.org/x/tools themselves, this change
ensures that packages using exec.LookPath or exec.Command to find or run
binaries do not accidentally run programs from the current directory when
they mean to run programs from the system PATH instead.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you!

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.

2 participants