[Feature Request] Use embedded wintun.dll

[mokhtarabadi]

In my project, I added code to build.rs to include the wintun.dll library as bytes into the project as a constant. When the user wants to create a tunnel, the bytes are written to a temporary file, and WINTUN_LIBRARY_PATH is set. This change is detailed in this PR. After the user closes the tunnel, the temporary file is deleted. Is it okay to make a PR with my changes, and is it acceptable to include the wintun.dll binary in the project source?

[M0dEx]

I'm sorry, but isn't this a pretty significant security risk? If the temporary file is written to a location many users have an access to (such as %appdata%/Local/Temp, couldn't an attacker time an injection of their own bytes into the DLL, thus compromising the software using it (in my case a VPN, whose security is paramount)?

[ssrlive]

Um. According to you, adding a conditional switch is a better approach? @M0dEx

[M0dEx]

I do not think any of this is a good idea. If an attacker were to set the ENV variable to their DLL, you get arbitrary code execution.

If we wanted to statically compile wintun.dll, we should do that correctly by using bindings and compiling it from source during the build process, hopefully in a transparent and verifiable manner.

[mokhtarabadi]

Currently, the programs using tun2 like tun2proxy either place the wintun.dll file next to the binary itself or ask the user to do so. If a hacker has access to %appdata%/Local/Temp, they would likely also have access to the location where the wintun.dll file is manually copied.

Note: However, it's important to note that securing %appdata%/Local/Temp is generally more challenging because it is a shared directory, and other applications might also write to it. Therefore, using a less exposed directory and implementing additional security measures, such as verifying the integrity of the DLL before loading it, could enhance security. What is your idea?

[mokhtarabadi]

So. is this https://github.com/SagerNet/sing-tun/ library has security issues? This is used in many software that have many users like clash and signbox

[M0dEx]

Currently, the programs using tun2 like tun2proxy either place the wintun.dll file next to the binary itself or ask the user to do so. If a hacker has access to %appdata%/Local/Temp, they would likely also have access to the location where the wintun.dll file is manually copied.

Note: However, it's important to note that securing %appdata%/Local/Temp is generally more challenging because it is a shared directory, and other applications might also write to it. Therefore, using a less exposed directory and implementing additional security measures, such as verifying the integrity of the DLL before loading it, could enhance security. What is your idea?

The thing is, it is much easier to secure the directory where the binaries are, or the other directories Windows uses during dynamically linked library loading, than a temp directory or an arbitrarily chosen directory by the user OR the attacker.

As I've said, I do not think anything else than default DLL loading OR statically compiling wintun from source are a good idea security wise.

[M0dEx]

So. is this https://github.com/SagerNet/sing-tun/ library has security issues? This is used in many software that have many users like clash and signbox

Just because the same approach was chosen by another project does not mean it is secure or that it should be trusted.

[ssrlive]

It is not possible to statically compile wintun from source, it contains a sys kernel driver, which must have a Microsoft certified signature before it can be loaded. We don't have any of these conditions.

[mokhtarabadi]

So. is this https://github.com/SagerNet/sing-tun/ library has security issues? This is used in many software that have many users like clash and signbox

Just because the same approach was chosen by another project does not mean it is secure or that it should be trusted.

No, no, you misunderstood. I'm just amazed at how such a security bug has easily made its way into the systems of thousands or millions of users, and we are only discovering it now!

[M0dEx]

It is not possible to statically compile wintun from source, it contains a sys kernel driver, which must have a Microsoft certified signature before it can be loaded. We don't have any of these conditions.

In that case, if we really want this feature (which I am still hesitant to say we do), this should be hidden behind a non-default, compile time feature flag.

[ssrlive]

As discussed before, it may be a good method to verify the correctness of wintun.

[M0dEx]

As discussed before, it may be a good method to verify the correctness of wintun.

Sure, but it is paramount for this behaviour not to be the default and not to be conditional at runtime. It must be a compile-time feature flag, to be secure by default.

[mokhtarabadi]

Presently, I'll be adjusting the PR to introduce support for the feature flag and have it disabled by default.

As for your question, would it suffice to generate a hash of the wintun.dll during compile time and then, before using the temporary files, check the hash?

[ssrlive]

I think it would be better to submit a PR directly to the wintun rust crate.

[blechschmidt]

So far, I have not been involved in the rust-tun part of the project, but if you are interested in hearing my two cents: If you just check the hash and load the library, you may already a TOCTTOU if the file is loaded for hashing and once again by the WINAPI loader function.

1. Library is opened, read and hash is successfully verified.
2. Library is replaced by an attacker.
3. Malicious library is opened and loaded.

I wouldn't say including this feature does introduce a vulnerability in rust-tun because it can be safely used if you can ensure as the caller that the library is located in a location that cannot be written to by an unprivileged user. (Coming from the Linux world, have a look at sudo. It will happily use the PATH variable coming from an unprivileged context for application execution.)

What's the goal of the PR? Just distributing a single-binary executable?

[mokhtarabadi]

@blechschmidt, Yes, you're right. The hash is only checked the first time. Unfortunately, I have no knowledge about Windows. If there's a way in Windows to lock a file to a process and prevent other processes from accessing it, that would be great, and I'm researching this aspect.

Yes, the goal is simply to have a single-binary executable that doesn't require placing the wintun.dll next to it.

[blechschmidt]

What I don't like about the approach of using an environment variable is that it is invisible to developers using the interface.

I would prefer the following approach:

1. Make the path to the library explicit as part of the Configuration struct.
2. Pass it down from wherever you are using it. For tun2proxy this would mean an extra CLI flag.

Still, ensuring the security of the library will be in the responsibility the caller. Only unpack it to secure locations.

[mokhtarabadi]

I make this feature optional in this PR https://github.com/tun2proxy/rust-tun/pull/72. Currently is ok for me.

[MatejKafka]

This whole feature just makes me uneasy from a security standpoint. You might be able to hack around the TOCTOU issue using mandatory file locks, but I would not trust you (or most other people) to get it right the first time, and significantly increasing the attack surface of a library that works with sensitive data just because you don't like having to ship a separate file seems a bit bizarre.

I'd ship the dll separately, ask users of the library to place it alongside the rust binary in the same directory, and have the caller explicitly pass a path to the .dll, using a library function call, not an environment variable that might be set by an attacker. That way, the caller of the library has control over the directory layout, but an attacker cannot just set an environment variable to override the default path.

[blechschmidt]

I have opened PR #73 to address the issues.

[mokhtarabadi]

Thanks, @blechschmidt. I'm waiting for the PR to be accepted.

[ssrlive]

I have open a PR in the wintun crate. Hopefully this patch will reduce security risks.