Re: [Openvpn-devel] [PATCH] Do not set pkcs11-helper "safe fork mode"

2019-07-20 Thread Steffan Karger 
On 28-04-19 11:23, Steffan Karger wrote:
> Together with the pkcs11-helper fixes, I do think this is the right fix.
> I'll try to experiment a bit with it myself too.

Finally got to some testing and staring at code. The patch resolves the
issue for me, and I didn't find any other issues. Our code seems to be
in good shape to disable safe fork mode as far as I can tell.

I replied to the github thread on
https://github.com/OpenVPN/openvpn/pull/121 with my conclusions:

"The initialization order has been changed around 2015. Since then,
OpenVPN initializes the crypto - including pkcs11 - always after
daemonizing. Any PIN / password is always queried before that point.
Since this patch will only be applied to release/2.4 and master, we're
good in that aspect.

With respect to slot events, I believe we're good too. If I understand
the code correctly (@alonbl, please correct me if I'm wrong), slot
events are only used if someone calls pkcs11h_setSlotEventHook(), which
OpenVPN doesn't do.

Summarizing, unless @alonbl tells me I'm wrong I think this patch is
good and should be merged."

-Steffan


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Do not set pkcs11-helper "safe fork mode"

2019-04-28 Thread Steffan Karger 
Hi David, Hilko,

On 07-03-19 20:14, David Sommerseth wrote:
> On 18/02/2019 16:31, Hilko Bengen wrote:
>> From the pkcs11-helper API documentation about pkcs11h_setForkMode():
>>
>>> This funciton is releavant if PKCS11H_FEATURE_MASK_THREADING is
>>> set. If safe mode is on, the child process can use the loaded
>>> PKCS#11 providers but it cannot use fork(), while it is in one of
>>> the hooks functions, since locked mutexes cannot be released.
>>
>> As far as I can tell, pkcs11-helper functionality is not used in a
>> child process that is created after initialization. Even if OpenVPN is
>> turned into a daemon, the pkcs11-helper library is only initialized
>> after calling possibly_become_daemon(), i.e. in the child process. All
>> other uses of fork() are immediately followed by an exec()
>>
>> This simple change fixes the symptoms described in both
>>  (hang on password
>> prompt when systemd support is enabled) and
>>  (hang on
>> initialization with newer versions of pkcs11-helper).
>>
>> I have successfully tested that this makes the described symptoms go
>> away. For this, I used a YubiKey NEO on Debian/stable, a rebuild of
>> OpenVPN 2.4.6 and two versions of libpkcs11-helper:
>>
>> - libpkcs11-helper 1.21-1 from Debian/stretch
>> - a backport of libpkcs11-helper 1.25-1 from Debian/buster
> 
> Hi,
> 
> Sorry for the time it has take to have a look at this.  I've spent time trying
> to understand how the pkcs11-helper handles things ... and how the whole
> forking stuff happens.  And this approach does look promising.
> 
> But ... it doesn't really work on my RHEL 7.6 system (using
> pkcs11-helper-1.11-3.el7.x86_64).  I've tested this with a yubikey 4 token,
> with a RSA-2048 key.
> 
> The relevant log lines:
> 
> Thu Mar  7 19:37:54 2019 us=108092 VERIFY OK: depth=0, CN=devtest.example.org
> Enter test.user token Password: **
> Thu Mar  7 19:37:56 2019 us=5368 PKCS#11: Cannot perform signature
> 179:'CKR_SESSION_HANDLE_INVALID'
> Thu Mar  7 19:37:56 2019 us=5429 OpenSSL: error:14099006:SSL
> routines:ssl3_send_client_verify:EVP lib
> Thu Mar  7 19:37:56 2019 us=5438 TLS_ERROR: BIO read tls_read_plaintext error
> Thu Mar  7 19:37:56 2019 us=5448 TLS Error: TLS object -> incoming plaintext
> read error
> Thu Mar  7 19:37:56 2019 us=5454 TLS Error: TLS handshake failed
> Thu Mar  7 19:37:56 2019 us=5601 TCP/UDP: Closing socket
> 
> 
> When I compile *without* --enable-systemd, this works.  Which is odd.  I've
> checked that the input provided is identical.  Both builds with and without
> systemd support ends up with the same "pin" value.  But for some reason the
> systemd build fails when calling pkcs11h_openssl_session_getEVP() in
> pkcs11_init_tls_session() [pkcs11_openssl.c:65].
> 
> Based on the error message (CKR_SESSION_HANDLE_INVALID), it seems the locking
> being disabled with using pkcs11h_setForkMode(FALSE) still breaks something
> along the way.
> 
> On the positive side, the "hang" we experience without this patch is gone.
> But I can't claim this being a proper fix as it is currently :-/
> 


That is probably because you need the fork fixes from pkcs11-helper
1.25. Could you try again when linking against 1.25?

Together with the pkcs11-helper fixes, I do think this is the right fix.
I'll try to experiment a bit with it myself too.

-Steffan


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Do not set pkcs11-helper "safe fork mode"

2019-03-07 Thread David Sommerseth 
On 18/02/2019 16:31, Hilko Bengen wrote:
> From the pkcs11-helper API documentation about pkcs11h_setForkMode():
> 
>> This funciton is releavant if PKCS11H_FEATURE_MASK_THREADING is
>> set. If safe mode is on, the child process can use the loaded
>> PKCS#11 providers but it cannot use fork(), while it is in one of
>> the hooks functions, since locked mutexes cannot be released.
> 
> As far as I can tell, pkcs11-helper functionality is not used in a
> child process that is created after initialization. Even if OpenVPN is
> turned into a daemon, the pkcs11-helper library is only initialized
> after calling possibly_become_daemon(), i.e. in the child process. All
> other uses of fork() are immediately followed by an exec()
> 
> This simple change fixes the symptoms described in both
>  (hang on password
> prompt when systemd support is enabled) and
>  (hang on
> initialization with newer versions of pkcs11-helper).
> 
> I have successfully tested that this makes the described symptoms go
> away. For this, I used a YubiKey NEO on Debian/stable, a rebuild of
> OpenVPN 2.4.6 and two versions of libpkcs11-helper:
> 
> - libpkcs11-helper 1.21-1 from Debian/stretch
> - a backport of libpkcs11-helper 1.25-1 from Debian/buster

Hi,

Sorry for the time it has take to have a look at this.  I've spent time trying
to understand how the pkcs11-helper handles things ... and how the whole
forking stuff happens.  And this approach does look promising.

But ... it doesn't really work on my RHEL 7.6 system (using
pkcs11-helper-1.11-3.el7.x86_64).  I've tested this with a yubikey 4 token,
with a RSA-2048 key.

The relevant log lines:

Thu Mar  7 19:37:54 2019 us=108092 VERIFY OK: depth=0, CN=devtest.example.org
Enter test.user token Password: **
Thu Mar  7 19:37:56 2019 us=5368 PKCS#11: Cannot perform signature
179:'CKR_SESSION_HANDLE_INVALID'
Thu Mar  7 19:37:56 2019 us=5429 OpenSSL: error:14099006:SSL
routines:ssl3_send_client_verify:EVP lib
Thu Mar  7 19:37:56 2019 us=5438 TLS_ERROR: BIO read tls_read_plaintext error
Thu Mar  7 19:37:56 2019 us=5448 TLS Error: TLS object -> incoming plaintext
read error
Thu Mar  7 19:37:56 2019 us=5454 TLS Error: TLS handshake failed
Thu Mar  7 19:37:56 2019 us=5601 TCP/UDP: Closing socket


When I compile *without* --enable-systemd, this works.  Which is odd.  I've
checked that the input provided is identical.  Both builds with and without
systemd support ends up with the same "pin" value.  But for some reason the
systemd build fails when calling pkcs11h_openssl_session_getEVP() in
pkcs11_init_tls_session() [pkcs11_openssl.c:65].

Based on the error message (CKR_SESSION_HANDLE_INVALID), it seems the locking
being disabled with using pkcs11h_setForkMode(FALSE) still breaks something
along the way.

On the positive side, the "hang" we experience without this patch is gone.
But I can't claim this being a proper fix as it is currently :-/


-- 
kind regards,

David Sommerseth
OpenVPN Inc



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel