diff --git a/secrethandshake/conn_test.go b/secrethandshake/conn_test.go index 060121d..61b57f0 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,113 @@ 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) + 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 <- errors.Wrap(err, "client failed") + 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) + 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 e4d1da7..b84cb7d 100644 --- a/secrethandshake/state.go +++ b/secrethandshake/state.go @@ -11,11 +11,11 @@ package secrethandshake import ( "bytes" - "crypto/hmac" "crypto/rand" "crypto/sha256" "crypto/sha512" + "crypto/subtle" "go.cryptoscope.co/secretstream/internal/lo25519" @@ -26,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 @@ -165,10 +172,13 @@ 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 + 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) @@ -180,35 +190,22 @@ 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? - var openOk bool - s.hello, openOk = box.OpenAfterPrecomputation(s.hello, data, &nonce, &s.secret2) + var ( + nonce [24]byte // always 0? + sig [ed25519.SignatureSize]byte + public [ed25519.PublicKeySize]byte + openOk bool + ) - 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:]) - } + _, openOk = box.OpenAfterPrecomputation(s.hello[:0], data, &nonce, &s.secret2) - if lo25519.IsEdLowOrder(sig[:32]) { - openOk = false - } + // 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]) var sigMsg bytes.Buffer sigMsg.Write(s.appKey) @@ -217,7 +214,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 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) + } +}