Use interactive mode for darwin #26
Conversation
This prevents the password from being leaked.
| } | ||
|
|
||
| // Write the command to stdIn. | ||
| command := fmt.Sprintf("add-generic-password -U -s %s -a %s -w %s\n", service, username, password) |
There was a problem hiding this comment.
Hi, I completely lost track of this PR. Sorry about that.
I'm a bit concerned about this because the user input is no longer parameterized meaning you can change the command that is executed here. So it solves one security issue but introduces another, and I'm not sure about the trade off.
There was a problem hiding this comment.
I'm not sure if there exists a Golang version of this but shellescape (https://github.com/xxorax/node-shell-escape/blob/master/shell-escape.js) is pretty robust in protection against injection attacks
There was a problem hiding this comment.
Drive-by comment: darwin keychain could probably be switched to use the DLL like this python lib and there wouldn't be any worries about CLI usage.
There was a problem hiding this comment.
keybase/go-keychain uses cgo to communicate with the macOS Keychain API in the Security.framework. This also has the benefit of isolating keychain item access to the signed go binary using it (instead of the /usr/bin/security binary).
There was a problem hiding this comment.
This would also solve the referenced issue from kooky.
There was a problem hiding this comment.
I'm not sure if there exists a Golang version of this but shellescape (https://github.com/xxorax/node-shell-escape/blob/master/shell-escape.js) is pretty robust in protection against injection attacks
Potentially something like shellescape could work here.
There was a problem hiding this comment.
cgo here would make cross-compilation for darwin quite rough.
There was a problem hiding this comment.
I'm not sure if there exists a Golang version of this but shellescape (https://github.com/xxorax/node-shell-escape/blob/master/shell-escape.js) is pretty robust in protection against injection attacks
Potentially something like shellescape could work here.
This is input for the security tool not a shell and would likely require a different kind of escaping. I can't test it without access to a macos machine though.
| "-i", // start in interactive mode to be able to pass the password using stdIn. | ||
| ) | ||
|
|
||
| stdIn, err := cmd.StdinPipe() |
There was a problem hiding this comment.
There is no need for the io import
EOT (0x04 / Ctrl-D) signals EOF (might be unnecessary)
(untested)
command := fmt.Sprintf("add-generic-password -U -s %s -a %s -w %s\n\x04", service, username, password)
cmd.Stdin = strings.NewReader(command)
|
@Piccirello is it correct that this PR can be closed, since an alternative implementation to solve the same problem has been merged from PR #65 ? |
|
Indeed, this PR is no longer needed. |
This fixes issue #24 . I needed to implement this library for a project and saw this open security issue. Because that project needs to ship I made a fix for it.
I tried to think of a way of testing this but I am ensure how to add an automatic test for it and I see no other tests that test on darwin.
Please let me know if I can make improvements to code style.