5

WireGuard is extremely simple and fast kernel-space VPN based on modern cryptography. I want to use it in production and need automatic IP assignment for new peers. The project provides two short scripts for server and client that do just this. However it states:

Do not use these scripts in production. They are simply a demonstration of how easy the wg(8) tool is at the command line, but by no means should you actually attempt to use these. They are horribly insecure and defeat the purpose of WireGuard. STAY AWAY!

The scripts are:
Server:

#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
#
# Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.

if [[ -z $NCAT_REMOTE_ADDR ]]; then
    ip link del dev wg0 2>/dev/null
    set -e
    ip link add dev wg0 type wireguard
    ip address add 192.168.4.1/24 dev wg0
    wg set wg0 private-key <(wg genkey) listen-port 12912
    ip link set up dev wg0
    exec ncat -e "$(readlink -f "$0")" -k -l -p 42912 -v
fi
read -r public_key
[[ $(wg show wg0 peers | wc -l) -ge 253 ]] && wg set wg0 peer $(wg show wg0 latest-handshakes | sort -k 2 -b -n | head -n 1 | cut -f 1) remove
next_ip=$(all="$(wg show wg0 allowed-ips)"; for ((i=2; i<=254; i++)); do ip="192.168.4.$i"; [[ $all != *$ip/32* ]] && echo $ip && break; done)
wg set wg0 peer "$public_key" allowed-ips $next_ip/32 2>/dev/null && echo "OK:$(wg show wg0 private-key | wg pubkey):$(wg show wg0 listen-port):$next_ip" || echo ERROR

Client:

#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
#
# Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.

set -e
[[ $UID == 0 ]] || { echo "You must be root to run this."; exit 1; }
umask 077
trap 'rm -f /tmp/wg_private_key' EXIT INT TERM
exec 3<>/dev/tcp/demo.wireguard.com/42912
wg genkey | tee /tmp/wg_private_key | wg pubkey >&3
IFS=: read -r status server_pubkey server_port internal_ip <&3
[[ $status == OK ]]
ip link del dev wg0 2>/dev/null || true
ip link add dev wg0 type wireguard
wg set wg0 private-key /tmp/wg_private_key peer "$server_pubkey" allowed-ips 0.0.0.0/0 endpoint "demo.wireguard.com:$server_port" persistent-keepalive 25
ip address add "$internal_ip"/24 dev wg0
ip link set up dev wg0
if [ "$1" == "default-route" ]; then
    host="$(wg show wg0 endpoints | sed -n 's/.*\t\(.*\):.*/\1/p')"
    ip route add $(ip route get $host | sed '/ via [0-9]\{1,3\}\.[0-9]\{1,3\}\.[0-9]\{1,3\}\.[0-9]\{1,3\}/{s/^\(.* via [0-9]\{1,3\}\.[0-9]\{1,3\}\.[0-9]\{1,3\}\.[0-9]\{1,3\}\).*/\1/}' | head -n 1) 2>/dev/null || true
    ip route add 0/1 dev wg0
    ip route add 128/1 dev wg0
fi

Questions:

  1. What is wrong with those scripts? What is the worst case?
  2. Is there a way to fix those issues?
  3. Could somebody write a short comment what each line of those scripts does?

Update: the author of WireGuard has stated that "The problem is that it uses unauthenticated TCP." So what is the worst case and how can it be fixed? Can one provide this TCP socket inside of an SSH tunnel?

user1876484
  • 279
  • 3
  • 6
  • @forest, please see my update... – user1876484 Apr 18 '18 at 10:01
  • @forest, `wg genkey` generates a private key, then it is written into a file and piped as input to `wg pubkey`, which generates corresponding public key, based on that private key. So what is send seems to be the public key. There is no reason to send the private one. It is also seemingly read on the server side: `read -r public_key`. – user1876484 Apr 18 '18 at 10:34
  • Ah thank you. I just skimmed it and assumed `wg` would concatenate the private and public key. – forest Apr 19 '18 at 02:24

2 Answers2

3

The problem boils down to your third point. You do not know what it does without reading it in detail and it contains quite a lot of unclean code.

It probably "just works", but if anything fails there is no error handling implemented. In addition it screen scrapes a few shell utilities without knowing if their output format is fixed. Maybe you upgrade your distribution, get a new version of one of the tools and suddenly the script fails.

What is the worst case

Some of the output parsing fails and the next command gets garbled output and deletes /home. Not as an actual result of analyzing the script, but something which could happen with shell scripts without proper error handling and did in the past (I.e. rm -r $uninitialized/*).

The most likely error to happen is leaving you with broken network and as you have to ask for explanation of the script (which is fine. I would need to study it quite some time before fully understanding the risks of it) this would probably force you to reboot to get the system into a consistent state again.

This is not per se insecure, but it is sloppy and may be insecure. For a script which runs as root and setups important things there should be careful error handling for each thing which may fail.

The general devise is that everything has a risk of being insecure until proven to be secure. You could try to verify the script if it is secure, but it is not worth it as there are several anti-patterns (i.e. parsing output which is not guaranteed (yet) to be stable, processing IPs with regex, no cleanup when a line fails) which make it advisable to rewrite it in a more secure manner.

People may for example rely on the VPN to prevent sending confidential company data unencrypted over the internet. When the VPN script (silently) did not succeed setting up the VPN, the data may get sent insecurely.

Is there a way to fix those issues?

Rewriting it with careful error handling after each command. Probably in a better suited language than bash and reading the data using an API instead of screen scraping other programs.

allo
  • 3,173
  • 11
  • 24
  • -1 **The scripts are using `set -e` which aborts automatically on error.** The argument that there is potentially bad error handling cannot be made. At least skimming the code, I do not see anything in particular that is dangerous, so I imagine the risk has to do with how the networking utilities are being used. Maybe it simply sets it up in a way that is vulnerable to MITM. – forest Apr 18 '18 at 02:24
  • ``set -e`` is NOT a reasonable error *handling*. ``set -e`` *aborts* the script and leaves cleaning up the mess to you. That's probably why they recommend against using these scripts in production. Do not call ``set -e`` (and ``set -u``) error handling, that's at least misleading. It is like an assert in a C program, which crashes the program to prevent unexpected behavior instead of ensuring error handling on failure conditions. – allo Apr 18 '18 at 07:37
  • You're right that `set -e` is not good for error handling and is lazy, but it does mean that the result of an error will be the script exiting rather than going on and wrecking the system because some `cd` failed, even if you have to clean up its mess. Obviously explicit error checking is better. – forest Apr 18 '18 at 07:39
  • You assume every error leads to a program exiting with nonzero status. Some unexpected content of a variable could change the behavior of the next commands without leading to program errors. I would need to review the script in detail to tell if it is dangerous or not, which I do not want to do as I consider a shell script of this (medium) complexity not a proper solution. Just as the authors consider it a usable proof of concept which probably works find for most people, but something they recommend not to use in production. – allo Apr 18 '18 at 07:42
  • 1
    Well it's true that `set -e` has some very odd behavior (in part because it has to avoid crashing the script with things like `if false; then`), but _error handling_ is dealt with. Checking for unexpected content in a variable would be sanity checking (but of course that's just arguing semantics). – forest Apr 18 '18 at 07:45
  • I think our expectations on error handling do not match. I like my scripts to cleanup after themself in case of an error. ``set -e`` is a safety measure to prevent very bad things from happening. Cleanup happens e.g. in the ``trap 'rm -f /tmp/wg_private_key' EXIT INT TERM`` line of the script. But it neither removes the added routes nor removes the ``wg0`` device. – allo Apr 18 '18 at 07:50
  • 2
    Surely the point allo is making isn't over whether `-e` is good/bad error handling/avoiding, nor whether _this particular script suffers_ from bad error handling (or anything else), but that _if you do not know what the script is doing_ you don't know what _potential_ risks it contains. Having said that, if it _does_ send a private key over an unsecured network (as other comments suggest), then that's a _specific_ security risk that it does have. – TripeHound Apr 18 '18 at 10:23
2

The worst case is you are connecting directly to your attacker and read unsanitized input from him.

This script connects your shell directly to an outside host only learned from DNS, probably half around the world, which might not be the party you expect, since a selective MITM would work quite nicely.

Matthias š
  • 21
  • 1