agent: Clean up pinentry access locking.

* agent/agent.h (struct server_control_s): Rename PINENTRY_ACTIVE.
* agent/call-pinentry.c (entry_owner): Remove.
(agent_reset_query): Use thread private object of PINENTRY_ACTIVE.
(unlock_pinentry): Add CTRL to arguments to access thread private.
Check and decrement PINENTRY_ACTIVE for recursive use.
(start_pinentry): Check and increment PINENTRY_ACTIVE for recursion.
(agent_askpin): Follow the change of unlock_pinentry API.
(agent_get_passphrase, agent_get_confirmation): Likewise.
(agent_show_message, agent_popup_message_start): Likewise.
(agent_popup_message_stop, agent_clear_passphrase): Likewise.

--

We use the member PINENTRY_ACTIVE as a thread private object.
It's only valid for a single thread at a time.

It would be possible to have a thread shared object of
PINENTRY_ACTIVE, keeping ENTRY_OWNER for distinguishing its
owner (which is also a thread shared object).  But, in this case,
access to ENTRY_OWNER is tricky (only comparison to accessing thread
would be OK with no lock), or we need to introduce another lock for
accessing ENTRY_OWNER, which complicates the code too much.

So, simply have a thread private object for recursive pinentry access.

GnuPG-bug-id: 3190
Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>
(cherry picked from commit fb7828676c)
This commit is contained in:
NIIBE Yutaka 2017-10-27 09:54:48 +09:00 committed by Werner Koch
parent 4738256f2e
commit 3924e1442c
No known key found for this signature in database
GPG Key ID: E3FDFF218E45B72B
2 changed files with 60 additions and 67 deletions

View File

@ -255,8 +255,9 @@ struct server_control_s
count. */
unsigned long s2k_count;
/* Recursion level of pinentry. */
int pinentry_level;
/* If pinentry is active for this thread. It can be more than 1,
when pinentry is called recursively. */
int pinentry_active;
};

View File

@ -67,12 +67,6 @@ static struct
} entry_features;
/* The control variable of the connection owning the current pinentry.
This is only valid if ENTRY_CTX is not NULL. Note, that we care
only about the value of the pointer and that it should never be
dereferenced. */
static ctrl_t entry_owner;
/* A mutex used to serialize access to the pinentry. */
static npth_mutex_t entry_lock;
@ -128,7 +122,7 @@ agent_query_dump_state (void)
void
agent_reset_query (ctrl_t ctrl)
{
if (entry_ctx && popup_tid && entry_owner == ctrl)
if (entry_ctx && popup_tid && ctrl->pinentry_active)
{
agent_popup_message_stop (ctrl);
}
@ -140,7 +134,7 @@ agent_reset_query (ctrl_t ctrl)
stalled pinentry does not block other threads. Fixme: We should
have a timeout in Assuan for the disconnect operation. */
static gpg_error_t
unlock_pinentry (gpg_error_t rc)
unlock_pinentry (ctrl_t ctrl, gpg_error_t rc)
{
assuan_context_t ctx = entry_ctx;
int err;
@ -177,9 +171,8 @@ unlock_pinentry (gpg_error_t rc)
}
}
if (--entry_owner->pinentry_level == 0)
if (--ctrl->pinentry_active == 0)
{
entry_owner = NULL;
entry_ctx = NULL;
err = npth_mutex_unlock (&entry_lock);
if (err)
@ -292,10 +285,11 @@ start_pinentry (ctrl_t ctrl)
char *flavor_version;
int err;
if (entry_owner == ctrl)
if (ctrl->pinentry_active)
{
/* Allow recursive use of pinentry. */
ctrl->pinentry_level++;
/* It's trying to use pinentry recursively. In this situation,
the thread holds ENTRY_LOCK already. */
ctrl->pinentry_active++;
return 0;
}
@ -313,8 +307,6 @@ start_pinentry (ctrl_t ctrl)
return rc;
}
entry_owner = ctrl;
if (entry_ctx)
return 0;
@ -336,7 +328,7 @@ start_pinentry (ctrl_t ctrl)
the Wine implementation does not flush stdin,stdout and stderr
- see above. Let's try to ignore the error. */
#ifndef HAVE_W32_SYSTEM
return unlock_pinentry (tmperr);
return unlock_pinentry (ctrl, tmperr);
#endif
}
@ -383,7 +375,7 @@ start_pinentry (ctrl_t ctrl)
return rc;
}
ctrl->pinentry_level = 1;
ctrl->pinentry_active = 1;
entry_ctx = ctx;
/* We don't want to log the pinentry communication to make the logs
@ -404,7 +396,7 @@ start_pinentry (ctrl_t ctrl)
{
log_error ("can't connect to the PIN entry module '%s': %s\n",
full_pgmname, gpg_strerror (rc));
return unlock_pinentry (gpg_error (GPG_ERR_NO_PIN_ENTRY));
return unlock_pinentry (ctrl, gpg_error (GPG_ERR_NO_PIN_ENTRY));
}
if (DBG_IPC)
@ -415,65 +407,65 @@ start_pinentry (ctrl_t ctrl)
{
char *optstr;
if (asprintf (&optstr, "OPTION pinentry-user-data=%s", value) < 0 )
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
rc = assuan_transact (entry_ctx, optstr, NULL, NULL, NULL, NULL, NULL,
NULL);
xfree (optstr);
if (rc && gpg_err_code (rc) != GPG_ERR_UNKNOWN_OPTION)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
rc = assuan_transact (entry_ctx,
opt.no_grab? "OPTION no-grab":"OPTION grab",
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
value = session_env_getenv (ctrl->session_env, "GPG_TTY");
if (value)
{
char *optstr;
if (asprintf (&optstr, "OPTION ttyname=%s", value) < 0 )
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
rc = assuan_transact (entry_ctx, optstr, NULL, NULL, NULL, NULL, NULL,
NULL);
xfree (optstr);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
value = session_env_getenv (ctrl->session_env, "TERM");
if (value)
{
char *optstr;
if (asprintf (&optstr, "OPTION ttytype=%s", value) < 0 )
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
rc = assuan_transact (entry_ctx, optstr, NULL, NULL, NULL, NULL, NULL,
NULL);
xfree (optstr);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (ctrl->lc_ctype)
{
char *optstr;
if (asprintf (&optstr, "OPTION lc-ctype=%s", ctrl->lc_ctype) < 0 )
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
rc = assuan_transact (entry_ctx, optstr, NULL, NULL, NULL, NULL, NULL,
NULL);
xfree (optstr);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (ctrl->lc_messages)
{
char *optstr;
if (asprintf (&optstr, "OPTION lc-messages=%s", ctrl->lc_messages) < 0 )
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
rc = assuan_transact (entry_ctx, optstr, NULL, NULL, NULL, NULL, NULL,
NULL);
xfree (optstr);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
@ -489,7 +481,7 @@ start_pinentry (ctrl_t ctrl)
rc = assuan_transact (entry_ctx, "OPTION allow-external-password-cache",
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc && gpg_err_code (rc) != GPG_ERR_UNKNOWN_OPTION)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (opt.allow_emacs_pinentry)
@ -499,7 +491,7 @@ start_pinentry (ctrl_t ctrl)
rc = assuan_transact (entry_ctx, "OPTION allow-emacs-prompt",
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc && gpg_err_code (rc) != GPG_ERR_UNKNOWN_OPTION)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
@ -537,7 +529,7 @@ start_pinentry (ctrl_t ctrl)
if (*s == '|' && (s2=strchr (s+1,'|')))
s = s2+1;
if (asprintf (&optstr, "OPTION default-%s=%s", tbl[idx].key, s) < 0 )
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
assuan_transact (entry_ctx, optstr, NULL, NULL, NULL, NULL, NULL,
NULL);
xfree (optstr);
@ -664,8 +656,8 @@ start_pinentry (ctrl_t ctrl)
rc = agent_inq_pinentry_launched (ctrl, pinentry_pid, flavor_version);
if (gpg_err_code (rc) == GPG_ERR_CANCELED
|| gpg_err_code (rc) == GPG_ERR_FULLY_CANCELED)
return unlock_pinentry (gpg_err_make (GPG_ERR_SOURCE_DEFAULT,
gpg_err_code (rc)));
return unlock_pinentry (ctrl, gpg_err_make (GPG_ERR_SOURCE_DEFAULT,
gpg_err_code (rc)));
rc = 0;
}
@ -1035,18 +1027,18 @@ agent_askpin (ctrl_t ctrl,
rc = assuan_transact (entry_ctx, line,
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc && gpg_err_code (rc) != GPG_ERR_ASS_UNKNOWN_CMD)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
build_cmd_setdesc (line, DIM(line), desc_text);
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
snprintf (line, DIM(line), "SETPROMPT %s",
prompt_text? prompt_text : is_pin? L_("PIN:") : L_("Passphrase:"));
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
/* If a passphrase quality indicator has been requested and a
minimum passphrase length has not been disabled, send the command
@ -1055,7 +1047,7 @@ agent_askpin (ctrl_t ctrl,
{
rc = setup_qualitybar (ctrl);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (initial_errtext)
@ -1064,7 +1056,7 @@ agent_askpin (ctrl_t ctrl,
rc = assuan_transact (entry_ctx, line,
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (pininfo->with_repeat)
@ -1095,7 +1087,7 @@ agent_askpin (ctrl_t ctrl,
rc = assuan_transact (entry_ctx, line,
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
errtext = NULL;
}
@ -1105,7 +1097,7 @@ agent_askpin (ctrl_t ctrl,
rc = assuan_transact (entry_ctx, line,
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
saveflag = assuan_get_flag (entry_ctx, ASSUAN_CONFIDENTIAL);
@ -1133,7 +1125,7 @@ agent_askpin (ctrl_t ctrl,
errtext = is_pin? L_("PIN too long")
: L_("Passphrase too long");
else if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
if (!errtext && pininfo->min_digits)
{
@ -1159,7 +1151,7 @@ agent_askpin (ctrl_t ctrl,
|| gpg_err_code (rc) == GPG_ERR_BAD_PIN)
errtext = (is_pin? L_("Bad PIN") : L_("Bad Passphrase"));
else if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (!errtext)
@ -1167,7 +1159,7 @@ agent_askpin (ctrl_t ctrl,
if (pininfo->with_repeat
&& (pinentry_status & PINENTRY_STATUS_PIN_REPEATED))
pininfo->repeat_okay = 1;
return unlock_pinentry (0); /* okay, got a PIN or passphrase */
return unlock_pinentry (ctrl, 0); /* okay, got a PIN or passphrase */
}
if ((pinentry_status & PINENTRY_STATUS_PASSWORD_FROM_CACHE))
@ -1176,7 +1168,7 @@ agent_askpin (ctrl_t ctrl,
pininfo->failed_tries --;
}
return unlock_pinentry (gpg_error (pininfo->min_digits? GPG_ERR_BAD_PIN
return unlock_pinentry (ctrl, gpg_error (pininfo->min_digits? GPG_ERR_BAD_PIN
: GPG_ERR_BAD_PASSPHRASE));
}
@ -1242,7 +1234,7 @@ agent_get_passphrase (ctrl_t ctrl,
rc = assuan_transact (entry_ctx, line,
NULL, NULL, NULL, NULL, NULL, NULL);
if (rc && gpg_err_code (rc) != GPG_ERR_ASS_UNKNOWN_CMD)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
if (desc)
@ -1251,18 +1243,18 @@ agent_get_passphrase (ctrl_t ctrl,
snprintf (line, DIM(line), "RESET");
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
snprintf (line, DIM(line), "SETPROMPT %s", prompt);
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
if (with_qualitybar && opt.min_passphrase_len)
{
rc = setup_qualitybar (ctrl);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (errtext)
@ -1270,14 +1262,14 @@ agent_get_passphrase (ctrl_t ctrl,
snprintf (line, DIM(line), "SETERROR %s", errtext);
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
memset (&parm, 0, sizeof parm);
parm.size = ASSUAN_LINELENGTH/2 - 5;
parm.buffer = gcry_malloc_secure (parm.size+10);
if (!parm.buffer)
return unlock_pinentry (out_of_core ());
return unlock_pinentry (ctrl, out_of_core ());
saveflag = assuan_get_flag (entry_ctx, ASSUAN_CONFIDENTIAL);
assuan_begin_confidential (entry_ctx);
@ -1301,7 +1293,7 @@ agent_get_passphrase (ctrl_t ctrl,
xfree (parm.buffer);
else
*retpass = parm.buffer;
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
@ -1345,7 +1337,7 @@ agent_get_confirmation (ctrl_t ctrl,
rc = gpg_err_make (gpg_err_source (rc), GPG_ERR_CANCELED);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
if (ok)
{
@ -1353,7 +1345,7 @@ agent_get_confirmation (ctrl_t ctrl,
rc = assuan_transact (entry_ctx,
line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
if (notok)
{
@ -1376,7 +1368,7 @@ agent_get_confirmation (ctrl_t ctrl,
NULL, NULL, NULL, NULL, NULL, NULL);
}
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
rc = assuan_transact (entry_ctx, "CONFIRM",
@ -1384,7 +1376,7 @@ agent_get_confirmation (ctrl_t ctrl,
if (rc && gpg_err_source (rc) && gpg_err_code (rc) == GPG_ERR_ASS_CANCELED)
rc = gpg_err_make (gpg_err_source (rc), GPG_ERR_CANCELED);
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
@ -1418,7 +1410,7 @@ agent_show_message (ctrl_t ctrl, const char *desc, const char *ok_btn)
rc = gpg_err_make (gpg_err_source (rc), GPG_ERR_CANCELED);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
if (ok_btn)
{
@ -1426,7 +1418,7 @@ agent_show_message (ctrl_t ctrl, const char *desc, const char *ok_btn)
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL,
NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
rc = assuan_transact (entry_ctx, "CONFIRM --one-button", NULL, NULL, NULL,
@ -1434,7 +1426,7 @@ agent_show_message (ctrl_t ctrl, const char *desc, const char *ok_btn)
if (rc && gpg_err_source (rc) && gpg_err_code (rc) == GPG_ERR_ASS_CANCELED)
rc = gpg_err_make (gpg_err_source (rc), GPG_ERR_CANCELED);
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
@ -1482,19 +1474,19 @@ agent_popup_message_start (ctrl_t ctrl, const char *desc, const char *ok_btn)
snprintf (line, DIM(line), "RESET");
rc = assuan_transact (entry_ctx, line, NULL, NULL, NULL, NULL, NULL, NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
if (ok_btn)
{
snprintf (line, DIM(line), "SETOK %s", ok_btn);
rc = assuan_transact (entry_ctx, line, NULL,NULL,NULL,NULL,NULL,NULL);
if (rc)
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
err = npth_attr_init (&tattr);
if (err)
return unlock_pinentry (gpg_error_from_errno (err));
return unlock_pinentry (ctrl, gpg_error_from_errno (err));
npth_attr_setdetachstate (&tattr, NPTH_CREATE_JOINABLE);
popup_finished = 0;
@ -1505,7 +1497,7 @@ agent_popup_message_start (ctrl_t ctrl, const char *desc, const char *ok_btn)
rc = gpg_error_from_errno (err);
log_error ("error spawning popup message handler: %s\n",
strerror (err) );
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}
npth_setname_np (popup_tid, "popup-message");
@ -1566,7 +1558,7 @@ agent_popup_message_stop (ctrl_t ctrl)
memset (&popup_tid, '\0', sizeof (popup_tid));
/* Now we can close the connection. */
unlock_pinentry (0);
unlock_pinentry (ctrl, 0);
}
int
@ -1592,5 +1584,5 @@ agent_clear_passphrase (ctrl_t ctrl,
rc = assuan_transact (entry_ctx, line,
NULL, NULL, NULL, NULL, NULL, NULL);
return unlock_pinentry (rc);
return unlock_pinentry (ctrl, rc);
}