security audit fixes

Makefile

@@ -4,15 +4,16 @@ COUNT ?= 5
 TIMEOUT ?= 10
 LOCAL_TARGET := $(shell ip -4 addr show docker0 | grep 'inet ' | awk '{print $$2}' | cut -f 1 -d /)
 COVERAGE_THRESHOLD := 88
+FLAGS=-ldflags="-w -s -buildid=none -linkmode=external" -buildmode=pie -buildvcs=false
 
 build:
-	@go build
+	@go build ${FLAGS}
 
 build-rel:
-	@go build -ldflags="-w -s" -buildvcs=false -o minivpn
+	@go build ${FLAGS} -o minivpn
 	@upx --brute minivpn
-	@GOOS=darwin go build -ldflags="-w -s" -buildvcs=false -o minivpn-osx
-	@GOOS=windows go build -ldflags="-w -s" -buildvcs=false -o minivpn.exe
+	@GOOS=darwin go build -d ${FLAGS} -o minivpn-osx
+	@GOOS=windows go build ${FLAGS} -o minivpn.exe
 
 build-race:
 	@go build -race

proxy.go

@@ -1,9 +1,12 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 	"net"
 	"os"
+	"runtime"
+	"syscall"
 
 	socks5 "github.com/armon/go-socks5"
 	"github.com/ooni/minivpn/vpn"
@@ -40,8 +43,38 @@ func ListenAndServeSocks(opts *vpn.Options) {
 	}
 
 	addr := net.JoinHostPort(ip, port)
-	fmt.Printf("[+] Started socks5 proxy at %s\n", addr)
+	fmt.Printf("[+] Starting socks5 proxy at %s\n", addr)
 	if err := server.ListenAndServe("tcp", addr); err != nil {
-		panic(err)
+		if isErrorAddressAlreadyInUse(err) {
+			fmt.Printf("[!] Address %s already in use\n", addr)
+			for i := 1; i < 1e4; i++ {
+				addr := net.JoinHostPort(ip, fmt.Sprintf("%d", i+1024))
+				fmt.Println("[+] Trying to listen on", addr)
+				if err := server.ListenAndServe("tcp", addr); err != nil {
+					continue
+				}
+			}
+		} else {
+			panic(err)
+		}
+	}
+}
+
+func isErrorAddressAlreadyInUse(err error) bool {
+	var eOsSyscall *os.SyscallError
+	if !errors.As(err, &eOsSyscall) {
+		return false
+	}
+	var errErrno syscall.Errno // doesn't need a "*" (ptr) because it's already a ptr (uintptr)
+	if !errors.As(eOsSyscall, &errErrno) {
+		return false
+	}
+	if errErrno == syscall.EADDRINUSE {
+		return true
+	}
+	const WSAEADDRINUSE = 10048
+	if runtime.GOOS == "windows" && errErrno == WSAEADDRINUSE {
+		return true
 	}
+	return false
 }

scripts/bootstrap-provider

@@ -4,6 +4,8 @@ import os
 import sys
 import urllib.request
 
+PROVIDERS=("calyx", "riseup")
+
 APIS = {
         "calyx": "https://api.vpn.calyx.dev/",
         "riseup": "https://api.float.hexacab.org/"
@@ -31,8 +33,10 @@ key cert.pem
 def check_args():
     if len(sys.argv) != 2:
         print("Usage: bootstrap-provider <provider>")
-        os.exit(1)
-
+        sys.exit(1)
+    if sys.argv[1] not in PROVIDERS:
+        print("Invalid provider")
+        sys.exit(1)
 
 def getPath(provider):
     return os.path.join(os.getcwd(), "data", provider)

vpn/bytes.go

@@ -113,6 +113,9 @@ func bytesUnpadPKCS7(b []byte, blockSize int) ([]byte, error) {
 
 // bytesPadPKCS7 returns the PKCS#7 padding of a byte array.
 func bytesPadPKCS7(b []byte, blockSize int) ([]byte, error) {
+	if blockSize == 0 {
+		return nil, fmt.Errorf("%w: %s", errBadInput, "blocksize cannot be zero")
+	}
 	// If lth mod blockSize == 0, then the input gets appended a whole block size
 	// See https://datatracker.ietf.org/doc/html/rfc5652#section-6.3
 	if blockSize > math.MaxUint8 {
@@ -142,3 +145,13 @@ func bufWriteUint32(buf *bytes.Buffer, val uint32) {
 	binary.BigEndian.PutUint32(numBuf[:], val)
 	buf.Write(numBuf[:])
 }
+
+// bufWriteUint24 is a convenience function that appends to the given buffer
+// 3 bytes containing the big-endian representation of the given uint32 value.
+// Caller is responsible to ensure the passed value does not overflow the
+// maximal capacity of 3 bytes.
+func bufWriteUint24(buf *bytes.Buffer, val uint32) {
+	b := &bytes.Buffer{}
+	bufWriteUint32(b, val)
+	buf.Write(b.Bytes()[1:])
+}

vpn/bytes_test.go

@@ -383,3 +383,8 @@ func Test_bytesPadPKCS7(t *testing.T) {
 		})
 	}
 }
+
+// Regression test for MIV-01-002
+func Test_Crash_bytesPadPCKS7(t *testing.T) {
+	bytesPadPKCS7(nil, 0)
+}

vpn/client.go

@@ -30,9 +30,10 @@ var (
 // - during the handshake (mtu).
 // - after server pushes config options(ip, gw).
 type tunnelInfo struct {
-	mtu int
-	ip  string
-	gw  string
+	mtu    int
+	ip     string
+	gw     string
+	peerID int
 }
 
 // vpnClient is a net.Conn that uses the VPN tunnel. It is a net.Conn with an

vpn/crypto.go

@@ -225,6 +225,7 @@ func (a *dataCipherAES) encrypt(key []byte, data *plaintextData) ([]byte, error)
 			return []byte{}, fmt.Errorf("%w: wrong padding", errCannotEncrypt)
 		}
 		mode := cipher.NewCBCEncrypter(block, data.iv)
+
 		ciphertext := make([]byte, len(data.plaintext))
 		mode.CryptBlocks(ciphertext, data.plaintext)
 		return ciphertext, nil
@@ -370,17 +371,3 @@ func pHash(result, secret, seed []byte, hash func() hash.Hash) {
 		a = h.Sum(nil)
 	}
 }
-
-// TODO(ainghazal): this function is not needed if we use Hash.Size()
-// TODO: refactor, use hash.Reset() and create hash function in the constructor.
-func getHashLength(s string) uint8 {
-	switch s {
-	case "sha1":
-		return 20
-	case "sha256":
-		return 32
-	case "sha512":
-		return 64
-	}
-	return 0
-}