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

Readpassphrase #692 #156

Merged
merged 10 commits into from
Jun 8, 2017
84 changes: 44 additions & 40 deletions contrib/win32/win32compat/misc.c
Original file line number Diff line number Diff line change
Expand Up @@ -1044,58 +1044,62 @@ w32_strerror(int errnum)
return _sys_errlist_ext[errnum - EADDRINUSE];
return strerror(errnum);
}
/*
* Temporary implementation of readpassphrase.
* TODO - this needs to be reimplemented as per
* https://linux.die.net/man/3/readpassphrase
*/
char *
readpassphrase(const char *prompt, char *out, size_t out_len, int flags) {
char *askpass = NULL;
char *ret = NULL;

DWORD mode;
size_t len = 0;
int retr = 0;
char *
readpassphrase(const char *prompt, char *outBuf, size_t outBufLen, int flags) {
Copy link

@bingbing8 bingbing8 Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw RPP_REQUIRE_TTY-- fail if there is no tty at: https://linux.die.net/man/3/readpassphras
is there case where this function is called from virtual tty?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unix has the concept of reading from /dev/tty..
if we fail to open /dev/tty and RPP_REQUIRE_TTY is set to false then unix reads the input from STDIN..

since in windows there is no concept of /dev/tty this flag and the corresponding logic is ignored..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bagajjal the equivalent of /dev/tty in Windows is the console handle (CreateFile("CONIN")).
We need to map open(/dev/tty) to the above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this.

int currentIndex = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name the variable like current_index for consistency with bsd code

char ch;

if (outBufLen == 0) {
errno = EINVAL;
return(NULL);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to pull NULL in (), right?

}

while (_kbhit()) _getch();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this line do? flush out previous console input?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_kbhit() returns a nonzero value, a keystroke is waiting in the buffer..
before we read from user.. we are clearing the keystroke in the waiting buffer.. Ideally this should never happen but the existing code has it and it doesn't harm so I kept it..

Copy link

@manojampalam manojampalam Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we read on a character basis? Why not call an API that reads an entire line (ReadFile on a console handle with console )?
Checkout console APIs on msdn, there are way to control a console handle for echoing, raw mode, etc that could be used for this purpose. We also use these APIs in enter_raw_mode, exit_raw_mode. check them out.


/* prompt user */
wchar_t* wtmp = utf8_to_utf16(prompt);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the general guidance of openssh code suggest declare variables at the beginning.

if (wtmp == NULL)
fatal("unable to alloc memory");

_cputws(wtmp);
free(wtmp);

len = retr = 0;

while (_kbhit())
_getch();

while (len < out_len) {
out[len] = (unsigned char)_getch();

if (out[len] == '\r') {
if (_kbhit()) /* read linefeed if its there */
_getch();
while (currentIndex < outBufLen - 1) {
ch = _getch();

if (ch == '\r') {
if (_kbhit()) _getch(); /* read linefeed if its there */
break;
}
else if (out[len] == '\n') {
} else if (ch == '\n') {
break;
}
else if (out[len] == '\b') { /* backspace */
if (len > 0)
len--; /* overwrite last character */
}
else if (out[len] == '\003') {
/* exit on Ctrl+C */
} else if (ch == '\b') { /* backspace */
if (currentIndex > 0) {
if (flags & RPP_ECHO_ON)
printf("%c \b", ch);

currentIndex--; /* overwrite last character */
}
} else if (ch == '\003') { /* exit on Ctrl+C */
Copy link

@bingbing8 bingbing8 Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if user hit ESC (27)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unix doesn't have special treatment for ESC.. so it will be read as any other input character..

Copy link

@manojampalam manojampalam Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use console APIs, as I described above, there is no need to handle and treat individual key strokes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed, we will read character by character to align with the unix code and to treat the control+c for stopping the process.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familar with key stroke. but some characters like ESC, the logic will fall to this line printf("%c", ch); what will be displayed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't fail.. ESC is printed as [?].. here is the screenshot
image

fatal("");
}
else {
len++; /* keep reading in the loop */
} else {
if (flags & RPP_SEVENBIT)
ch &= 0x7f;

if (isalpha((unsigned char)ch)) {
if(flags & RPP_FORCELOWER)
ch = tolower((unsigned char)ch);
if(flags & RPP_FORCEUPPER)
ch = toupper((unsigned char)ch);
}

outBuf[currentIndex++] = ch;
if(flags & RPP_ECHO_ON)
printf("%c", ch);
}
}

out[len] = '\0'; /* get rid of the cr/lf */
_cputs("\n"); /*show a newline as we do not echo password or the line */
outBuf[currentIndex] = '\0';
_cputs("\n");

return out;
}
return outBuf;
}