wiki:QuarkslabAndCryptographyEngineerAudits

Introduction

OpenVPN 2.4.1 was simultaneously reviewed by Quarkslab (funded by OSTIF) and Cryptography Engineering LCC (funded by Private Internet Access). The reports have been published on OSTIF's and PIA's web pages:

OpenVPN Technologies, Inc's official press release is here:

This page lists the findings in their respective reports and shows how the issues were resolved.

Quarkslab findings

Quarkslab 5.1: Pre-authentication Denial of Service

This issue have been assigned CVE-2017-7478.

An authenticated client can do the 'three way handshake' (P_HARD_RESET, P_HARD_RESET, P_CONTROL), where the P_CONTROL packet is the first that is allowed to carry payload. If that payload is too big, the OpenVPN server process will stop running due to an ASSERT() exception. That is also the reason why servers using tls-auth/tls-crypt are protected against this attack - the P_CONTROL packet is only accepted if it contains the session ID we specified, with a valid HMAC (challenge-response).

This affects OpenVPN 2.3.12 and newer. The problem has been fixed by commit "Don't assert out on receiving too-large control packets":

  • release/2.3: feb35ee5ca
  • release/2.4: 66b99a0753
  • master: 5774cf4c25e

OpenVPN versions 2.3.15, 2.4.2 and later include these fixes.

Quarkslab 5.2: Denial of Service due to Exhaustion of Packet-ID counter

This issue have been assigned CVE-2017-7479.

An authenticated client can cause the server's the packet-id counter to roll over, which would lead the server process to hit an ASSERT() and stop running. To make the server hit the ASSERT(), the client must first cause the server to send it 232 packets (at least 196GB).

This problem is fixed by commit "Drop packets instead of asserting out if packet id rolls over":

  • release/2.3: b727643cdf
  • release/2.4: 591a4e574c
  • master: e498cb0ea8

The fix requires commit "cleanup: merge packet_id_alloc_outgoing() into packet_id_write()" to apply cleanly (see commit 653d391922).

OpenVPN versions 2.4.2 and 2.3.15 (and later) include these fixes. The "release/2.2" branch in Git has also been patched, primarily for the benefit of external package maintainers. We do not, however, intend to make a 2.2-release, not even in source-only format.

Quarkslab 5.3: Invalid Retrieval of X.509 Certificate Fields With mbed TLS

We agree with the report that the security impact of this finding is negligible. While no immediate action would have been necessary, this is fixed by commit "mbedtls: correctly check return value in pkcs11_certificate_dn()":

  • release/2.4: 1ebd3ade
  • master: 423bb16e

Quarkslab 5.4: Usernames/Passwords not Erased from Memory

We agree with the analysis in the report. The likelihood of an occurrence of this issue in real life is exceptionally low since an attacker needs elevated privileges on the server to exploit this kind of information leak. The severity of this issue is therefore rated as very low.

This is fixed by commit "Always clear username/password from memory on error":

  • release/2.4: 47d80b95
  • master: 2b60198e

Quarkslab 5.5: NULL Pointer Dereference in the Data Compression Stub

While this is low security issue, we will look into fixing this in a future release, as the fix is trivial.

Quarkslab 5.6: Invalid Size Parameter Passed when Retrieving the Program Application Path

The report states that this is of "informational" severity as it can't be exploited. Regardless we will fix this in a future release.

Quarkslab 5.7: Leak of Service Manager Handles in OpenVPN GUI

This is an "informational" severity issue which we will fix in a future release of OpenVPN GUI.

Cryptography Engineering findings

OVPN-01: Sensitive authentication token not wiped on certain TLS authentication errors

This is fixed by commit "auth-token: Ensure tokens are always wiped on de-auth". Commit IDs per Git branch:

  • release/2.4: a52fd9575e
  • master: daab0a9fa8

OVPN-02: Potentially flawed TLS control channel encryption

OVPN-02-1: Potentially flawed TLS control channel encryption: IV collision

This is a known issue and has been documented in the commit message of "Add control channel encryption (--tls-crypt)".

The wording "would result in a complete loss of privacy" in the report is cryptographic jargon that might mislead non-cryptographers reading the report to conclude that his/her traffic is no longer protected at all if such an IV collision occurs. That is of course not the case; tls-crypt is an additional layer of protection, and even if IVs collide, one of the packets is with high probability an (unpredictable) encrypted TLS record. Colliding IVs would of course result in some loss of privacy, but are unlikely to lead to a complete loss of privacy. As such, we believe "would result in a loss of privacy" is more appropriate.

We want to emphasize that --tls-crypt is protecting the OpenVPN control channel. The control channel is used to establish the TLS connection between server and client, where certificate information is used. The TLS protocol is designed so that the certificate information (which in essence is a public key, with some signed meta data such as Common Name, etc) is passed over connection in clear-text. This is not unique for OpenVPN, even HTTPS does it like this. So when enabling --tls-crypt, OpenVPN adds another layer of encryption only on this control channel. All the tunnelled network traffic which is encrypted between the OpenVPN instances does not depend on --tls-crypt itself; thus the tunnelled network traffic is not directly affected by this issue.

We have clarified the limitations of --tls-crypt in the man page, including recommendations for rekeying. We will also implement support for client-specific tls-auth/tls-crypt keys in a future OpenVPN release. This provides the option to trade some extra work during provisioning for mitigating this issue.

The relevant man page text:

All peers use the same --tls-crypt pre-shared group key to  authenti‐
cate  and encrypt control channel messages.  To ensure that IV colli‐
sions remain unlikely, this key should not be used  to  encrypt  more
than  2^48  client-to-server or 2^48 server-to-client control channel
messages.  A typical initial negotiation is about 10 packets in  each
direction.   Assuming both initial negotiation and renegotiations are
at most 2^16 (65536) packets (to be conservative),  and  (re)negotia‐
tions  happen  each  minute  for  each  user  (24/7), this limits the
tls-crypt key lifetime to 8171 years divided by the number of  users.
So  a  setup with 1000 users should rotate the key at least once each
eight years.  (And a setup with 8000 users each year.)

If IV collisions were to occur, this could result in the security  of
--tls-crypt degrading to the same security as using --tls-auth.  That
is, the control channel still  benefits  from  the  extra  protection
against  active man-in-the-middle-attacks and DoS attacks, but may no
longer offer extra privacy and post-quantum security on top  of  what
TLS itself offers.

The man-page fix is implemented by these commits:

  • release/2.4: 9444506e
  • master: 5806f66e

OVPN-02-2: Potentially flawed TLS control channel encryption: ciphertext is not MAC'ed

We believe that replacing authentication (--tls-auth) with authenticated encryption (--tls-crypt) does not introduce risks. At the exact place where --tls-auth authenticates the plaintext and checks for replays of control channel packets, --tls-crypt performs decryption, authenticates the plaintext and checks for replays. Our analysis is that --tls-crypt provides the same DoS protection as --tls-auth does, with the sole exception that --tls-crypt requires slightly more work to authenticate a packet (specifically, one AES-CTR operation).

We do not see that encrypt-and-authenticate relates in any way to replay protection, which is always performed after authentication.

Finally, we are aware that MAC-then-encrypt is problematic for CBC cipher modes (or other modes that require padding), but this is not applicable to stream ciphers and in particular not to the SIV-construction[0] we use (with HMAC-SHA2 + AES-CTR).

[0] Rogaway & Shrimpton, A Provable-Security Treatment of the Key-Wrap

Problem, 2006 (https://www.iacr.org/archive/eurocrypt2006/40040377/40040377.pdf)

OVPN-03: Insecure configuration options

OVPN-03-1: Insecure configuration options: --prng

The current prng is non-standard, but seems to rely solely on the preimage-resistance of hash functions, which makes SHA1 (still) a sane default. This custom prng is mostly in place as a performant alternative for CBC IVs. It is not used to produce keys.

Given the above, we see no need to change this right away. But we are considering two options for the internal prng:

  1. Keep --prng but reject using MD4 or MD5
  2. Rip out the OpenVPN prng implementation and rather use the crypto libraries' implementation directly. This has the side-effect of reduced performance in CBC-mode.

In regard to NIST compliance, OpenVPN users with such demands will need configure and use OpenVPN in a compliant way (e.g. --prng SHA256).

OVPN-03-2: Insecure configuration options: --no-iv

The --no-iv option is an advanced feature that reduces security for a slight improve in performance. This option has been deprecated in the release/2.4 branch (commit 4969f0d6bb) and removed in the master branch (commit ef910e3e3a). _It will not be present in OpenVPN 2.5._

OVPN-03-3: Insecure configuration options: --no-replay

The --no-replay is an advanced feature that allows disabling OpenVPN's replay attack protection. This option can only be used without reducing security if OpenVPN was already used in an insecure mode that disables authentication. We are considering to deprecate this feature.

OVPN-03-4: Insecure configuration options: --reneg-bytes

The --reneg-bytes option can be used to disable SWEET32 mitigation in OpenVPN. The report suggests ignoring --reneg-bytes when 64-bit block ciphers such as Blowfish are used. In practice doing what the report suggests would break configurations that use OTP but cannot make use of --auth-gen-token. We believe that the current approach, where we use sane but overrideable defaults and log warnings if small block ciphers are used, suffices.

We will review the warning messages and possibly make them more scary in v2.5. If people are still using weak ciphers in the 2.6 release cycle we will consider doing what the report suggests.

OVPN-03-5: Defaulting to --cipher BF-CBC and --auth SHA1

We agree that it would be good to migrate to stronger algorithms such as SHA-2 and AES by default, but doing so would break lots of client setups.

Cipher negotiation (NCP) in OpenVPN 2.4 defaults to AES-256-GCM and allows negotiating stronger ciphers when clients support it. Using NCP is a more user-friendly way to migrating away from unoptimal defaults like BF-CBS and SHA-1.

We will continue to looking into gradually upgrade the connection's crypto parameters without breaking existing configurations. Once that has been deployed and widely used for a longer time we can truly consider changing the default values, or deprecate the old way of setting ciphers.

OVPN-03-6: Insecure configuration options: encryption without authentication

This has been fixed in commit "Make --cipher/--auth none more explicit on the risks":

  • release/2.4: 1935729fe6
  • master: 7a1b6a0dd7

The commit modifies the warning messages to make it absolutely clear that using --auth none or --cipher none is a really bad idea.

OVPN-04: Possible NULL pointer dereferences

OVPN-04-1: Possible NULL pointer derefence in contrib/keychain-mcd/cert_data.c

This code needs a considerable overhaul. It lacks a proper defensive coding style, where the crux of the issues is that there are basically no error checking on functions which may very well return NULL. This issue also goes deeper than what this report covers.

We have initiated a discussion in the community about getting this code fixed. In addition we are discussing with the Tunnelblick maintainer if it would be more appropriate to transfer this code to the Tunnelblick project directly; the keychain-mcd contrib code is only targetting macOS.

OVPN-04-2: Possible NULL pointer dereference in sample-plugins/defer/simple.c

A complete rewrite of the sample-plugins/defer/simple.c example plug-in has been sent for review. This will replace simple.c completely and resolves both this issue and OVPN-07.

OVPN-04-3: Possible NULL pointer dereference in src/openvpn/proxy.c

Our analysis shows that the code path referred to in the report cannot fail. "la" is a zero-initialized struct buffer, that is only ever changed if "lookahead" is non-null:

struct buffer la;
int lastc = 0;

CLEAR(la);
if (lookahead)
{
    la = *lookahead;
}

That means that if (buf_defined(&la)) will always be false if *lookahead is NULL, and thus that it will not be dereferenced:

/* also store char in lookahead buffer */
if (buf_defined(&la))
{
    buf_write_u8(&la, c);
    if (!isprint(c) && !isspace(c)) /* not ascii? */
    {
        if (verbose)
        {
            msg(D_LINK_ERRORS | M_ERRNO, "recv_line: Non-ASCII
character (%d) read on recv()", (int)c);
        }
        *lookahead = la;
        return false;
    }
}

We do think, though, that the code path is not easy to follow and should be improved.

OVPN-05: Possible allocation of zero bytes with malloc

The issue itself is very minor and very unlikely to cause problems. This piece of code can however definitely use some improving - which we'll tackle later.

OVPN-06: Remove workaround to support obsolete versions of OpenSSL (v0.9.6b)

This has been fixed commit "Require minimum OpenSSL 1.0.1" (039a89c331) in Git master branch.

OVPN-07: Remove system() from sample authentication plugin

A pending patch, "plugins: Replace defer/simple with an improved replacement", refactors the sample plugin code and gets rid of the code patch described in there report. This patch also covers OVPN-04-2.

OVPN-08: OpenVPN configuration risks

OVPN-08-1: OpenVPN configuration risks: --script-security 3

The report claims the following:

"The third script security level (--script-security 3) allows passwords to be passed to scripts using environment variables. Unprivileged processes on the same system will be able to access these variables as well. Given that this is unsafe on certain platforms and users are encouraged not to use it, there does not seem to be a good enough reason outside of backwards compatibility to continue supporting this mode. Any feature that requires this level of script security should be rewritten to use safer approaches such as files."

We are not fully convinced there are any viable attack vectors on how this is used in OpenVPN. We acknowledge that environment variables of a process can be accessed with 'ps' or procfs (/proc/$PID/environ) on Linux. FreeBSD has this information only available via ps. However, based on our testing on Linux, FreeBSD and Windows the environment variables of a process (e.g. a script hook) launched by OpenVPN are only available to other processes that run with the same UID/GID as the OpenVPN process itself.

We do not consider this a security issue, but we do agree that using files instead of environment variables would be slightly safer as more advanced filesystem ACLs could be used for access control (such as SELinux). But since this feature has been available for a long time in OpenVPN and people depend on it, removing would not be trivial.

OVPN-08-2: OpenVPN configuration risks: --tls-verify

The --tls-verify option allows running a command to verify the X509 name of a pending TLS connection that has otherwise passed all other tests of certification, except the revocation test via --crl-verify, which occurs after --tls-verify. The --tls-verify option can be used to provide fine grained access control on the server side in ways where the alternative, --verify-x509-name, will not suffice.

We see --tls-verify as primarily server-side option. It could probably be removed from the client-side without any effect to our users. However, it is not altogether clear what the attack vector would be, so we currently don't have any plans on removing it.

OVPN-08-3: OpenVPN configuration risks: --comp-lzo

We agree that we should warn users about the risks of using compression on encrypted protocols such as OpenVPN. In OpenVPN 2.4.3 we will warn users about this. In addition to --comp-lzo we will also warn about --compress.

Last modified 2 months ago Last modified on 05/15/17 07:12:33