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

chisel client - Avoiding plain-text password when http_proxy #340

Open
yaakov-berkovitch opened this issue Mar 9, 2022 · 6 comments
Open

Comments

@yaakov-berkovitch
Copy link

Hello,

This is not a real issue or bug, but a question, even if I think it violates some basic security rules.

We are using chisel v1.7.6.
I'm wondering if there is a way to avoid passing a plain-text password in the chisel client command line. After starting the chisel client, anyone that list the running processes will see the command arguments and so the clear password.

I didn't find a way to hide this command or the password from users, so any help with this concern will be very helpful.

Thanks.

@jpillora
Copy link
Owner

Good point, at this stage, no way to hide it from the process args (ps aux will display it)

Easiest change to fix this is to use an environment variable fallback for the --proxy option, like CHISEL_HTTP_PROXY and then define this env var in a systemd unit file (or similar). Feel free to raise a PR, will merge it

@yaakov-berkovitch
Copy link
Author

yaakov-berkovitch commented Mar 10, 2022

Edited.
Thanks @jpillora, but your workaround is not working. The environment variable is evaluated and then passed to the command. So the password still appears .
There are 2 simple solutions I would propose:

  • to use environment variables for proxy server user and password. If defined use them to authenticate the proxy server else use the command line ARGS if exist
  • to accept password argument but, immediately upon startup, overwrite argv with a blank value, effectively hiding the secret

What do you think ?

@tman204
Copy link

tman204 commented Mar 14, 2022

I am new to Go, so my coding skills are weak, but I had this same problem, and solved it with these edits. This may or may not fit your use case.

$ git diff share/settings/user.go
diff --git a/share/settings/user.go b/share/settings/user.go
index 40b2c4e..46a007b 100644
--- a/share/settings/user.go
+++ b/share/settings/user.go
@@ -2,7 +2,11 @@ package settings
 
 import (
        "regexp"
+       "syscall"
+       "fmt"
        "strings"
+
+       "golang.org/x/term"
 )
 
 var UserAllowAll = regexp.MustCompile("")
@@ -10,7 +14,21 @@ var UserAllowAll = regexp.MustCompile("")
 func ParseAuth(auth string) (string, string) {
        if strings.Contains(auth, ":") {
                pair := strings.SplitN(auth, ":", 2)
-               return pair[0], pair[1]
+               if pair[1] == "" {
+                 fmt.Print("*** Enter session password: ")
+                 bytePassword, err := term.ReadPassword(int(syscall.Stdin))
+                 fmt.Printf("\n\n")
+                 if err != nil {
+                   return "", ""
+                 }
+                 var password string = string(bytePassword)
+                 return pair[0], strings.TrimSpace(password)
+               } else {
+                 var password string = pair[1]
+                 return pair[0], strings.TrimSpace(password)
+               }
+               //fmt.Println("user", pair[0])
+               //fmt.Println("pass", strings.TrimSpace(password))
        }
        return "", ""
 }

It allows one to specify --user "user:pass" as usual, however, if you specify --user "user:" then it will prompt you for a session password to use. This prevents this password from being discovered via ps. HTH

@yaakov-berkovitch
Copy link
Author

@tman204 your solution is also a good approach, and by reading the password from stdin will indeed make it invisible using ps.
I didn't find a PR for this change, why not creating a PR and having your changes part of releases ?
@jpillora your advise would be very appreciated.

@ayelet-ack
Copy link

ayelet-ack commented May 25, 2022

Added a PR to fix this issue: #361
And also another PR to fix an issue we found with the re-connection mechanism: #362
@jpillora could you please review & merge them? Thanks!

@ayelet-ack
Copy link

ayelet-ack commented Aug 11, 2022

@jpillora I opened a PR with a fix for this issue a while ago: #361
could you please merge? Thanks!
@yaakov-berkovitch FYI

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

No branches or pull requests

4 participants