erlef / rebar3_hex

Rebar3 Hex library
Apache License 2.0
101 stars 49 forks source link

Get correct password on win32. #92

Closed jkrukoff closed 5 years ago

jkrukoff commented 5 years ago

This changes the password routines on windows to ignore the newline on password entry.

In addition, this adds additional overwriting on the clear routines to both hide the ANSI escape code when it is ignored and to overprint pasted passwords less than 16 characters long with spaces.

There's still some edge cases with narrow terminals, but this allows for authentication and publishing to work on windows again.

This is an alternative to https://github.com/tsloughter/rebar3_hex/pull/90 that tries to solve a few more problems. It works for me on Windows 10, both with and without ANSI escape codes enabled in the terminal.

starbelly commented 5 years ago

@jkrukoff I tested this and it works... the only key difference is the time it takes to clear input. I'm able to type in a password fast enough where I can see some of the chars. Of course, neither way is secure by any means, but the original did clear it fast enough where characters could not be seen and apparently did the same for pasting as well (I.e., I could not see what I pasted with the original function).

jkrukoff commented 5 years ago

That's interesting, since delay is the same, there must be really interesting things going on in the windows terminal code.

On original version, pasted characters were not cleared unless ANSI escape codes were enabled (which is not the windows terminal default, and takes a system call or registry change to enable). It actually only cleared a couple characters in that situation by accident, as it overwote them with the uninterpreted escape code.

If we're seeing a speed difference between the two versions, maybe could try just overwriting with spaces, or with fewer spaces. I'm not sure it matters, as I could see the flicker of the typed characters with both versions. It's probably dependent on how fast update actually happens, which appears to be somewhere around ~50ms on my machine when I was testing the narrow terminal case. The actual overwrite delay doesn't seem to particularly matter.

jkrukoff commented 5 years ago

Makes me wonder when the flush happens on stderr, and if multiple writes would change things.

starbelly commented 5 years ago

Good point. I didn't try messing with my settings on this, so the original version may be just as bad depending settings.

jkrukoff commented 5 years ago

Or depending on machine! Terminal write latency can be effected by a bunch of things, like GPU, virtualization, monitor refresh, activesync/gsync. There's just no way of getting around that this is a mostly working hack.

jkrukoff commented 5 years ago

Looks like IO writes are flushed immediately AFAICT, and I don't seem to be able to do any better. Looks like this is my best attempt.

Updated blanking length followed Ferd's note that 24 characters was the default in some password managers.

jkrukoff commented 5 years ago

As an attempt to justify the refactoring that was done here and simplify review:

The structural changes here are mostly to emulate the normal password entry as closely as possible and try to get consistent behavior between the two. This includes using the same newline removal code to fix the original bug. It also makes a change to leave the io options in the same state as the default path.

Additionally, this attempts to ensure the blanking process is always terminated so that error output isn't overwritten. As a result it also adds a timeout to the receive that waits for that termination in order to prevent an infinite hang in the unlikely event the blanking process has already exited.

starbelly commented 5 years ago

I closed https://github.com/tsloughter/rebar3_hex/pull/90 in favor of this PR. They both solve the same issue and are both somewhat insecure 😜 . This PR at least makes the interaction a little more friendly. @jkrukoff It looks like this needs to be rebased.

tsloughter commented 5 years ago

@jkrukoff can you rebase this on master

jkrukoff commented 5 years ago

Rebased.