From a61be298527f6ae590268bfedcf0693afef27fc3 Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 29 Jan 2020 20:52:44 +0100 Subject: [PATCH 1/6] constant-time auth verification --- secrethandshake/state.go | 48 ++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/secrethandshake/state.go b/secrethandshake/state.go index e4d1da7..74ace51 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -11,11 +11,13 @@ package secrethandshake import ( "bytes" + "unsafe" // ☢ ☣ "crypto/hmac" "crypto/rand" "crypto/sha256" "crypto/sha512" + "crypto/subtle" "go.cryptoscope.co/secretstream/internal/lo25519" @@ -169,6 +171,11 @@ var nullHello [ed25519.SignatureSize + ed25519.PublicKeySize]byte // verifyClientAuth returns whether a buffer contains a valid clientAuth message func (s *State) verifyClientAuth(data []byte) bool { + // this branch is okay because there are no secrets involved + if len(data) != ed25519.SignatureSize+ed25519.PublicKeySize+box.Overhead { + return false + } + var cvSec, aBob [32]byte extra25519.PrivateKeyToCurve25519(&cvSec, &s.local.Secret) curve25519.ScalarMult(&aBob, &cvSec, &s.remoteExchange.Public) @@ -182,33 +189,20 @@ func (s *State) verifyClientAuth(data []byte) bool { s.hello = make([]byte, 0, len(data)-16) - var nonce [24]byte // always 0? - var openOk bool - s.hello, openOk = box.OpenAfterPrecomputation(s.hello, data, &nonce, &s.secret2) + var ( + nonce [24]byte // always 0? + openOk bool + sig [ed25519.SignatureSize]byte + public [ed25519.PublicKeySize]byte + ) - var sig [ed25519.SignatureSize]byte - var public [ed25519.PublicKeySize]byte - /* TODO: is this const time!?! - - this is definetly not: - if !openOK { - s.hello = nullHello - } - copy(sig, ...) - copy(pub, ...) - */ - if openOk { - copy(sig[:], s.hello[:ed25519.SignatureSize]) - copy(public[:], s.hello[ed25519.SignatureSize:]) - - } else { - copy(sig[:], nullHello[:ed25519.SignatureSize]) - copy(public[:], nullHello[ed25519.SignatureSize:]) - } + s.hello, openOk = box.OpenAfterPrecomputation(s.hello, data, &nonce, &s.secret2) + // subtle API requires an int containing 0 or 1, we only have bool. + // we can't branch because openOk is secret. + okInt := int(*((*byte)(unsafe.Pointer(&openOk)))) - if lo25519.IsEdLowOrder(sig[:32]) { - openOk = false - } + subtle.ConstantTimeCopy(okInt, sig[:], s.hello[:ed25519.SignatureSize]) + subtle.ConstantTimeCopy(okInt, public[:], s.hello[ed25519.SignatureSize:ed25519.SignatureSize+ed25519.PublicKeySize]) var sigMsg bytes.Buffer sigMsg.Write(s.appKey) @@ -217,7 +211,9 @@ func (s *State) verifyClientAuth(data []byte) bool { verifyOk := ed25519.Verify(&public, sigMsg.Bytes(), &sig) copy(s.remotePublic[:], public[:]) - return openOk && verifyOk + + keyOk := !lo25519.IsEdLowOrder(sig[:32]) + return openOk && keyOk && verifyOk } // createServerAccept returns a buffer containing a serverAccept message From 0b8a3e13fc00df9724f1a00ca17d903ed891b7cf Mon Sep 17 00:00:00 2001 From: Henry Date: Fri, 16 Aug 2019 09:39:48 +0200 Subject: [PATCH 2/6] add test case with invalid clientAuth packet --- secrethandshake/conn_test.go | 109 +++++++++++++++++++++++++++++++++++ secrethandshake/state.go | 3 + 2 files changed, 112 insertions(+) diff --git a/secrethandshake/conn_test.go b/secrethandshake/conn_test.go index 060121d..dcb9db0 100644 --- a/secrethandshake/conn_test.go +++ b/secrethandshake/conn_test.go @@ -3,11 +3,15 @@ package secrethandshake import ( + "bytes" "io" "log" + "math/rand" "os" "reflect" "testing" + + "github.com/pkg/errors" ) // StupidRandom always reads itself. Goal is determinism. @@ -88,3 +92,108 @@ func TestAuth(t *testing.T) { t.Error("secrets not equal") } } + +func TestAuthWrong(t *testing.T) { + log.SetOutput(os.Stdout) + + keySrv, err := GenEdKeyPair(StupidRandom(0)) + if err != nil { + t.Fatal(err) + } + + keyClient, err := GenEdKeyPair(StupidRandom(1)) + if err != nil { + t.Fatal(err) + } + + appKey := make([]byte, 32) + io.ReadFull(StupidRandom(255), appKey) + + rServer, wClient := io.Pipe() + rClient, wServer := io.Pipe() + + rwServer := rw{rServer, wServer} + rwClient := rw{rClient, wClient} + + serverState, err := NewServerState(appKey, *keySrv) + if err != nil { + t.Error("error making server state:", err) + } + + clientState, err := NewClientState(appKey, *keyClient, keySrv.Public) + if err != nil { + t.Error("error making client state:", err) + } + + // buffered channel + ch := make(chan error, 2) + + go func() { + err := Server(serverState, rwServer) + ch <- err + wServer.Close() + }() + + go func() { + err := wrongClient(clientState, rwClient) + ch <- err + wClient.Close() + }() + + // t.Error may only be called from this goroutine :/ + if err = <-ch; err != nil { + t.Error(err) + } + if err = <-ch; err != nil { + t.Error(err) + } + + if !reflect.DeepEqual(clientState.secret, serverState.secret) { + t.Error("secrets not equal") + } +} + +// wrong client is a version of Client() that messes up the client auth and expects the connection to be shut down afterwards +func wrongClient(state *State, conn io.ReadWriter) (err error) { + _, err = io.Copy(conn, bytes.NewReader(state.createChallenge())) + if err != nil { + return errors.Wrapf(err, "secrethandshake: sending challenge failed.") + } + + // recv challenge + chalResp := make([]byte, ChallengeLength) + _, err = io.ReadFull(conn, chalResp) + if err != nil { + return errors.Wrapf(err, "secrethandshake: receiving challenge failed.") + } + + // verify challenge + if !state.verifyChallenge(chalResp) { + return errors.Errorf("secrethandshake: challenge didn't verify") + } + + // prepare authentication vector + cauth := state.createClientAuth() + + // flip two bytes + n := len(cauth) - 1 + i := rand.Intn(n) + if i == n { + i = n - 1 + } + cauth[i], cauth[n] = cauth[n], cauth[i] + + _, err = io.Copy(conn, bytes.NewReader(cauth)) + if err != nil { + return errors.Wrapf(err, "secrethandshake: sending client auth failed.") + } + + // recv authentication vector? shouldn't get it + boxedSig := make([]byte, ServerAuthLength) + _, err = io.ReadFull(conn, boxedSig) + if err != io.ErrUnexpectedEOF { + return errors.Errorf("wrongClient: expected unepexcted EOF, got %s", err) + } + + return nil +} diff --git a/secrethandshake/state.go b/secrethandshake/state.go index 74ace51..bb80e7b 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -197,6 +197,9 @@ func (s *State) verifyClientAuth(data []byte) bool { ) s.hello, openOk = box.OpenAfterPrecomputation(s.hello, data, &nonce, &s.secret2) + if !openOk && s.hello == nil { + fmt.Println("warning: nil hello") + } // subtle API requires an int containing 0 or 1, we only have bool. // we can't branch because openOk is secret. okInt := int(*((*byte)(unsafe.Pointer(&openOk)))) From 60fae77e540b4a22e6c425004ca836bbfa14389c Mon Sep 17 00:00:00 2001 From: Henry Date: Fri, 16 Aug 2019 09:42:05 +0200 Subject: [PATCH 3/6] missing import and gofmt --- secrethandshake/state.go | 1 + 1 file changed, 1 insertion(+) diff --git a/secrethandshake/state.go b/secrethandshake/state.go index bb80e7b..c8d8be2 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -11,6 +11,7 @@ package secrethandshake import ( "bytes" + "fmt" "unsafe" // ☢ ☣ "crypto/hmac" From cd073173d62d8c3a22a784ea931c26bb51921ad4 Mon Sep 17 00:00:00 2001 From: keks Date: Sun, 26 Jan 2020 11:55:32 +0100 Subject: [PATCH 4/6] progress in fixing the issue --- secrethandshake/state.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/secrethandshake/state.go b/secrethandshake/state.go index c8d8be2..3833db3 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -188,23 +188,28 @@ func (s *State) verifyClientAuth(data []byte) bool { secHasher.Write(s.aBob[:]) copy(s.secret2[:], secHasher.Sum(nil)) - s.hello = make([]byte, 0, len(data)-16) + s.hello = make([]byte, len(data)-16) var ( nonce [24]byte // always 0? openOk bool sig [ed25519.SignatureSize]byte public [ed25519.PublicKeySize]byte + hello = make([]byte, 0, len(data)-16) ) - s.hello, openOk = box.OpenAfterPrecomputation(s.hello, data, &nonce, &s.secret2) - if !openOk && s.hello == nil { + hello, openOk = box.OpenAfterPrecomputation(hello, data, &nonce, &s.secret2) + if !openOk && hello == nil { fmt.Println("warning: nil hello") } + // subtle API requires an int containing 0 or 1, we only have bool. // we can't branch because openOk is secret. okInt := int(*((*byte)(unsafe.Pointer(&openOk)))) + // this is not super secret data like keys, so we can copy it around + copy(s.hello, hello) + subtle.ConstantTimeCopy(okInt, sig[:], s.hello[:ed25519.SignatureSize]) subtle.ConstantTimeCopy(okInt, public[:], s.hello[ed25519.SignatureSize:ed25519.SignatureSize+ed25519.PublicKeySize]) From 120f00ce6f330a8b4cc3acb64aeab448df5df0c2 Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 29 Jan 2020 20:51:30 +0100 Subject: [PATCH 5/6] no extra hello --- secrethandshake/state.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/secrethandshake/state.go b/secrethandshake/state.go index 3833db3..4ba2846 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -11,7 +11,6 @@ package secrethandshake import ( "bytes" - "fmt" "unsafe" // ☢ ☣ "crypto/hmac" @@ -192,24 +191,19 @@ func (s *State) verifyClientAuth(data []byte) bool { var ( nonce [24]byte // always 0? - openOk bool sig [ed25519.SignatureSize]byte public [ed25519.PublicKeySize]byte - hello = make([]byte, 0, len(data)-16) ) - hello, openOk = box.OpenAfterPrecomputation(hello, data, &nonce, &s.secret2) - if !openOk && hello == nil { - fmt.Println("warning: nil hello") - } + _, openOk := box.OpenAfterPrecomputation(s.hello[:0], data, &nonce, &s.secret2) + // if !openOk && hello == nil { + // fmt.Println("warning: nil hello") + // } // subtle API requires an int containing 0 or 1, we only have bool. // we can't branch because openOk is secret. okInt := int(*((*byte)(unsafe.Pointer(&openOk)))) - // this is not super secret data like keys, so we can copy it around - copy(s.hello, hello) - subtle.ConstantTimeCopy(okInt, sig[:], s.hello[:ed25519.SignatureSize]) subtle.ConstantTimeCopy(okInt, public[:], s.hello[ed25519.SignatureSize:ed25519.SignatureSize+ed25519.PublicKeySize]) From 2be863a3fe71ed9dc4696d348db94ebf000797f8 Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 29 Jan 2020 21:40:32 +0100 Subject: [PATCH 6/6] document castBoolToInt --- secrethandshake/conn_test.go | 15 ++++++++++----- secrethandshake/state.go | 22 ++++++++++----------- secrethandshake/unsafecast.go | 31 ++++++++++++++++++++++++++++++ secrethandshake/unsafecast_test.go | 11 +++++++++++ 4 files changed, 63 insertions(+), 16 deletions(-) create mode 100644 secrethandshake/unsafecast.go create mode 100644 secrethandshake/unsafecast_test.go diff --git a/secrethandshake/conn_test.go b/secrethandshake/conn_test.go index dcb9db0..61b57f0 100644 --- a/secrethandshake/conn_test.go +++ b/secrethandshake/conn_test.go @@ -130,13 +130,18 @@ func TestAuthWrong(t *testing.T) { go func() { err := Server(serverState, rwServer) - ch <- err + expErr := ErrProtocol{1} + if err != expErr { + ch <- errors.Wrap(err, "server failed differntly then expected") + } else { + ch <- nil + } wServer.Close() }() go func() { err := wrongClient(clientState, rwClient) - ch <- err + ch <- errors.Wrap(err, "client failed") wClient.Close() }() @@ -190,9 +195,9 @@ func wrongClient(state *State, conn io.ReadWriter) (err error) { // recv authentication vector? shouldn't get it boxedSig := make([]byte, ServerAuthLength) - _, err = io.ReadFull(conn, boxedSig) - if err != io.ErrUnexpectedEOF { - return errors.Errorf("wrongClient: expected unepexcted EOF, got %s", err) + n, err = io.ReadFull(conn, boxedSig) + if err != io.EOF || n != 0 { + return errors.Errorf("wrongClient: expected unepexcted EOF, got %s %d", err, n) } return nil diff --git a/secrethandshake/state.go b/secrethandshake/state.go index 4ba2846..b84cb7d 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -11,8 +11,6 @@ package secrethandshake import ( "bytes" - "unsafe" // ☢ ☣ - "crypto/hmac" "crypto/rand" "crypto/sha256" @@ -28,6 +26,13 @@ import ( "golang.org/x/crypto/nacl/box" ) +func init() { + err := testMemoryLayoutAssumption() + if err != nil { + panic(err) + } +} + // State is the state each peer holds during the handshake type State struct { appKey, secHash []byte @@ -167,8 +172,6 @@ func (s *State) createClientAuth() []byte { return out } -var nullHello [ed25519.SignatureSize + ed25519.PublicKeySize]byte - // verifyClientAuth returns whether a buffer contains a valid clientAuth message func (s *State) verifyClientAuth(data []byte) bool { // this branch is okay because there are no secrets involved @@ -193,16 +196,13 @@ func (s *State) verifyClientAuth(data []byte) bool { nonce [24]byte // always 0? sig [ed25519.SignatureSize]byte public [ed25519.PublicKeySize]byte + openOk bool ) - _, openOk := box.OpenAfterPrecomputation(s.hello[:0], data, &nonce, &s.secret2) - // if !openOk && hello == nil { - // fmt.Println("warning: nil hello") - // } + _, openOk = box.OpenAfterPrecomputation(s.hello[:0], data, &nonce, &s.secret2) - // subtle API requires an int containing 0 or 1, we only have bool. - // we can't branch because openOk is secret. - okInt := int(*((*byte)(unsafe.Pointer(&openOk)))) + // see unsafecast_test.go + okInt := castBoolToInt(&openOk) subtle.ConstantTimeCopy(okInt, sig[:], s.hello[:ed25519.SignatureSize]) subtle.ConstantTimeCopy(okInt, public[:], s.hello[ed25519.SignatureSize:ed25519.SignatureSize+ed25519.PublicKeySize]) diff --git a/secrethandshake/unsafecast.go b/secrethandshake/unsafecast.go new file mode 100644 index 0000000..49227c8 --- /dev/null +++ b/secrethandshake/unsafecast.go @@ -0,0 +1,31 @@ +package secrethandshake + +import ( + "errors" + "unsafe" // ☢ ☣ +) + +// the idea here is that the memory backed by bool +// will be on byte and no other values than 0x00 and 0x01 +func castBoolToInt(bptr *bool) int { + uptr := unsafe.Pointer(bptr) + bytePtr := (*byte)(uptr) + + return int(*bytePtr) +} + +func testMemoryLayoutAssumption() error { + var boolTrue, boolFalse bool + boolTrue = true + + okTrue := castBoolToInt(&boolTrue) + if okTrue != 1 { + return errors.New("expected bool to int cast failed on true") + } + + okFalse := castBoolToInt(&boolFalse) + if okFalse != 0 { + return errors.New("expected bool to int cast failed on false") + } + return nil +} diff --git a/secrethandshake/unsafecast_test.go b/secrethandshake/unsafecast_test.go new file mode 100644 index 0000000..9b149d0 --- /dev/null +++ b/secrethandshake/unsafecast_test.go @@ -0,0 +1,11 @@ +package secrethandshake + +import ( + "testing" +) + +func TestUnsafeBoolCase(t *testing.T) { + if err := testMemoryLayoutAssumption(); err != nil { + t.Fatal(err) + } +}