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:
This page lists the findings in their respective reports and shows how the issues were resolved.
-
## Quarkslab findings
+
# Quarkslab findings
-
### Quarkslab 5.1: Pre-authentication Denial of Service
+
## Quarkslab 5.1: Pre-authentication Denial of Service
-
This issue has been assigned CVE-2017-7478.
+
This issue have been assigned CVE-2017-7478.
-
An authenticated client can perform 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. 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).
+
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":
+
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
+
* release/2.3: feb35ee5ca
+
* release/2.4: 66b99a0753
+
* master: 5774cf4c25e
-
OpenVPN versions 2.3.15, 2.4.2, and later include these fixes.
+
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
+
## Quarkslab 5.2: Denial of Service due to Exhaustion of Packet-ID counter
-
This issue has been assigned CVE-2017-7479.
+
This issue have been assigned CVE-2017-7479.
-
An authenticated client can cause the server's 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 2^32 packets (at least 196GB).
+
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 2^32 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
+
* 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.
+
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
+
## 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
+
* release/2.4: 1ebd3ade
+
* master: 423bb16e
-
### Quarkslab 5.4: Usernames/Passwords not Erased from Memory
+
## 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
+
* release/2.4: 47d80b95
+
* master: 2b60198e
-
### Quarkslab 5.5: NULL Pointer Dereference in the Data Compression Stub
+
## Quarkslab 5.5: NULL Pointer Dereference in the Data Compression Stub
-
While this is a low-security issue, we will look into fixing this in a future release, as the fix is trivial.
+
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
+
## 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.
+
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
+
## 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
+
# Cryptography Engineering findings
-
### OVPN-01: Sensitive authentication token not wiped on certain TLS authentication errors
+
## 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
+
* release/2.4: a52fd9575e
+
* master: daab0a9fa8
-
### OVPN-02: Potentially flawed TLS control channel encryption
+
## OVPN-02: Potentially flawed TLS control channel encryption
-
#### OVPN-02-1: Potentially flawed TLS control channel encryption: IV collision
+
### 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.
+
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 metadata 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 tunneled network traffic which is encrypted between the OpenVPN instances does not depend on --tls-crypt itself; thus the tunneled network traffic is not directly affected by this issue.
+
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 authenticate and encrypt control channel messages. To ensure that IV collisions 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)negotiations 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.
+
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
+
* release/2.4: 9444506e
+
* master: 5806f66e
-
#### OVPN-02-2: Potentially flawed TLS control channel encryption: ciphertext is not MAC'ed
+
### 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 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
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.''
+
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.
+
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 to configure and use OpenVPN in a compliant way (e.g. --prng SHA256).
+
In regard to NIST compliance, OpenVPN users with such demands will need configure and use OpenVPN in a compliant way (e.g. --prng SHA256).
The --no-iv option is an advanced feature that reduces security for a slight improvement 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._
+
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._
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 deprecating this feature.
+
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.
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 overridable defaults and log warnings if small block ciphers are used, suffices.
+
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.
+
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
+
### 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 suboptimal defaults like BF-CBS and SHA-1.
+
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 looking into gradually upgrading 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 deprecating the old way of setting ciphers.
+
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
+
### 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
+
* 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
+
## OVPN-04: Possible NULL pointer dereferences
-
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.
+
### 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 targeting macOS.
+
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
+
### 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.
+
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
+
### 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:
@@ 182,7 197,7 @@
CLEAR(la);
if (lookahead)
{
-
la = *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:
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
+
## 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.
+
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)
+
## OVPN-06: Remove workaround to support obsolete versions of OpenSSL (v0.9.6b)
-
This has been fixed by commit "Require minimum OpenSSL 1.0.1" (039a89c331) in the Git master branch.
+
This has been fixed commit "Require minimum OpenSSL 1.0.1" (039a89c331) in Git master branch.
-
### OVPN-07: Remove system() from sample authentication plugin
+
## 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.
+
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.
"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 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.
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.
+
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 a server-side option. It could probably be removed from the client-side without any effect on 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.
+
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.
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.
\
No newline at end of file
+
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.
