Skip to content

Commit

Permalink
Polish up passphrase prompts for key decryption.
Browse files Browse the repository at this point in the history
Now Windows Pageant has two clearly distinct dialog boxes for
requesting a key passphrase: one to use synchronously when the user
has just used the 'Add Key' GUI action, and one to use asynchronously
in response to an agent client's attempt to use a key that was loaded
encrypted.

Also fixed the wording in the asynchronous box: there were two copies
of the 'enter passphrase' instruction, one from the dialog definition
in pageant.rc file and one from the cross-platform pageant.c. Now
pageant.c doesn't format a whole user-facing message any more: it
leaves that to the platform front end to do it the way it wants.

I've also added a call to SetForegroundWindow, to try to get the
passphrase prompt into the foreground. In my experience this doesn't
actually get it the keyboard focus, which I think is deliberate on
Windows's part and there's nothing I can do about it. But at least the
user should _see_ that the prompt is there, so they can focus it
themself.
  • Loading branch information
sgtatham committed Apr 2, 2021
1 parent ceb645b commit efc31ee
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 23 deletions.
11 changes: 4 additions & 7 deletions pageant.c
Original file line number Diff line number Diff line change
Expand Up @@ -392,11 +392,8 @@ static bool request_passphrase(PageantClient *pc, PageantKey *pk)
if (!pk->decryption_prompt_active) {
assert(!gui_request_in_progress);

strbuf *sb = strbuf_new();
strbuf_catf(sb, "Enter passphrase to decrypt key '%s'", pk->comment);
bool created_dlg = pageant_client_ask_passphrase(
pc, &pk->dlgid, sb->s);
strbuf_free(sb);
pc, &pk->dlgid, pk->comment);

if (!created_dlg)
return false;
Expand Down Expand Up @@ -1525,11 +1522,11 @@ static void pageant_conn_got_response(
}

static bool pageant_conn_ask_passphrase(
PageantClient *pc, PageantClientDialogId *dlgid, const char *msg)
PageantClient *pc, PageantClientDialogId *dlgid, const char *comment)
{
struct pageant_conn_state *pcs =
container_of(pc, struct pageant_conn_state, pc);
return pageant_listener_client_ask_passphrase(pcs->plc, dlgid, msg);
return pageant_listener_client_ask_passphrase(pcs->plc, dlgid, comment);
}

static const PageantClientVtable pageant_connection_clientvt = {
Expand Down Expand Up @@ -1719,7 +1716,7 @@ static void internal_client_got_response(
}

static bool internal_client_ask_passphrase(
PageantClient *pc, PageantClientDialogId *dlgid, const char *msg)
PageantClient *pc, PageantClientDialogId *dlgid, const char *comment)
{
/* No delaying operations are permitted in this mode */
return false;
Expand Down
14 changes: 8 additions & 6 deletions pageant.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ struct PageantClientVtable {
void (*got_response)(PageantClient *pc, PageantClientRequestId *reqid,
ptrlen response);
bool (*ask_passphrase)(PageantClient *pc, PageantClientDialogId *dlgid,
const char *msg);
const char *key_comment);
};

static inline void pageant_client_log_v(
Expand All @@ -58,8 +58,8 @@ static inline void pageant_client_got_response(
PageantClient *pc, PageantClientRequestId *reqid, ptrlen response)
{ pc->vt->got_response(pc, reqid, response); }
static inline bool pageant_client_ask_passphrase(
PageantClient *pc, PageantClientDialogId *dlgid, const char *msg)
{ return pc->vt->ask_passphrase(pc, dlgid, msg); }
PageantClient *pc, PageantClientDialogId *dlgid, const char *comment)
{ return pc->vt->ask_passphrase(pc, dlgid, comment); }

/* PageantClientRequestId is used to match up responses to the agent
* requests they refer to. A client may allocate one of these for each
Expand Down Expand Up @@ -159,7 +159,8 @@ struct PageantListenerClient {
struct PageantListenerClientVtable {
void (*log)(PageantListenerClient *, const char *fmt, va_list ap);
bool (*ask_passphrase)(PageantListenerClient *pc,
PageantClientDialogId *dlgid, const char *msg);
PageantClientDialogId *dlgid,
const char *key_comment);
};

static inline void pageant_listener_client_log_v(
Expand All @@ -179,8 +180,9 @@ static inline PRINTF_LIKE(2, 3) void pageant_listener_client_log(
}
}
static inline bool pageant_listener_client_ask_passphrase(
PageantListenerClient *plc, PageantClientDialogId *dlgid, const char *msg)
{ return plc->vt->ask_passphrase(plc, dlgid, msg); }
PageantListenerClient *plc, PageantClientDialogId *dlgid,
const char *comment)
{ return plc->vt->ask_passphrase(plc, dlgid, comment); }

struct pageant_listen_state;
struct pageant_listen_state *pageant_listener_new(
Expand Down
12 changes: 11 additions & 1 deletion unix/uxpgnt.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,24 +103,34 @@ static int make_pipe_to_askpass(const char *msg)
}

static bool uxpgnt_ask_passphrase(
PageantListenerClient *plc, PageantClientDialogId *dlgid, const char *msg)
PageantListenerClient *plc, PageantClientDialogId *dlgid,
const char *comment)
{
struct uxpgnt_client *upc = container_of(plc, struct uxpgnt_client, plc);

assert(!upc->dlgid); /* Pageant core should be serialising requests */

char *msg = dupprintf(
"A client of Pageant wants to use the following encrypted key:\n"
"%s\n"
"If you intended this, enter the passphrase to decrypt the key.",
comment);

switch (upc->prompt_type) {
case RTPROMPT_UNAVAILABLE:
sfree(msg);
return false;

case RTPROMPT_GUI:
upc->passphrase_fd = make_pipe_to_askpass(msg);
sfree(msg);
if (upc->passphrase_fd < 0)
return false; /* something went wrong */
break;

case RTPROMPT_DEBUG:
fprintf(upc->logfp, "pageant passphrase request: %s\n", msg);
sfree(msg);
break;
}

Expand Down
17 changes: 15 additions & 2 deletions windows/pageant.rc
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,29 @@

210 DIALOG DISCARDABLE 0, 0, 140, 60
STYLE DS_MODALFRAME | WS_POPUP | WS_CAPTION | WS_SYSMENU
CAPTION "Pageant: Enter Passphrase"
CAPTION "Pageant: Loading Encrypted Key"
FONT 8, "MS Shell Dlg"
BEGIN
CTEXT "Enter passphrase for key", 100, 10, 6, 120, 8
CTEXT "Enter passphrase to load key", 100, 10, 6, 120, 8
CTEXT "", 101, 10, 16, 120, 8
EDITTEXT 102, 10, 26, 120, 12, ES_PASSWORD | ES_AUTOHSCROLL
DEFPUSHBUTTON "O&K", IDOK, 20, 42, 40, 14
PUSHBUTTON "&Cancel", IDCANCEL, 80, 42, 40, 14
END

212 DIALOG DISCARDABLE 0, 0, 250, 70
STYLE DS_MODALFRAME | WS_POPUP | WS_CAPTION | WS_SYSMENU
CAPTION "Pageant: Decrypting Stored Key"
FONT 8, "MS Shell Dlg"
BEGIN
CTEXT "A client of Pageant wants to use the following encrypted key:", 100, 10, 6, 230, 8
CTEXT "", 101, 10, 16, 230, 8
CTEXT "If you intended this, enter the passphrase to decrypt the key.", 101, 10, 26, 230, 8
EDITTEXT 102, 10, 36, 230, 12, ES_PASSWORD | ES_AUTOHSCROLL
DEFPUSHBUTTON "O&K", IDOK, 75, 52, 40, 14
PUSHBUTTON "&Cancel", IDCANCEL, 135, 52, 40, 14
END

211 DIALOG DISCARDABLE 0, 0, 450, 211
STYLE DS_MODALFRAME | WS_POPUP | WS_CAPTION | WS_SYSMENU
CAPTION "Pageant Key List"
Expand Down
52 changes: 45 additions & 7 deletions windows/winpgnt.c
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ static void wm_copydata_got_response(
}

static bool ask_passphrase_common(PageantClientDialogId *dlgid,
const char *msg)
const char *comment)
{
/* Pageant core should be serialising requests, so we never expect
* a passphrase prompt to exist already at this point */
Expand All @@ -873,18 +873,55 @@ static bool ask_passphrase_common(PageantClientDialogId *dlgid,
pps->modal = false;
pps->dlgid = dlgid;
pps->passphrase = NULL;
pps->comment = msg;
pps->comment = comment;

nonmodal_passphrase_hwnd = CreateDialogParam(
hinst, MAKEINTRESOURCE(210), NULL, PassphraseProc, (LPARAM)pps);
hinst, MAKEINTRESOURCE(212), NULL, PassphraseProc, (LPARAM)pps);

/*
* Try to put this passphrase prompt into the foreground.
*
* This will probably not succeed in giving it the actual keyboard
* focus, because Windows is quite opposed to applications being
* able to suddenly steal the focus on their own initiative.
*
* That makes sense in a lot of situations, as a defensive
* measure. If you were about to type a password or other secret
* data into the window you already had focused, and some
* malicious app stole the focus, it might manage to trick you
* into typing your secrets into _it_ instead.
*
* In this case it's possible to regard the same defensive measure
* as counterproductive, because the effect if we _do_ steal focus
* is that you type something into our passphrase prompt that
* isn't the passphrase, and we fail to decrypt the key, and no
* harm is done. Whereas the effect of the user wrongly _assuming_
* the new passphrase prompt has the focus is much worse: now you
* type your highly secret passphrase into some other window you
* didn't mean to trust with that information - such as the
* agent-forwarded PuTTY in which you just ran an ssh command,
* which the _whole point_ was to avoid telling your passphrase to!
*
* On the other hand, I'm sure _every_ application author can come
* up with an argument for why they think _they_ should be allowed
* to steal the focus. Probably most of them include the claim
* that no harm is done if their application receives data
* intended for something else, and of course that's not always
* true!
*
* In any case, I don't know of anything I can do about it, or
* anything I _should_ do about it if I could. If anyone thinks
* they can improve on all this, patches are welcome.
*/
SetForegroundWindow(nonmodal_passphrase_hwnd);

return true;
}

static bool wm_copydata_ask_passphrase(
PageantClient *pc, PageantClientDialogId *dlgid, const char *msg)
PageantClient *pc, PageantClientDialogId *dlgid, const char *comment)
{
return ask_passphrase_common(dlgid, msg);
return ask_passphrase_common(dlgid, comment);
}

static const PageantClientVtable wmcpc_vtable = {
Expand Down Expand Up @@ -1270,9 +1307,10 @@ void cleanup_exit(int code)
}

static bool winpgnt_listener_ask_passphrase(
PageantListenerClient *plc, PageantClientDialogId *dlgid, const char *msg)
PageantListenerClient *plc, PageantClientDialogId *dlgid,
const char *comment)
{
return ask_passphrase_common(dlgid, msg);
return ask_passphrase_common(dlgid, comment);
}

struct winpgnt_client {
Expand Down

0 comments on commit efc31ee

Please sign in to comment.