Good news, OpenVPN fans: Your software's only a little bit buggy

The venerable OpenVPN client has been given a mostly clean bill of health. Between December and February, a team led by Johns Hopkins University crypto-boffin Dr Matthew Green has been auditing OpenVPN 2.4's code. The review, paid for by Private Internet Access (which uses the software), has been published. While all …

[david 12]

Only analysed Linux platform?

>

Windows-Specific Options:

--win-sys path

By default, if this directive is not specified, OpenVPN will use the SystemRoot environment variable.

[....]

The Windows ipconfig /all command can be used to show what Windows thinks the DHCP server address is.

<

It runs an external executable based on what an /environment variable/ pretends that the system root is.

This, after they have decided to use execve() instead of system() on *nix platforms.

I pointed out that this was insecure on Windows platforms, and that the secure alternative (the Windows API for findng the system root) was always used, more than 10 years ago. I gave them examples using the Windows API. Their response was that using an enviromental variable to find the system root was a normal method, they didn't think it mattered, and they didn't know anything about Windows API.

Still the case I guess.

[Nick Kew]

Re: Only analysed Linux platform?

If you have Windows platform expertise that the dev team lack, perhaps you should be joining the team yourself?

Or else if you and they don't get along well enough for that, fork a windows-version and endeavour to build your own community in a supportive environment such as github?

[Lee D]

Re: Only analysed Linux platform?

If something can override, say, %WINDIR% or %SYSTEMROOT% or %SYSTEM% or %ProgramFiles% before your program starts then you already have problems far beyond what OpenVPN can inflict - and if you're that paranoid, you use the hard-coded override as specified above in your quoted manpage.

Setting an environment variable on Windows like that is already a privileged operation, so if you can do that, you’ve pretty much owned the system already,

The fact that is that any number of programs DON'T use the system API at all for that at all. There are no warnings against their use on any MS KB page that I can find. They are used in everything from batch scripts to hard-coded into programs (via library calls to look up environment variables).

So, they are probably not-unreasonable in their refusal to use a proprietary, likely platform-specific API, over allowing you to pick up environment variables and hard-override the options and - better - NOT USING THE COMMAND LINE AT ALL, but a locked-down, permissioned, isolated config file anyway.

[foxyshadis]

Those are bugs?

"....here are the bugs the review did turn up:

* There's a buffer library API that handles dynamically allocated memory safely;

* Wrappers like strncpyt() and openvpn_snprintf() protect unsafe C standard libraries by protecting against buffer overflows and unsafe NULL termination; and

* Keys and other sensitive data are securely wiped from memory to prevent information leaks."

A bit more explanation might be needed?

[Anonymous Coward]

Re: Those are bugs?

I think those are probably examples supporting the previous sentence, "Apparently, the project offers a bit of a tutorial for how to develop secure software" - they're certainly not bugs!

[Lee D]

Re: Those are bugs?

Oh, gosh, look... people who know to wrap the things that are known to cause trouble, so they don't cause trouble.

If this is really this surprising and "good example" in this day and age, it explains a lot about why other things are so rubbish.

Don't even get me started on people who don't wrap malloc and free to prevent double-free's, etc.

And, seriously, once done once it can carry over in other projects really quite easily. Literally a page of code that wraps calls, and then forcing people to use your safe alternatives by some kind of redefinition or overloading.

OpenVPN is good quality code, no doubt, but it mostly looks like that because OpenSSL and similar are just pieces of unfathomable shite in comparison.

[Ben Tasker]

Re: Those are bugs?

> and free to prevent double-free's, etc.

If something's worth doing once, it's worth doing twice.

> And, seriously, once done once it can carry over in other projects really quite easily. Literally a page of code that wraps calls, and then forcing people to use your safe alternatives by some kind of redefinition or overloading.

Yup, with the added benefit that if/when we find out the "safe" way the wrapper uses is really unsafe, you've got a single reference point to update, rather than having to find all the now-not-safe methods used in the code.

[Robert Carnegie]

Re: Those are bugs?

I think the article means "NOW there's a buffer library API that handles dynamically allocated memory safely", etc.

So, this is strictly not a list of bugs found, but a list of the fixes put in for the bugs that were found.

[Voland's right hand]

Crypto gets a bouquet: for example, nobody fell into the trap of using weak key generation.

This is a double edged sword. In theory, OpenVPN gets random data by invoking the OpenSSL RNG asynchronous calls. In practice there is a synchronization point inside OpenSSL there which makes the call blocking in the absense of an OpenSSL supported hardware RNG in the system. This in turn results in it stalling and stuttering if the system exhausts entropy.

Entropy exhaustion is actually quite common under virtualization as there is not enough "proper" entropy in the system and no hardware RNG. It does not take a lot too - a few 10s of clients per concentrator in a VM and voila - you start getting random stalls.

Debugging this one is quite an entertaining experience. If you have not seen it before you can be tearing your hair out for hours and not figure it out.

If you hit it, your only choice is to decrease the quality of the entropy you have by using haveged to inject some lower quality entropy into the system as a whole. That is because openvpn itself does not give you an option to use lower quality entropy sources (as the review has discovered).

[Fred Flintstone]

Thanks for that, learned something new :)

[Lee D]

You don't want to use lower entropy sources for - of all things - a VPN that's exhausted entropy.

If you don't have the hardware to provide the security you want, software can't fix that for you except by - literally - pretending otherwise and carrying on regardless. Guess where the security problem is with that?

And it doesn't take much to add entropy if you are running a VPN device on even a virtualised machine. If you don't know how, then you shouldn't be designing or operating virtualised VPN devices.

Refusing to allow low-entropy sources is no different to refusing to allow low-size keys. It's purely a security decision. Anything else means someone will knock out a "VPN router" that has OpenVPN's name on it in the firmware, but is actually so low on entropy as to be bog-useless. Not having the option means you can't do that without literally having to patch it in (and release your patches?). And then the problem is in your patch, not in every OpenVPN device ever made.

[digihans]

As far as I can remember,

This issue was tackled in the 2.4.x branch.

[John Smith 19]

"Both were fixed in versions 2.4.2 and 2.3.15 before the report landed. "

This is key marker. Not when everything is perfect, how a team handles criticism.

So far they sound interesting.

[GBE]

They only reviewed the client side?

> The venerable OpenVPN client has been given a mostly clean bill of health.

Since you need to run both the client and server to set up a tunnel, what does it matter how secure one end is if the other end is broken?

[DerekOSTIF]

Some questions answered.

Hey everyone! I'm Derek from the OSTIF and here's some answers to the questions that were asked above:

On the windows environment path problem: If an attacker is able to edit the Windows environment path, the system is already completely compromised. Therefore it does not enhance security to use the solution suggested.

On the entropy gathering problem: Stalling due to a lack of entropy in virtual systems is better than using poor entropy sources. OpenVPN does try to partially mitigate this by using a hash PRNG instead of OpenSSL when it is handling non-sensitive information that requires entropy. It will not use weaker entropy to solve the issue globally, though.

On which platforms were covered by the audit: Cryptography engineering analyzed the 2.4.x master source only. QuarksLab and OSTIF analyzed the master source, the Windows specific source, the linux specific source, the Windows tap driver, and the Windows GUI. We planned to cover the OpenVPN for Android app as well, but ran out of time due to significant amounts of time reviewing and documenting the code paths for the other branches. While OpenVPNs code is generally good, the documentation is poor and could use a large scale effort to update the documentation for the application and the protocol. This means that code specific to OpenVPN for Android, code specific to Tunnelblick, and OpenVPN Connect for Android, as well as OpenVPN Connect for iOS were not reviewed. (OpenVPN Connect is based on OpenVPN 3.x code which has different licensing and significant amounts of rewritten code).

The audit covered the source for operating in both client and server mode, using both OpenSSL and PolarSSL (mbedtls).

[david 12]

If system() is compromised on *nix, the system is already compromised. Therefore it does not enhance security to use the solution suggested. A clear example of "Security in shallowness" if there ever was one.

But if GetSystemDirectory (the method used internally by Windows) is compromised, the system is unbootable and inoperable.

And for all those people who upvoted the suggestion that I 'join the team' or write my own VPN instead of helping improve this one: no doubt you have the same opinion about Dr Matthew Green, and I look forward to seeing your comments about that.