From 8f85f49404aadc5684dfa52181adbae9f0d7d806 Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Thu, 7 Nov 2019 16:39:27 -0800 Subject: [PATCH] Initial pass at race condition fixes --- .drone.yml | 2 +- app/plugins/networkCheck.go | 8 ++++ event/eventmanager.go | 1 + go.mod | 7 +++- go.sum | 12 ++++++ model/group.go | 22 ++++++----- model/profile.go | 3 +- protocol/connections/peerserverconnection.go | 6 +++ protocol/fuzzing/groups/fuzz.go | 38 +++++++++++++++++++ protocol/fuzzing/invites/fuzz.go | 20 ++++++++++ storage/profile_store_test.go | 3 ++ testing/cwtch_peer_server_integration_test.go | 3 ++ testing/tests.sh | 27 ++++++------- 13 files changed, 126 insertions(+), 26 deletions(-) create mode 100644 protocol/fuzzing/groups/fuzz.go create mode 100644 protocol/fuzzing/invites/fuzz.go diff --git a/.drone.yml b/.drone.yml index 2973775..5366861 100644 --- a/.drone.yml +++ b/.drone.yml @@ -43,7 +43,7 @@ pipeline: commands: - ./tor -f ./torrc - sleep 15 - - go test -v cwtch.im/cwtch/testing + - go test -v cwtch.im/cwtch/testing/ notify-email: image: drillster/drone-email host: build.openprivacy.ca diff --git a/app/plugins/networkCheck.go b/app/plugins/networkCheck.go index 3bcfcfe..6f569f5 100644 --- a/app/plugins/networkCheck.go +++ b/app/plugins/networkCheck.go @@ -6,6 +6,7 @@ import ( "git.openprivacy.ca/openprivacy/libricochet-go/connectivity" "git.openprivacy.ca/openprivacy/libricochet-go/log" "git.openprivacy.ca/openprivacy/libricochet-go/policies" + "sync" "time" ) @@ -18,6 +19,7 @@ type networkCheck struct { breakChan chan bool running bool offline bool + offlineLock sync.Mutex } // NewNetworkCheck returns a Plugin that when started will attempt various network tests @@ -57,21 +59,25 @@ func (nc *networkCheck) run() { case event.ServerStateChange: // if we successfully connect / authenticated to a remote server / peer then we obviously have internet connectionState := e.Data[event.ConnectionState] + nc.offlineLock.Lock() if nc.offline && (connectionState == connections.ConnectionStateName[connections.AUTHENTICATED] || connectionState == connections.ConnectionStateName[connections.CONNECTED]) { lastMessageReceived = time.Now() nc.bus.Publish(event.NewEvent(event.NetworkStatus, map[event.Field]string{event.Error: "", event.Status: "Success"})) nc.offline = false } + nc.offlineLock.Unlock() default: // if we receive either an encrypted group message or a peer acknowledgement we can assume the network // is up and running (our onion service might still not be available, but we would aim to detect that // through other actions // we reset out timer lastMessageReceived = time.Now() + nc.offlineLock.Lock() if nc.offline { nc.bus.Publish(event.NewEvent(event.NetworkStatus, map[event.Field]string{event.Error: "", event.Status: "Success"})) nc.offline = false } + nc.offlineLock.Unlock() } case <-time.After(tickTime): // if we haven't received an action in the last minute...kick off a set of testing @@ -104,6 +110,8 @@ func (nc *networkCheck) checkConnection(onion string) { } return err }) + nc.offlineLock.Lock() + defer nc.offlineLock.Unlock() // regardless of the outcome we want to report a status to let anyone who might care know that we did do a check if err != nil { log.Debugf("publishing network error for %v", onion) diff --git a/event/eventmanager.go b/event/eventmanager.go index c571ca0..2785b36 100644 --- a/event/eventmanager.go +++ b/event/eventmanager.go @@ -40,6 +40,7 @@ type manager struct { } // Manager is an interface for an event bus +// FIXME this interface lends itself to race conditions around channels type Manager interface { Subscribe(Type, Queue) Publish(Event) diff --git a/go.mod b/go.mod index f706204..5e72e4e 100644 --- a/go.mod +++ b/go.mod @@ -2,20 +2,23 @@ module cwtch.im/cwtch require ( cwtch.im/tapir v0.1.11 - git.openprivacy.ca/openprivacy/libricochet-go v1.0.6 + git.openprivacy.ca/openprivacy/libricochet-go v1.0.8 github.com/c-bata/go-prompt v0.2.3 github.com/client9/misspell v0.3.4 // indirect + github.com/dvyukov/go-fuzz v0.0.0-20191022152526-8cb203812681 // indirect + github.com/elazarl/go-bindata-assetfs v1.0.0 // indirect github.com/golang/protobuf v1.3.2 github.com/gordonklaus/ineffassign v0.0.0-20190601041439-ed7b1b5ee0f8 // indirect github.com/mattn/go-colorable v0.1.2 // indirect github.com/mattn/go-runewidth v0.0.4 // indirect github.com/mattn/go-tty v0.0.0-20190424173100-523744f04859 // indirect github.com/pkg/term v0.0.0-20190109203006-aa71e9d9e942 // indirect + github.com/stephens2424/writerset v1.0.2 // indirect github.com/struCoder/pidusage v0.1.2 golang.org/x/crypto v0.0.0-20190701094942-4def268fd1a4 golang.org/x/lint v0.0.0-20190930215403-16217165b5de // indirect golang.org/x/net v0.0.0-20190628185345-da137c7871d7 - golang.org/x/tools v0.0.0-20191031220737-6d8f1af9ccc0 // indirect + golang.org/x/tools v0.0.0-20191108175616-46f5a7f28bf0 // indirect ) go 1.13 diff --git a/go.sum b/go.sum index c54c71f..2fb8382 100644 --- a/go.sum +++ b/go.sum @@ -6,6 +6,9 @@ git.openprivacy.ca/openprivacy/libricochet-go v1.0.4 h1:GWLMJ5jBSIC/gFXzdbbeVz7f git.openprivacy.ca/openprivacy/libricochet-go v1.0.4/go.mod h1:yMSG1gBaP4f1U+RMZXN85d29D39OK5s8aTpyVRoH5FY= git.openprivacy.ca/openprivacy/libricochet-go v1.0.6 h1:5o4K2qn3otEE1InC5v5CzU0yL7Wl7DhVp4s8H3K6mXY= git.openprivacy.ca/openprivacy/libricochet-go v1.0.6/go.mod h1:yMSG1gBaP4f1U+RMZXN85d29D39OK5s8aTpyVRoH5FY= +git.openprivacy.ca/openprivacy/libricochet-go v1.0.8 h1:HVoyxfivFaEtkfO5K3piD6oq6MQB1qGF5IB2EYXeCW8= +git.openprivacy.ca/openprivacy/libricochet-go v1.0.8/go.mod h1:6I+vO9Aagv3/yUWv+e7A57H8tgXgR67FCjfSj9Pp970= +github.com/Julusian/godocdown v0.0.0-20170816220326-6d19f8ff2df8/go.mod h1:INZr5t32rG59/5xeltqoCJoNY7e5x/3xoY9WSWVWg74= github.com/agl/ed25519 v0.0.0-20170116200512-5312a6153412 h1:w1UutsfOrms1J05zt7ISrnJIXKzwaspym5BTKGx93EI= github.com/agl/ed25519 v0.0.0-20170116200512-5312a6153412/go.mod h1:WPjqKcmVOxf0XSf3YxCJs6N6AOSrOx3obionmG7T0y0= github.com/c-bata/go-prompt v0.2.3 h1:jjCS+QhG/sULBhAaBdjb2PlMRVaKXQgn+4yzaauvs2s= @@ -16,6 +19,10 @@ github.com/cretz/bine v0.1.0 h1:1/fvhLE+fk0bPzjdO5Ci+0ComYxEMuB1JhM4X5skT3g= github.com/cretz/bine v0.1.0/go.mod h1:6PF6fWAvYtwjRGkAuDEJeWNOv3a2hUouSP/yRYXmvHw= github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dvyukov/go-fuzz v0.0.0-20191022152526-8cb203812681 h1:3WV5aRRj1ELP3RcLlBp/v0WJTuy47OQMkL9GIQq8QEE= +github.com/dvyukov/go-fuzz v0.0.0-20191022152526-8cb203812681/go.mod h1:11Gm+ccJnvAhCNLlf5+cS9KjtbaD5I5zaZpFMsTHWTw= +github.com/elazarl/go-bindata-assetfs v1.0.0 h1:G/bYguwHIzWq9ZoyUQqrjTmJbbYn3j3CKKpKinvZLFk= +github.com/elazarl/go-bindata-assetfs v1.0.0/go.mod h1:v+YaWX3bdea5J/mo8dSETolEo7R71Vk1u8bnjau5yw4= github.com/golang/protobuf v1.2.0 h1:P3YflyNX/ehuJFLhxviNdFxQPkGK5cDcApsge1SqnvM= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs= @@ -34,6 +41,9 @@ github.com/pkg/term v0.0.0-20190109203006-aa71e9d9e942 h1:A7GG7zcGjl3jqAqGPmcNjd github.com/pkg/term v0.0.0-20190109203006-aa71e9d9e942/go.mod h1:eCbImbZ95eXtAUIbLAuAVnBnwf83mjf6QIVH8SHYwqQ= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/robertkrimen/godocdown v0.0.0-20130622164427-0bfa04905481/go.mod h1:C9WhFzY47SzYBIvzFqSvHIR6ROgDo4TtdTuRaOMjF/s= +github.com/stephens2424/writerset v1.0.2 h1:znRLgU6g8RS5euYRcy004XeE4W+Tu44kALzy7ghPif8= +github.com/stephens2424/writerset v1.0.2/go.mod h1:aS2JhsMn6eA7e82oNmW4rfsgAOp9COBTTl8mzkwADnc= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= @@ -66,4 +76,6 @@ golang.org/x/tools v0.0.0-20190311212946-11955173bddd h1:/e+gpKk9r3dJobndpTytxS2 golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20191031220737-6d8f1af9ccc0 h1:+o3suEKE/4hCUt6qjV8SDcVZhz2dO8UWlHliCa+4bvg= golang.org/x/tools v0.0.0-20191031220737-6d8f1af9ccc0/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20191108175616-46f5a7f28bf0 h1:crJFnQNts8HmLQ4G+0O/30eU/J8peLNNKhX1jnzV58s= +golang.org/x/tools v0.0.0-20191108175616-46f5a7f28bf0/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/model/group.go b/model/group.go index 377badc..d2444a5 100644 --- a/model/group.go +++ b/model/group.go @@ -182,7 +182,9 @@ func (g *Group) EncryptMessage(message *protocol.DecryptedGroupMessage) ([]byte, return nil, err } wire, err := proto.Marshal(message) - utils.CheckError(err) + if err != nil { + return nil, err + } encrypted := secretbox.Seal(nonce[:], []byte(wire), &nonce, &g.GroupKey) return encrypted, nil } @@ -190,14 +192,16 @@ func (g *Group) EncryptMessage(message *protocol.DecryptedGroupMessage) ([]byte, // DecryptMessage takes a ciphertext and returns true and the decrypted message if the // cipher text can be successfully decrypted,else false. func (g *Group) DecryptMessage(ciphertext []byte) (bool, *protocol.DecryptedGroupMessage) { - var decryptNonce [24]byte - copy(decryptNonce[:], ciphertext[:24]) - decrypted, ok := secretbox.Open(nil, ciphertext[24:], &decryptNonce, &g.GroupKey) - if ok { - dm := &protocol.DecryptedGroupMessage{} - err := proto.Unmarshal(decrypted, dm) - if err == nil { - return true, dm + if len(ciphertext) > 24 { + var decryptNonce [24]byte + copy(decryptNonce[:], ciphertext[:24]) + decrypted, ok := secretbox.Open(nil, ciphertext[24:], &decryptNonce, &g.GroupKey) + if ok { + dm := &protocol.DecryptedGroupMessage{} + err := proto.Unmarshal(decrypted, dm) + if err == nil { + return true, dm + } } } return false, nil diff --git a/model/profile.go b/model/profile.go index fef480d..8460c69 100644 --- a/model/profile.go +++ b/model/profile.go @@ -320,7 +320,7 @@ func (p *Profile) VerifyGroupMessage(onion string, groupID string, message strin m := groupID + group.GroupServer + string(ciphertext) decodedPub, err := base32.StdEncoding.DecodeString(strings.ToUpper(onion)) - if err == nil { + if err == nil && len(decodedPub) >= 32 { return ed25519.Verify(decodedPub[:32], []byte(m), signature) } return false @@ -456,6 +456,7 @@ func (p *Profile) EncryptMessageToGroup(message string, groupID string) ([]byte, PreviousMessageSig: prevSig, Padding: padding[:], } + ciphertext, err := group.EncryptMessage(dm) if err != nil { return nil, nil, err diff --git a/protocol/connections/peerserverconnection.go b/protocol/connections/peerserverconnection.go index 86b527c..55be614 100644 --- a/protocol/connections/peerserverconnection.go +++ b/protocol/connections/peerserverconnection.go @@ -14,6 +14,7 @@ import ( "git.openprivacy.ca/openprivacy/libricochet-go/identity" "git.openprivacy.ca/openprivacy/libricochet-go/log" "golang.org/x/crypto/ed25519" + "sync" "time" ) @@ -21,6 +22,7 @@ import ( type PeerServerConnection struct { connection.AutoConnectionHandler Server string + stateMutex sync.Mutex state ConnectionState connection *connection.Connection protocolEngine Engine @@ -40,11 +42,15 @@ func NewPeerServerConnection(engine Engine, serverhostname string) *PeerServerCo // GetState returns the current connection state func (psc *PeerServerConnection) GetState() ConnectionState { + psc.stateMutex.Lock() + defer psc.stateMutex.Unlock() return psc.state } func (psc *PeerServerConnection) setState(state ConnectionState) { log.Debugf("Setting State to %v for %v\n", ConnectionStateName[state], psc.Server) + psc.stateMutex.Lock() + defer psc.stateMutex.Unlock() psc.state = state psc.protocolEngine.EventManager().Publish(event.NewEvent(event.ServerStateChange, map[event.Field]string{ event.GroupServer: string(psc.Server), diff --git a/protocol/fuzzing/groups/fuzz.go b/protocol/fuzzing/groups/fuzz.go new file mode 100644 index 0000000..08b677f --- /dev/null +++ b/protocol/fuzzing/groups/fuzz.go @@ -0,0 +1,38 @@ +package groups + +import ( + "crypto/rand" + "cwtch.im/cwtch/event" + "cwtch.im/cwtch/peer" + "golang.org/x/crypto/nacl/secretbox" + "io" +) + +// Fuzz various group related functions +func Fuzz(data []byte) int { + peer := peer.NewCwtchPeer("fuzz") + peer.Init(event.NewEventManager()) + + inviteid, err := peer.GetProfile().ProcessInvite(string(data), peer.GetProfile().Onion) + + if err != nil { + if inviteid != "" { + panic("should not have added a group on err") + } + return 1 + } + + id, _, _ := peer.StartGroup("2c3kmoobnyghj2zw6pwv7d57yzld753auo3ugauezzpvfak3ahc4bdyd") + var nonce [24]byte + io.ReadFull(rand.Reader, nonce[:]) + encrypted := secretbox.Seal(nonce[:], data, &nonce, &peer.GetGroup(id).GroupKey) + ok, _, _, _ := peer.GetProfile().AttemptDecryption(encrypted, data) + if ok { + panic("this probably shouldn't happen") + } + ok = peer.GetProfile().VerifyGroupMessage(string(data), string(data), string(data), 0, encrypted, data) + if ok { + panic("this probably shouldn't happen") + } + return 0 +} diff --git a/protocol/fuzzing/invites/fuzz.go b/protocol/fuzzing/invites/fuzz.go new file mode 100644 index 0000000..5d94886 --- /dev/null +++ b/protocol/fuzzing/invites/fuzz.go @@ -0,0 +1,20 @@ +package invites + +import ( + "cwtch.im/cwtch/event" + "cwtch.im/cwtch/peer" +) + +// Fuzz import group function +func Fuzz(data []byte) int { + peer := peer.NewCwtchPeer("fuzz") + peer.Init(event.NewEventManager()) + err := peer.ImportGroup(string(data)) + if err != nil { + if len(peer.GetGroups()) > 0 { + panic("group added despite error") + } + return 0 + } + return 1 +} diff --git a/storage/profile_store_test.go b/storage/profile_store_test.go index 777f04c..f4ef4b2 100644 --- a/storage/profile_store_test.go +++ b/storage/profile_store_test.go @@ -1,3 +1,6 @@ +// Known race issue with event bus channel closure +// +build !race + package storage import ( diff --git a/testing/cwtch_peer_server_integration_test.go b/testing/cwtch_peer_server_integration_test.go index 184d095..6d58163 100644 --- a/testing/cwtch_peer_server_integration_test.go +++ b/testing/cwtch_peer_server_integration_test.go @@ -1,3 +1,6 @@ +// FIXME currently we have channel related races inside the event bus which cause this to trigger. Remove once fixed +// +build !race + package testing import ( diff --git a/testing/tests.sh b/testing/tests.sh index d8db9f8..52ff8f8 100755 --- a/testing/tests.sh +++ b/testing/tests.sh @@ -2,20 +2,21 @@ set -e pwd -go test ${1} -coverprofile=model.cover.out -v ./model -go test ${1} -coverprofile=event.cover.out -v ./event +GORACE="haltonerror=1" +go test -race ${1} -coverprofile=model.cover.out -v ./model +go test -race ${1} -coverprofile=event.cover.out -v ./event go test ${1} -coverprofile=storage.cover.out -v ./storage -go test ${1} -coverprofile=peer.connections.cover.out -v ./protocol/connections -go test ${1} -coverprofile=protocol.spam.cover.out -v ./protocol/connections/spam -go test ${1} -coverprofile=peer.fetch.cover.out -v ./protocol/connections/fetch -go test ${1} -coverprofile=peer.listen.cover.out -v ./protocol/connections/listen -go test ${1} -coverprofile=peer.send.cover.out -v ./protocol/connections/send -go test ${1} -coverprofile=peer.cover.out -v ./peer -go test ${1} -coverprofile=server.fetch.cover.out -v ./server/fetch -go test ${1} -coverprofile=server.listen.cover.out -v ./server/listen -go test ${1} -coverprofile=server.send.cover.out -v ./server/send -go test ${1} -coverprofile=server.metrics.cover.out -v ./server/metrics -go test ${1} -coverprofile=server.cover.out -v ./server +go test -race ${1} -coverprofile=peer.connections.cover.out -v ./protocol/connections +go test -race ${1} -coverprofile=protocol.spam.cover.out -v ./protocol/connections/spam +go test -race ${1} -coverprofile=peer.fetch.cover.out -v ./protocol/connections/fetch +go test -race ${1} -coverprofile=peer.listen.cover.out -v ./protocol/connections/listen +go test -race ${1} -coverprofile=peer.send.cover.out -v ./protocol/connections/send +go test -race ${1} -coverprofile=peer.cover.out -v ./peer +go test -race ${1} -coverprofile=server.fetch.cover.out -v ./server/fetch +go test -race ${1} -coverprofile=server.listen.cover.out -v ./server/listen +go test -race ${1} -coverprofile=server.send.cover.out -v ./server/send +go test -race ${1} -coverprofile=server.metrics.cover.out -v ./server/metrics +go test -race ${1} -coverprofile=server.cover.out -v ./server echo "mode: set" > coverage.out && cat *.cover.out | grep -v mode: | sort -r | \ awk '{if($1 != last) {print $0;last=$1}}' >> coverage.out rm -rf *.cover.out