Skip to content

Commit

Permalink
Remove static ui_vpninfo hack for ENGINE callbacks
Browse files Browse the repository at this point in the history
This doesn't seem to be needed; all the TPM engines (even v1) handle
the callback properly now.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
  • Loading branch information
dwmw2 committed Oct 12, 2018
1 parent cc82898 commit 810ea00
Showing 1 changed file with 8 additions and 41 deletions.
49 changes: 8 additions & 41 deletions openssl.c
Expand Up @@ -335,12 +335,9 @@ struct ui_form_opt {
};

#ifdef HAVE_ENGINE
/* Ick. But there is no way to pass this sanely through OpenSSL */
static struct openconnect_info *ui_vpninfo;

static int ui_open(UI *ui)
{
struct openconnect_info *vpninfo = ui_vpninfo; /* Ick */
struct openconnect_info *vpninfo = UI_get0_user_data(ui);
struct ui_data *ui_data;

if (!vpninfo || !vpninfo->process_auth_form)
Expand All @@ -354,6 +351,7 @@ static int ui_open(UI *ui)
ui_data->last_opt = &ui_data->form.opts;
ui_data->vpninfo = vpninfo;
ui_data->form.auth_id = (char *)"openssl_ui";

UI_add_user_data(ui, ui_data);

return 1;
Expand Down Expand Up @@ -433,42 +431,10 @@ static int ui_close(UI *ui)
return 1;
}

static UI_METHOD *create_openssl_ui(struct openconnect_info *vpninfo)
static UI_METHOD *create_openssl_ui(void)
{
UI_METHOD *ui_method = UI_create_method((char *)"AnyConnect VPN UI");

/* There is a race condition here because of the use of the
static ui_vpninfo pointer. This sucks, but it's OpenSSL's
fault and in practice it's *never* going to hurt us.
This UI is only used for loading certificates from a TPM; for
PKCS#12 and PEM files we hook the passphrase request differently.
The ui_vpninfo variable is set here, and is used from ui_open()
when the TPM ENGINE decides it needs to ask the user for a PIN.
The race condition exists because theoretically, there
could be more than one thread using libopenconnect and
trying to authenticate to a VPN server, within the *same*
process. And if *both* are using certificates from the TPM,
and *both* manage to be within that short window of time
between setting ui_vpninfo and invoking ui_open() to fetch
the PIN, then one connection's ->process_auth_form() could
get a PIN request for the *other* connection.
However, the only thing that ever does run libopenconnect more
than once from the same process is KDE's NetworkManager support,
and NetworkManager doesn't *support* having more than one VPN
connected anyway, so first that would have to be fixed and then
you'd have to connect to two VPNs simultaneously by clicking
'connect' on both at *exactly* the same time and then getting
*really* unlucky.
Oh, and the KDE support won't be using OpenSSL anyway because of
licensing conflicts... so although this sucks, I'm not going to
lose sleep over it.
*/
ui_vpninfo = vpninfo;

/* Set up a UI method of our own for password/passphrase requests */
UI_method_set_opener(ui_method, ui_open);
UI_method_set_writer(ui_method, ui_write);
Expand Down Expand Up @@ -648,11 +614,12 @@ static int load_tpm_certificate(struct openconnect_info *vpninfo,
}
vpninfo->cert_password = NULL;
free(vpninfo->cert_password);
} else {
/* Provide our own UI method to handle the PIN callback. */
meth = create_openssl_ui(vpninfo);
}
key = ENGINE_load_private_key(e, vpninfo->sslkey, meth, NULL);

/* Provide our own UI method to handle the PIN callback. */
meth = create_openssl_ui();

key = ENGINE_load_private_key(e, vpninfo->sslkey, meth, vpninfo);
if (meth)
UI_destroy_method(meth);
if (!key) {
Expand Down

0 comments on commit 810ea00

Please sign in to comment.