cmd/age: use CONIN$ instead of /dev/tty on Windows #274
Conversation
|
Given that this command is security sensitive, you should have real Windows developer, like @zx2c4, not me review this change. Alex |
|
Awesome, thank you both, this is something that kept biting Windows users! It's kind of unfortunate cmd/age needs to know so much about open terminals. Maybe there's space in x/term for an If @zx2c4 has time to take a look that'd be great, but otherwise I am fairly comfortable with this, the failure mode is not particularly security-sensitive: we don't use this to store a secret, only to read one interactively, so if it doesn't work at most we'll fail to get terminal input. (I guess if permissions are wrong some other user on the system might be able to intercept the input?) I am getting a decent Windows VM set up so I can test these things properly. |
|
|
| if err != nil { | ||
| return nil, err | ||
| } | ||
| tty, err := windows.CreateFile(conin, windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, 0, 0) |
There was a problem hiding this comment.
I don't think you need x/sys/windows. os.Open should work too.
There was a problem hiding this comment.
I haven't tried it in age, but in another program and it didn't work. See golang/go#46164 (comment) for more info.
There was a problem hiding this comment.
@codesoap os.OpenFile works for me (windows 10, go 1.12)
There was a problem hiding this comment.
That's interesting, thanks. This may even be relevant for golang/go#46164. I'll play around with it a bit as well.
There was a problem hiding this comment.
I can now confirm that using os.OpenFile("CONIN$", os.O_RDWR, 0777) works and os.OpenFile("CONIN$", os.O_RDONLY, 0777) does not. Cool find! However, this seems quite odd to me, since CONIN$ is only supposed to read anyway. Is this to be considered a bug of Windows?!
There was a problem hiding this comment.
That's odd, SetConsoleMode says it only needs GENERIC_READ access. It would be nice to know which syscall in term.ReadPassword was throwing the access denied error.
There was a problem hiding this comment.
If I use the "long way" of reading a password with x/term, we can see, that the error seems to originate from MakeRaw's implementation on Windows. So I guess you are correct and SetConsoleMode probably is the culprit. Here is a demo: https://play.golang.org/p/qRpuBKj2poq
|
If that’s the case we can drop the build tagged files and just check runtime.GOOS to pick the filename.
… On Jun 1, 2021, at 21:14, Andrew Ekstedt ***@***.***> wrote:
@magical commented on this pull request.
In cmd/age/read_password_windows.go:
> +package main
+
+import (
+ "fmt"
+ "os"
+
+ "golang.org/x/sys/windows"
+ "golang.org/x/term"
+)
+
+func readPassphraseFromTerminal() ([]byte, error) {
+ conin, err := windows.UTF16PtrFromString("CONIN$")
+ if err != nil {
+ return nil, err
+ }
+ tty, err := windows.CreateFile(conin, windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, 0, 0)
I don't think you need x/sys/windows. os.Open should work too.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Oh my bad, it actually needs to be OpenFile to get r/w permission.
https://play.golang.org/p/FTJwXE0o6AP
Seems to work. I don't know if any of the extra stuff os.OpenFile does
could come back to bite us, but the simplification seems nice.
On Tue, Jun 1, 2021, 12:26 Filippo Valsorda ***@***.***>
wrote:
… If that’s the case we can drop the build tagged files and just check
runtime.GOOS to pick the filename.
> On Jun 1, 2021, at 21:14, Andrew Ekstedt ***@***.***> wrote:
>
>
> @magical commented on this pull request.
>
> In cmd/age/read_password_windows.go:
>
> > +package main
> +
> +import (
> + "fmt"
> + "os"
> +
> + "golang.org/x/sys/windows"
> + "golang.org/x/term"
> +)
> +
> +func readPassphraseFromTerminal() ([]byte, error) {
> + conin, err := windows.UTF16PtrFromString("CONIN$")
> + if err != nil {
> + return nil, err
> + }
> + tty, err := windows.CreateFile(conin,
windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil,
windows.OPEN_EXISTING, 0, 0)
> I don't think you need x/sys/windows. os.Open should work too.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#274 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABK3MYMVGZH73LHBFKAKL3TQUX7FANCNFSM453NFTSA>
.
|
|
The linked issue mentions Open, which is read only. If you didn't try OpenFile I will this evening.
… On Jun 1, 2021, at 21:45, Richard Ulmer ***@***.***> wrote:
@codesoap commented on this pull request.
In cmd/age/read_password_windows.go:
> +package main
+
+import (
+ "fmt"
+ "os"
+
+ "golang.org/x/sys/windows"
+ "golang.org/x/term"
+)
+
+func readPassphraseFromTerminal() ([]byte, error) {
+ conin, err := windows.UTF16PtrFromString("CONIN$")
+ if err != nil {
+ return nil, err
+ }
+ tty, err := windows.CreateFile(conin, windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, 0, 0)
I haven't tried it in age, but in another program and it didn't work. See golang/go#46164 (comment) for more info.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Sounds good. I haven't tried |
I'm intending to solve #128 with this pull request.
The solution for Windows was kindly provided to me by @alexbrainman at golang/go#46164 (comment).
I have to admit, though, that I do not fully understand what is going on, because I am unfamiliar with Windows and it's APIs. As far as I understood, using
CONIN$is also what is recommended by Microsoft: