Switch to golang.org/x/sys/execabs for windows security fix

[Jacalz]

Description:

The os/exec package on Windows will match the behaviour of cmd.exe by considering the local folder as a primary part of the path. This means that a malicious binary with the same name, in the current folder, would be run instead of the expected binary in the system path. Due to the backwards compat being an issue, this could not be fixed within ox/exec before Go v2. See https://blog.golang.org/path-security for more info.

This should not change any behaviour for anything else than Windows platforms. I think we should target this for develop as it potentially could break things if we run any binaries within the local path (easily fixed by adding ./ in the code). It did not look like any of the use cases here ran binaries in the current folder, so I think we should be fine. I do however not have access to any Windows computer to test it on at this point in time.

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[Jacalz]

As a further note, I have switched everything to use the new package for the sake of consistency. Some of the vendored code uses "os/exec" still, so I don't think we gain any binary size improvements from doing this.

[Jacalz]

From the vendor folder, these are the deps that still rely on os/exec. I will get a patch up for goinfo.

vendor/github.com/godbus/dbus/v5/conn_darwin.go:        "os/exec"
vendor/github.com/godbus/dbus/v5/conn_other.go: "os/exec"
vendor/github.com/lucor/goinfo/report/go_env.go:        "os/exec"
vendor/github.com/lucor/goinfo/report/go_module.go:     "os/exec"
vendor/github.com/lucor/goinfo/report/go_version.go:    "os/exec"
vendor/github.com/lucor/goinfo/report/os_darwin.go:     "os/exec"
vendor/github.com/lucor/goinfo/report/os_windows.go:    "os/exec"
vendor/github.com/lucor/goinfo/report/os_xdg.go:        "os/exec"

[Jacalz]

I have lucor/goinfo#3 open now.

[Jacalz]

Should I get a PR up for godbus as well? There is no extra security to gain for us as we just use it on other platforms, but there could potentially be a use case for knowing that we do not depend on "os/exec" in terms of binary size.

[lucor]

LGTM, just need to update github.com/lucor/goinfo@fce12a3 from your PR

[lucor]

Should I get a PR up for godbus as well? There is no extra security to gain for us as we just use it on other platforms, but there could potentially be a use case for knowing that we do not depend on "os/exec" in terms of binary size.

Probably having a PR for godbus does not hurt, even if as you said it does not add any extra security due to the target platform.
In any case I would not block this PR by the godbus one.

[Jacalz]

Should I get a PR up for godbus as well? There is no extra security to gain for us as we just use it on other platforms, but there could potentially be a use case for knowing that we do not depend on "os/exec" in terms of binary size.

Probably having a PR for godbus does not hurt, even if as you said it does not add any extra security due to the target platform.
In any case I would not block this PR by the godbus one.

Yeah, you are right. Always good to be a helpful open source citizen and contribute upstream where possible :)
Agreed, I think this should be fine to re-review now that goinfo is updated again.

[andydotxyz]

Please don’t delete khrplatform.h

[andydotxyz]

Should I get a PR up for godbus as well?

Yeah good idea

[andydotxyz]

I have switched everything to use the new package

I think updating unrelated dependencies should be a different PR - it requires a lot more testing and if there is an issue we would surely address them separately

[Jacalz]

I have switched everything to use the new package

I think updating unrelated dependencies should be a different PR - it requires a lot more testing and if there is an issue we would surely address them separately

Sorry if I was a bit vague here. The blog post above mentions that all of the golang.org/x/ packages have been updated to use the security fix as many of them rely on the execution of commands. I have not updated any other dependencies at all and they are thus not unrelated to this change.

Please don’t delete khrplatform.h

If it was deleted, it must have happened when running go mod vendor. We probably need a better way to get that in there. We can't have it break each time we update the modules.

[andydotxyz]

If it was deleted, it must have happened when running go mod vendor. We probably need a better way to get that in there. We can't have it break each time we update the modules.

yes. Our old approach (fyne vendor) could work, but we stopped using it. I filed a bug upstream some time ago go-gl/gl#140

[Jacalz]

If it was deleted, it must have happened when running go mod vendor. We probably need a better way to get that in there. We can't have it break each time we update the modules.

yes. Our old approach (fyne vendor) could work, but we stopped using it. I filed a bug upstream some time ago go-gl/gl#140

Can we not just add the file upstream instead of here then?

[andydotxyz]

Can we not just add the file upstream instead of here then?

Maybe you need to read the ticket more carefully… the file exists upstream but is removed during vendoring. I suggested they take the same workaround approach as GLFW did that allowed us to stop using our own vendor script

[Jacalz]

Can we not just add the file upstream instead of here then?

Maybe you need to read the ticket more carefully… the file exists upstream but is removed during vendoring. I suggested they take the same workaround approach as GLFW did that allowed us to stop using our own vendor script

Ah, sorry. Makes sense. Will add the file back in the meantime.

[Jacalz]

I have added back the file now.