security audit fixes #23

ainghazal · 2022-08-29T09:57:30Z

in progress: address security fixes found by 7asecurity.

bassosimone · 2022-09-21T06:28:10Z

vpn/bytes.go

@@ -113,6 +113,10 @@ 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")
+


Suggested change

MIV-01-001: Possible DoS via index out of range (Low) During the fuzzing process of the minivpn/vpn package, it was found that the parseServerHardResetPacket function fails to access the first element of the serverHardReset payload when the payload is empty. This led to the following crash error: “panic: runtime error: index out of range [0] with length 0”. Please note that the parseServerHardResetPacket function is used in association with the newServerHardReset method which checks that the payload is not empty and returns an error otherwise. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-001

During the fuzzing process of the minivpn/vpn package, it was found that the bytesPadPKCS7 function fails to perform a modulo operation when the blockSize argument is zero. This led to the following crash error: “panic: runtime error: integer divide by zero”. This issue affects projects using minivpn/vpn as a third party library or copying the vulnerable code into another project, if an attacker is able to control the blockSize value. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-002

MIV-01-003: Possible DoS via nil Pointer Dereference (Medium) During the fuzzing process of the minivpn/vpn package, it was found that the EncryptAndEncodePayload function fails when dcs.dataCipher is null. This issue led to a nil pointer dereference with the following crash error: “panic: runtime error: invalid memory address or nil pointer dereference”. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-003

MIV-01-004: Possible DoS via Index Out of Range (Medium) During the fuzzing process of the minivpn/vpn package, it was found that the maybeAddCompressPadding function fails to validate access to the array of byte location provided as an argument. This led to the following crash error: “panic: runtime error: index out of range [-1]”. Please note the EncryptAndEncodePayload function, which invokes maybeAddCompressPadding, fails to perform the length check as well. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-004

MIV-01-005: Possible DoS via Slice Bounds Out of Range (High) During the fuzzing process of the minivpn/vpn package, it was found that the decodeEncryptedPayloadNonAEAD function fails to perform slicing when buf is not long enough. This led to the following crash error: “panic: runtime error: slice bounds out of range [:36] with capacity 32”. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-005

Here I add a simple retry strategy that increments the port by one if the default or configured listening port is in use. While used as a library, it is be the caller's responsibility to catch the error and retry accordingly, this retry strategy is only provided for convenience while using the reference executable. - Reference: MIV-01-007

MIV-01-008 Possible File Disclosure via Error Messages (Info) It was found that the minivpn client reveals the contents of local files via error messages based on the user-supplied configuration path. A malicious attacker might leverage this weakness to fool some minivpn users, in order to gain access to data in local system files from the victim computer. This might be accomplished through social engineering, for example, providing fake minivpn instructions to a victim user and asking for the resulting minivpn errors via email or instant messaging. Fix consists of referencing the line number instead of the line content. - Reference: MIV-01-008

Otherwise, bogus provider names will lead to the creation of arbitrary folders. - Reference: MIV-01-011

On the topic of the canary stack protections, check golang/go#21871 (comment) - Reference: MIV-01-010

Revert a change by which we had ceased to explicitely set min and max TLS version. Apparently uTLS does not pick those from the passed clienthelo spec. Thanks to 7asecurity for raising this issue during their security audit. - Reference: MIV-01-012

As pointed out by the security audit, the use of P_DATA_V1 format was too conspicuous. In order to be able to use the v2 format, we needed to implement a few requirements: - client needs to push a specific IV_PROTO bit, so that the server knows that they have to assign us a peer-id. - client needs to parse the pushed peer-id from the options pushed by the server. - this peer-id is needed to encrypt (is passed to the authenticated data in GCM/AEAD mode) and to decrypt data payloads. As a note, the peer-id expires in the server some time after the client has terminated the session. Useful pointers: https://build.openvpn.net/doxygen/group__data__crypto.html https://sourceforge.net/p/openvpn/mailman/message/32979583/ https://community.openvpn.net/openvpn/attachment/ticket/268/0001-Always-push-basic-set-of-peer-info-values-to-server-v2.patch

decompression was only working for AEAD (GCM) before. the compress=stub case seems not to be working when compressing, but since the compress option appears to be deprecated in openvpn 2.5.x we will not care about this case for the time being.

this was a pending refactor: we really don't need to instatiate the hmac each time. we also don't need the getHashLength function anymore.

hellais · 2022-12-02T10:49:00Z

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")


MIV-01-002 Possible DoS via Integer Division by Zero

While we are at it, I think we should also protect against negative blockSize values. The assumption in the report is that the attacker may be able to control the blockSize value, hence if that is the case, this snippet will also lead to a crash:

b := []byte("a") blockSize := -1 psiz := blockSize - len(b) % blockSize padding := bytes.Repeat([]byte{byte(psiz)}, psiz)

via

panic: bytes: negative Repeat count goroutine 1 [running]: bytes.Repeat(0xc0000a2f27, 0x1, 0x1, 0xffffffffffffffff, 0x46, 0x4db500, 0xc000010080) /usr/local/go/src/bytes/bytes.go:499 +0x1aa main.main() /home/runner/3bh1srnac6d/main.go:11 +0x62

hellais · 2022-12-02T10:57:56Z

vpn/data.go

+		return []byte{}, fmt.Errorf("%w: nothing to encrypt", errCannotEncrypt)
+	}
+	if dcs == nil || dcs.dataCipher == nil {
+		return []byte{}, fmt.Errorf("%w: %s", errCannotEncrypt, fmt.Errorf("data chan not initialized"))


MIV-01-003 Possible DoS via nil Pointer Dereference

👍 for also adding regress tests for it

hellais · 2022-12-02T10:59:43Z

vpn/data.go

-func maybeAddCompressPadding(b []byte, compress compression, blockSize uint8) ([]byte, error) {
+func doPadding(b []byte, compress compression, blockSize uint8) ([]byte, error) {
+	if len(b) == 0 {
+		return nil, fmt.Errorf("%w: nothing to pad", errBadInput)


MIV-01-004 Possible DoS via Index Out of Range

hellais · 2022-12-02T11:04:57Z

vpn/data.go

@@ -502,37 +540,36 @@ func decodeEncryptedPayloadAEAD(buf []byte, state *dataChannelState) (*encrypted
 	encrypted := &encryptedData{
 		iv:         iv.Bytes(),
 		ciphertext: payload.Bytes(),
-		aead:       packet_id,
+		aead:       headers.Bytes(),
 	}
 	return encrypted, nil
 }

 func decodeEncryptedPayloadNonAEAD(buf []byte, state *dataChannelState) (*encryptedData, error) {


MIV-01-005 Possible DoS via Slice Bounds Out of Range

hellais · 2022-12-02T11:15:19Z

MIV-01-006

It is my understanding that a fix for this has not been implemented. Yet according to the audit report it seems like this is a flaw in the protocol design of OpenVPN and the issue is present in the reference implementation as well. Apparently the issue is not so severe in there, because a retry mechanism is in place.

While the retry mechanism might be effective as a protection against bad network conditions, I don't think it's really effective as a protection against a motivated attacker (such as a state actor or an ISP). In that case since they control the traffic flows, they can just pre all traffic that looks like OpenVPN (or that doesn't look like something they would recognise) with a bad payload leading to the connection never establishing.

In light of the fact that our primary goal for this client is measurement, we actually care to record these kinds of connectivity issues and while we might want to add retries eventually to understand the persistence of the attacker and rule out potential transient network conditions, I don't think it's necessarily critical to solve this in order to land this.

hellais · 2022-12-02T11:23:57Z

proxy.go

 	if err := server.ListenAndServe("tcp", addr); err != nil {
-		panic(err)
+		if isErrorAddressAlreadyInUse(err) {


I was thinking you could perhaps just call server.ListendAndServe with addr = {ip}:0 so as to make the OS pick the next available port for you.

I then saw, though, that the sock5 library you are using doesn't return the net.listener (https://github.com/armon/go-socks5/blob/master/socks5.go#L100) so if you actually need to know the picked port you have no way of extracting it :(

I guess we can leave this as-is for the time being, but it might be worth opening an issue as future work to open PR on https://github.com/armon/go-socks5 to return the listener (or set it as an attribute to the server struct).

MIV-01-007 Possible DoS via Predictable Port Usage

hellais · 2022-12-02T11:24:48Z

vpn/packet.go

@@ -335,13 +357,13 @@ func newServerHardReset(b []byte) (*serverHardReset, error) {
 // parseServerHardResetPacket returns the sessionID received from the server, or an
 // error if we could not parse the message.
 func parseServerHardResetPacket(p *serverHardReset) (sessionID, error) {
+	if len(p.payload) < 10 {


MIV-01-001 Possible DoS via index out of range

hellais · 2022-12-02T11:25:58Z

vpn/options.go

@@ -437,7 +459,7 @@ func parseOption(o *Options, dir, key string, p []string) error {
 			return e
 		}
 	default:
-		log.Println("warn: unsupported key:", key)
+		log.Printf("warn: unsupported key in line %d\n", lineno)


MIV-01-008 Possible File Disclosure via Error Messages

hellais · 2022-12-02T11:28:19Z

MIV-01-009

This seems very hard to do. I think we ought to document this as future work.

hellais · 2022-12-02T11:30:19Z

scripts/bootstrap-provider

-        os.exit(1)
-
+        sys.exit(1)
+    if sys.argv[1] not in PROVIDERS:


MIV-01-011 Missing Verification on VPN bootstrap-provider Utility Script

hellais · 2022-12-02T11:30:54Z

vpn/tls.go

@@ -199,6 +197,9 @@ func initTLS(session *session, cfg *certConfig) (*tls.Config, error) {
 		VerifyPeerCertificate: customVerify,
 		// disable DynamicRecordSizing to lower distinguishability.
 		DynamicRecordSizingDisabled: true,
+		// uTLS does not pick min/max version from the passed spec
+		MinVersion: tls.VersionTLS12,
+		MaxVersion: tls.VersionTLS13,


MIV-01-012 Possible MitM due to Missing TLS MinVersion

hellais

I have reviewed the PR comparing it against the report.

I think the only thing that should be addressed prior to merge is:

adding checks for negative blockSize values (see: https://github.com/ooni/minivpn/pull/23/files#r1038019424)

Beyond that I think we ought to document as future work the harder bits that are left un-addressed, but I don't think that should be blocking on landing this.

Thanks for working on this!

ainghazal · 2022-12-02T11:47:26Z

thanks for the review @hellais

re. MIV-01-009, the only outstanding pattern is the first of the three reported: #34 - P_ACK2 is addressed in this PR, and the multiple reset packets was a side-effect of not reusing properly the same underlying vpn connection for the long-lived socks5 proxy (which, incidentally, can be used for an implementation of a ooni tunnel).

I am going to go ahead and fix the issue with blockSize and merge this series, so that we can rebase the remaining PRs.

ainghazal · 2022-12-02T12:12:54Z

MIV-01-006

It is my understanding that a fix for this has not been implemented. Yet according to the audit report it seems like this is a flaw in the protocol design of OpenVPN and the issue is present in the reference implementation as well. Apparently the issue is not so severe in there, because a retry mechanism is in place.

While the retry mechanism might be effective as a protection against bad network conditions, I don't think it's really effective as a protection against a motivated attacker (such as a state actor or an ISP). In that case since they control the traffic flows, they can just pre all traffic that looks like OpenVPN (or that doesn't look like something they would recognise) with a bad payload leading to the connection never establishing.

In light of the fact that our primary goal for this client is measurement, we actually care to record these kinds of connectivity issues and while we might want to add retries eventually to understand the persistence of the attacker and rule out potential transient network conditions, I don't think it's necessarily critical to solve this in order to land this.

yes, I think this is a wontfix for the moment. however, since I wrote this patch series I've revisited the reliability layer implementation and I think it's quite close to be in an usable state (I keep debugging it a bit, and is missing proper tests). In my priority lists for further work in minivpn, I think implementing proper DoS protection (--tls-auth and --tls-crypt) is more urgent. I documented a bit how the DoS currently compares between this and the ref implementation in here - it's interesting that, if the injection rate is kept low, the handshake manages to complete successfully.

on the other hand, we want to be able to measure DoS happening - I think the trick is to be as sensitive as openvpn itself.

ainghazal marked this pull request as draft August 29, 2022 09:57

bassosimone reviewed Sep 21, 2022

View reviewed changes

ainghazal added 2 commits October 10, 2022 17:51

ainghazal force-pushed the bug/security-audit-fixes branch from afcebea to 3022603 Compare October 19, 2022 18:06

ainghazal added 9 commits October 24, 2022 11:21

bug: validate user input against known list of providers

b8da995

Otherwise, bogus provider names will lead to the creation of arbitrary folders. - Reference: MIV-01-011

harden binary build

1bf975c

On the topic of the canary stack protections, check golang/go#21871 (comment) - Reference: MIV-01-010

ainghazal force-pushed the bug/security-audit-fixes branch from 3022603 to 197ce1a Compare October 24, 2022 09:55

ainghazal added 2 commits October 28, 2022 20:13

refactor: use hash.Reset() and Size()

868a982

this was a pending refactor: we really don't need to instatiate the hmac each time. we also don't need the getHashLength function anymore.

ainghazal marked this pull request as ready for review November 22, 2022 13:33

ainghazal requested a review from hellais November 22, 2022 13:33

hellais reviewed Dec 2, 2022

View reviewed changes

ainghazal mentioned this pull request Dec 2, 2022

modify go-socks5 to return the listener #36

Open

1 task

ainghazal merged commit 868a982 into ooni:main Dec 3, 2022

ainghazal deleted the bug/security-audit-fixes branch February 8, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security audit fixes #23

security audit fixes #23

ainghazal commented Aug 29, 2022

bassosimone Sep 21, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais commented Dec 2, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais commented Dec 2, 2022

hellais Dec 2, 2022

hellais Dec 2, 2022

hellais left a comment

ainghazal commented Dec 2, 2022

ainghazal commented Dec 2, 2022

security audit fixes #23

security audit fixes #23

Conversation

ainghazal commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellais commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellais commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellais left a comment

Choose a reason for hiding this comment

ainghazal commented Dec 2, 2022

ainghazal commented Dec 2, 2022