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

Pkg.clone shows password in cleartext on screen when cloning via https #17422

Closed
davidanthoff opened this issue Jul 14, 2016 · 36 comments
Closed
Labels
bug Indicates an unexpected problem or unintended behavior packages Package management and loading system:windows Affects only Windows

Comments

@davidanthoff
Copy link
Contributor

davidanthoff commented Jul 14, 2016

When I Pkg.clone a private repo on github using the https URL, I get a prompt that asks for the username, and then a prompt that asks for the password. When I type my password, I can see it in cleartext on the screen. Instead, * should be shown on the screen for each character I type.

I think this should get the labels bug and assigned to the 0.5.0 milestone because this is quite a no-no from a security point of view.

@tkelman (Tony, feel free to stop me from CCing you on the things that I think should be assigned to 0.5.0, but I'm treating you as the release manager until I hear push back from you :)

julia> versioninfo()
Julia Version 0.5.0-dev+5435
Commit 894fbe7* (2016-07-14 16:00 UTC)
Platform Info:
  System: NT (x86_64-w64-mingw32)
  CPU: Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.7.1 (ORCJIT, haswell)
@tkelman tkelman added packages Package management and loading bug Indicates an unexpected problem or unintended behavior labels Jul 14, 2016
@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

It was either @wildart or @stevengj who wrote the authentication code I believe.

Re: pinging me personally, I'd rather you didn't, x-ref #15068 (comment)

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

I can reproduce this (on Windows, not on Linux). @wildart what's the best way to find out where in the call chain libgit2 gets to for cloning a private https package? wrong issue for that question

@tkelman tkelman added the system:windows Affects only Windows label Jul 14, 2016
@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

I'm very confused. When I call LibGit2.prompt with password=true by itself, it works as it should. But not within Pkg.clone which should be calling

userpass = prompt("Password for '$schema$username@$host'", password=true)

julia> a = LibGit2.prompt("foo", password=true);
foo:
julia> a
"blah"

@wildart
Copy link
Member

wildart commented Jul 14, 2016

ha, it's a terminal issue. Base.getpass is a culprit.

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2016

Happy to test if you have a proposed patch/PR.

@stevengj
Copy link
Member

For getpass on Windows, see #8228. We ended up using the implementation based on _getch, which is supposed to read a character without echoing it on the terminal.

@wildart
Copy link
Member

wildart commented Jul 18, 2016

I can confirm that Base.getpass on windows doesn't mask input and hangs causing #17423

@tkelman
Copy link
Contributor

tkelman commented Jul 18, 2016

It looks like it works differently depending on whether you're running from mintty (in cygwin or msys2) or cmd.

@wildart
Copy link
Member

wildart commented Jul 18, 2016

I tried 0.4.5 and ccall(:_getch, UInt8, ()) works fine. On master cygwin build in mintty.exe this call hangs.

@tkelman
Copy link
Contributor

tkelman commented Jul 18, 2016

It's not a difference between 0.4 and 0.5, it's a difference in what terminal you're using to run Julia. _getch doesn't know how to deal with the named pipes that cygwin's mintty terminal emulator uses. We might need to use something smarter if we want this to work properly in mintty. But note that this is a separate issue from the libgit2 authentication problem, since that happens even outside of mintty. As I said above, LibGit2.prompt and Base.getpass work fine from cmd, but Pkg.clone of a private package doesn't.

@wildart
Copy link
Member

wildart commented Jul 18, 2016

Question is from where Pkg.clone was executed? If from mintty terminal then it would hang because of faulty _getch.

@tkelman
Copy link
Contributor

tkelman commented Jul 18, 2016

Question is from where Pkg.clone was executed?

cmd in my tests. Probably powershell for @davidanthoff, but that's usually the same console host.

@wildart
Copy link
Member

wildart commented Jul 18, 2016

I'm running julia from cmd and it works fine, powershell as well. I only have problems with _getch call on mintty.

@davidanthoff What terminal do you use when running julia?

@tkelman
Copy link
Contributor

tkelman commented Jul 18, 2016

You can clone a private package over https with recent 0.5-dev in cmd, without the password getting echoed? I seem to get the password echoed sometimes in cmd if Pkg.clone is the first thing I run, but not if I do anything else in the REPL first. Something might be racy or very sensitive to REPL state.

@wildart
Copy link
Member

wildart commented Jul 18, 2016

Yes, no echo for me. The only problem that enter key is swallowed and following output starts right after password prompt.

@davidanthoff
Copy link
Contributor Author

I'm not using mintty, I'm just starting julia from the startmenu shortcut to reproduce this, so there is also no powershell involved.

Things got even weirder just now. I essentially got this output (I added line numbers in [] that are not present in the original output, and replaced my password with mypassword):

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.5.0-dev+5478 (2016-07-18 02:34 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 43f3c05 (0 days old master)
|__/                   |  x86_64-w64-mingw32

julia> Pkg.clone("https://github.com/davidanthoff/Judyp.jl.git")
[1] INFO: Cloning Judyp from https://github.com/davidanthoff/Judyp.jl.git
[2] Username for 'https://github.com':davidanthoff
[3] Password for 'https://davidanthoff@github.com':mypassword
[4] Username for 'https://github.com':Password for 'https://mypassword@github.com':

I typed in everything. In line 2, I entered my username and then hit Enter. In line 3 I entered my password and hit Enter. Then nothing happened. Then I hit Enter again, and line 4 appeared. And that is now echoing back the password that I entered as the username in front of the @github thing.

@davidanthoff
Copy link
Contributor Author

Oh, and I get the same behavior that @tkelman mentioned, i.e. if I execute any simple julia command at the REPL first, things work:

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.5.0-dev+5478 (2016-07-18 02:34 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 43f3c05 (0 days old master)
|__/                   |  x86_64-w64-mingw32

julia> 5*5
25

julia> Pkg.clone("https://github.com/davidanthoff/Judyp.jl.git")
INFO: Cloning Judyp from https://github.com/davidanthoff/Judyp.jl.git
Username for 'https://github.com':davidanthoff
Password for 'https://davidanthoff@github.com':INFO: Computing changes...
INFO: No packages to install, update or remove

julia>

There should still be a line-break before the INFO: Computing changes..., but no password is echoed.

@stevengj
Copy link
Member

stevengj commented Jul 19, 2016

Note that Python's getpass function similarly calls msvcrt.getwch on Windows (see win_getpass), which calls _getwch() (see msvcrt_getwch). Does Python's getpass function work in mintty?

@stevengj
Copy link
Member

cc @Keno on the terminal weirdness here.

@tkelman
Copy link
Contributor

tkelman commented Jul 19, 2016

No the native version of Python is pretty much unusable in mintty, which we've generally been doing a much better job of. How hard can it be to not echo input back to the screen, do we need to do a ccall for that?

@stevengj
Copy link
Member

stevengj commented Jul 19, 2016

Most of the sources I can find suggest that you use tcgetattr/tcsetattr suppress terminal output (see e.g. here). No idea how that interacts with mintty. (My fear is that getch_ does this already, so we won't gain anything.)

@stevengj
Copy link
Member

We can look at how pdcurses and cygwin's ncurses work.

@Keno
Copy link
Member

Keno commented Jul 19, 2016

Maybe just pop up a text box? Windows does have that available as an API function if I remember correctly.

@stevengj
Copy link
Member

(See also mintty/mintty#56)

@tkelman
Copy link
Contributor

tkelman commented Jul 19, 2016

that might be better if we're prompting for interaction anyway

@Keno
Copy link
Member

Keno commented Jul 19, 2016

Unfortunately I can't find the API function with text input. There's one with buttons, but unless we want the user to encode their password in ternary first that probably doesn't work.

@vtjnash
Copy link
Member

vtjnash commented Jul 19, 2016

How hard can it be to not echo input back to the screen, do we need to do a ccall for that?

extremely awful. We fork stty.exe twice every time you hit a character at the repl to provide this. (the alternative is to build against cygwin1.dll and hope the user gets lucky with libc dll versioning + fork + rebase triple hell all matching against the buildbot)

If GPL2 is OK for this, there's pinentry, https://www.gnupg.org/download/, implementing the Assuan protocol https://www.gnupg.org/documentation/manuals/assuan/index.html

@Keno
Copy link
Member

Keno commented Jul 19, 2016

@tkelman
Copy link
Contributor

tkelman commented Jul 19, 2016

I don't think GPL is okay for this unless we make a separate wrapper program that we shell out to. winapi looks more promising.

@Keno
Copy link
Member

Keno commented Jul 19, 2016

A little antiquated perhaps, but it's ok:

screen shot 2016-07-19 at 7 51 29 pm

@tkelman
Copy link
Contributor

tkelman commented Jul 20, 2016

That's probably why it says to use the newer API when you're on Vista or newer?

@Keno
Copy link
Member

Keno commented Jul 20, 2016

Yes, the new API is super complicated, but I managed to figure it out as well just now (though this is the server version, so the UI is different from what it would be on the desktop version):
screen shot 2016-07-19 at 8 23 51 pm

@stevengj
Copy link
Member

Some software (e.g. winpty) actually just creates a hidden console window for user input.

@davidanthoff
Copy link
Contributor Author

I think any of these dialog boxes would be ok for now, i.e. fix this security bug. Long term it would be nicer if the password prompt was console based, but imo that could come after 0.5.0.

@davidanthoff
Copy link
Contributor Author

I'm closing this, seems resolved.

@stevengj
Copy link
Member

Yes, fixed by #17506.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior packages Package management and loading system:windows Affects only Windows
Projects
None yet
Development

No branches or pull requests

6 participants