From ad4e4ce6b8755ccc641a246bb8fda7ee4ecdc291 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 17 Jul 2015 15:47:27 +0100 Subject: [PATCH 1/4] refactor: make 'inner' encoder/decoder explicit --- router/crypto.go | 24 ++++++++++-------------- router/protocol.go | 26 ++++++++++++-------------- 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index 3f172caf..095c344b 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -337,9 +337,9 @@ type SimpleTCPSender struct { } type EncryptedTCPSender struct { - SimpleTCPSender sync.RWMutex - state *TCPCryptoState + sender TCPSender + state *TCPCryptoState } func NewSimpleTCPSender(encoder *gob.Encoder) *SimpleTCPSender { @@ -350,10 +350,8 @@ func (sender *SimpleTCPSender) Send(msg []byte) error { return sender.encoder.Encode(msg) } -func NewEncryptedTCPSender(encoder *gob.Encoder, sessionKey *[32]byte, outbound bool) *EncryptedTCPSender { - return &EncryptedTCPSender{ - SimpleTCPSender: *NewSimpleTCPSender(encoder), - state: NewTCPCryptoState(sessionKey, outbound)} +func NewEncryptedTCPSender(sender TCPSender, sessionKey *[32]byte, outbound bool) *EncryptedTCPSender { + return &EncryptedTCPSender{sender: sender, state: NewTCPCryptoState(sessionKey, outbound)} } func (sender *EncryptedTCPSender) Send(msg []byte) error { @@ -361,7 +359,7 @@ func (sender *EncryptedTCPSender) Send(msg []byte) error { defer sender.Unlock() encodedMsg := secretbox.Seal(nil, msg, &sender.state.nonce, sender.state.sessionKey) sender.state.advance() - return sender.SimpleTCPSender.Send(encodedMsg) + return sender.sender.Send(encodedMsg) } type TCPReceiver interface { @@ -373,8 +371,8 @@ type SimpleTCPReceiver struct { } type EncryptedTCPReceiver struct { - SimpleTCPReceiver - state *TCPCryptoState + receiver TCPReceiver + state *TCPCryptoState } func NewSimpleTCPReceiver(decoder *gob.Decoder) *SimpleTCPReceiver { @@ -387,14 +385,12 @@ func (receiver *SimpleTCPReceiver) Receive() ([]byte, error) { return msg, err } -func NewEncryptedTCPReceiver(decoder *gob.Decoder, sessionKey *[32]byte, outbound bool) *EncryptedTCPReceiver { - return &EncryptedTCPReceiver{ - SimpleTCPReceiver: *NewSimpleTCPReceiver(decoder), - state: NewTCPCryptoState(sessionKey, !outbound)} +func NewEncryptedTCPReceiver(receiver TCPReceiver, sessionKey *[32]byte, outbound bool) *EncryptedTCPReceiver { + return &EncryptedTCPReceiver{receiver: receiver, state: NewTCPCryptoState(sessionKey, !outbound)} } func (receiver *EncryptedTCPReceiver) Receive() ([]byte, error) { - msg, err := receiver.SimpleTCPReceiver.Receive() + msg, err := receiver.receiver.Receive() if err != nil { return nil, err } diff --git a/router/protocol.go b/router/protocol.go index 0b0bab2f..65f25089 100644 --- a/router/protocol.go +++ b/router/protocol.go @@ -180,12 +180,13 @@ func (res *ProtocolIntroResults) doIntroV1(params ProtocolIntroParams, pubKey, p return err } + res.Sender = NewSimpleTCPSender(enc) + res.Receiver = NewSimpleTCPReceiver(dec) + if pubKey == nil { if _, present := res.Features["PublicKey"]; present { return ErrExpectedNoCrypto } - - res.setupNoCrypto(enc, dec) } else { remotePubKeyStr, ok := res.Features["PublicKey"] if !ok { @@ -197,7 +198,7 @@ func (res *ProtocolIntroResults) doIntroV1(params ProtocolIntroParams, pubKey, p return err } - res.setupCrypto(params, enc, dec, remotePubKey, privKey) + res.setupCrypto(params, remotePubKey, privKey) } res.Features = filterV1Features(res.Features) @@ -265,7 +266,8 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return ErrExpectedCrypto } - res.setupNoCrypto(gob.NewEncoder(params.Conn), gob.NewDecoder(params.Conn)) + res.Sender = NewSimpleTCPSender(gob.NewEncoder(params.Conn)) + res.Receiver = NewSimpleTCPReceiver(gob.NewDecoder(params.Conn)) case 1: if pubKey == nil { @@ -277,7 +279,9 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return err } - res.setupCrypto(params, gob.NewEncoder(params.Conn), gob.NewDecoder(params.Conn), rbuf, privKey) + res.Sender = NewSimpleTCPSender(gob.NewEncoder(params.Conn)) + res.Receiver = NewSimpleTCPReceiver(gob.NewDecoder(params.Conn)) + res.setupCrypto(params, rbuf, privKey) default: return fmt.Errorf("Bad encryption flag %d", rbuf[0]) @@ -314,18 +318,12 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return nil } -func (res *ProtocolIntroResults) setupNoCrypto(enc *gob.Encoder, dec *gob.Decoder) { - res.Sender = NewSimpleTCPSender(enc) - res.Receiver = NewSimpleTCPReceiver(dec) -} - -func (res *ProtocolIntroResults) setupCrypto(params ProtocolIntroParams, - enc *gob.Encoder, dec *gob.Decoder, remotePubKey []byte, privKey *[32]byte) { +func (res *ProtocolIntroResults) setupCrypto(params ProtocolIntroParams, remotePubKey []byte, privKey *[32]byte) { var remotePubKeyArr [32]byte copy(remotePubKeyArr[:], remotePubKey) res.SessionKey = FormSessionKey(&remotePubKeyArr, privKey, params.Password) - res.Sender = NewEncryptedTCPSender(enc, res.SessionKey, params.Outbound) - res.Receiver = NewEncryptedTCPReceiver(dec, res.SessionKey, params.Outbound) + res.Sender = NewEncryptedTCPSender(res.Sender, res.SessionKey, params.Outbound) + res.Receiver = NewEncryptedTCPReceiver(res.Receiver, res.SessionKey, params.Outbound) } type ProtocolTag byte From af049efb402a1df3ef7a16b6e2898a9d9defd831 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 17 Jul 2015 15:52:22 +0100 Subject: [PATCH 2/4] refactor: s/SimpleTCP{Sender,Receiver}/GobTCP{Sender,Receiver} --- router/crypto.go | 16 ++++++++-------- router/protocol.go | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index 095c344b..0ca7f846 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -332,7 +332,7 @@ type TCPSender interface { Send([]byte) error } -type SimpleTCPSender struct { +type GobTCPSender struct { encoder *gob.Encoder } @@ -342,11 +342,11 @@ type EncryptedTCPSender struct { state *TCPCryptoState } -func NewSimpleTCPSender(encoder *gob.Encoder) *SimpleTCPSender { - return &SimpleTCPSender{encoder: encoder} +func NewGobTCPSender(encoder *gob.Encoder) *GobTCPSender { + return &GobTCPSender{encoder: encoder} } -func (sender *SimpleTCPSender) Send(msg []byte) error { +func (sender *GobTCPSender) Send(msg []byte) error { return sender.encoder.Encode(msg) } @@ -366,7 +366,7 @@ type TCPReceiver interface { Receive() ([]byte, error) } -type SimpleTCPReceiver struct { +type GobTCPReceiver struct { decoder *gob.Decoder } @@ -375,11 +375,11 @@ type EncryptedTCPReceiver struct { state *TCPCryptoState } -func NewSimpleTCPReceiver(decoder *gob.Decoder) *SimpleTCPReceiver { - return &SimpleTCPReceiver{decoder: decoder} +func NewGobTCPReceiver(decoder *gob.Decoder) *GobTCPReceiver { + return &GobTCPReceiver{decoder: decoder} } -func (receiver *SimpleTCPReceiver) Receive() ([]byte, error) { +func (receiver *GobTCPReceiver) Receive() ([]byte, error) { var msg []byte err := receiver.decoder.Decode(&msg) return msg, err diff --git a/router/protocol.go b/router/protocol.go index 65f25089..500d0d61 100644 --- a/router/protocol.go +++ b/router/protocol.go @@ -180,8 +180,8 @@ func (res *ProtocolIntroResults) doIntroV1(params ProtocolIntroParams, pubKey, p return err } - res.Sender = NewSimpleTCPSender(enc) - res.Receiver = NewSimpleTCPReceiver(dec) + res.Sender = NewGobTCPSender(enc) + res.Receiver = NewGobTCPReceiver(dec) if pubKey == nil { if _, present := res.Features["PublicKey"]; present { @@ -266,8 +266,8 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return ErrExpectedCrypto } - res.Sender = NewSimpleTCPSender(gob.NewEncoder(params.Conn)) - res.Receiver = NewSimpleTCPReceiver(gob.NewDecoder(params.Conn)) + res.Sender = NewGobTCPSender(gob.NewEncoder(params.Conn)) + res.Receiver = NewGobTCPReceiver(gob.NewDecoder(params.Conn)) case 1: if pubKey == nil { @@ -279,8 +279,8 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return err } - res.Sender = NewSimpleTCPSender(gob.NewEncoder(params.Conn)) - res.Receiver = NewSimpleTCPReceiver(gob.NewDecoder(params.Conn)) + res.Sender = NewGobTCPSender(gob.NewEncoder(params.Conn)) + res.Receiver = NewGobTCPReceiver(gob.NewDecoder(params.Conn)) res.setupCrypto(params, rbuf, privKey) default: From 6f8622a1a03b85a92aeef9317e4ecd5719bb5a9c Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 17 Jul 2015 16:52:50 +0100 Subject: [PATCH 3/4] replace outermost framing on tcp connection switch from gob to length-prefix encoding --- router/crypto.go | 39 +++++++++++++++++++++++++++++++++++++++ router/protocol.go | 17 ++++++++--------- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index 0ca7f846..9d9f4d34 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -6,6 +6,7 @@ import ( "encoding/binary" "encoding/gob" "fmt" + "io" "sync" "github.com/andybalholm/go-bit" @@ -336,6 +337,10 @@ type GobTCPSender struct { encoder *gob.Encoder } +type LengthPrefixTCPSender struct { + writer io.Writer +} + type EncryptedTCPSender struct { sync.RWMutex sender TCPSender @@ -350,6 +355,21 @@ func (sender *GobTCPSender) Send(msg []byte) error { return sender.encoder.Encode(msg) } +func NewLengthPrefixTCPSender(writer io.Writer) *LengthPrefixTCPSender { + return &LengthPrefixTCPSender{writer: writer} +} + +func (sender *LengthPrefixTCPSender) Send(msg []byte) error { + l := len(msg) + // We copy the message so we can send it in a single Write + // operation, thus making this thread-safe without locking. + prefixedMsg := make([]byte, 4+l) + binary.BigEndian.PutUint32(prefixedMsg, uint32(l)) + copy(prefixedMsg[4:], msg) + _, err := sender.writer.Write(prefixedMsg) + return err +} + func NewEncryptedTCPSender(sender TCPSender, sessionKey *[32]byte, outbound bool) *EncryptedTCPSender { return &EncryptedTCPSender{sender: sender, state: NewTCPCryptoState(sessionKey, outbound)} } @@ -370,6 +390,10 @@ type GobTCPReceiver struct { decoder *gob.Decoder } +type LengthPrefixTCPReceiver struct { + reader io.Reader +} + type EncryptedTCPReceiver struct { receiver TCPReceiver state *TCPCryptoState @@ -385,6 +409,21 @@ func (receiver *GobTCPReceiver) Receive() ([]byte, error) { return msg, err } +func NewLengthPrefixTCPReceiver(reader io.Reader) *LengthPrefixTCPReceiver { + return &LengthPrefixTCPReceiver{reader: reader} +} + +func (receiver *LengthPrefixTCPReceiver) Receive() ([]byte, error) { + lenPrefix := make([]byte, 4) + if _, err := io.ReadFull(receiver.reader, lenPrefix); err != nil { + return nil, err + } + l := binary.BigEndian.Uint32(lenPrefix) + msg := make([]byte, l) + _, err := io.ReadFull(receiver.reader, msg) + return msg, err +} + func NewEncryptedTCPReceiver(receiver TCPReceiver, sessionKey *[32]byte, outbound bool) *EncryptedTCPReceiver { return &EncryptedTCPReceiver{receiver: receiver, state: NewTCPCryptoState(sessionKey, !outbound)} } diff --git a/router/protocol.go b/router/protocol.go index 500d0d61..8b5252d1 100644 --- a/router/protocol.go +++ b/router/protocol.go @@ -228,12 +228,11 @@ func filterV1Features(intro map[string]string) map[string]string { // - When the connection is encrypted, 32 bytes follow containing the // public key. // -// - Then a stream of gobified values. +// - Then a stream of length-prefixed messages, which are encrypted +// for an encrypted connection. // -// The gobified values are the messages on the connection (encrypted -// for an encrypted connection). The first message contains the -// encoded features map (so in contrast to V1, it will be encrypted on -// an encrypted connection). +// The first message contains the encoded features map (so in contrast +// to V1, it will be encrypted on an encrypted connection). func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, privKey *[32]byte) error { // Public key exchange var wbuf []byte @@ -266,8 +265,8 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return ErrExpectedCrypto } - res.Sender = NewGobTCPSender(gob.NewEncoder(params.Conn)) - res.Receiver = NewGobTCPReceiver(gob.NewDecoder(params.Conn)) + res.Sender = NewLengthPrefixTCPSender(params.Conn) + res.Receiver = NewLengthPrefixTCPReceiver(params.Conn) case 1: if pubKey == nil { @@ -279,8 +278,8 @@ func (res *ProtocolIntroResults) doIntroV2(params ProtocolIntroParams, pubKey, p return err } - res.Sender = NewGobTCPSender(gob.NewEncoder(params.Conn)) - res.Receiver = NewGobTCPReceiver(gob.NewDecoder(params.Conn)) + res.Sender = NewLengthPrefixTCPSender(params.Conn) + res.Receiver = NewLengthPrefixTCPReceiver(params.Conn) res.setupCrypto(params, rbuf, privKey) default: From 0b7d462adfae4fa665524e25ddabb1d0d3166ad6 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 17 Jul 2015 17:00:58 +0100 Subject: [PATCH 4/4] enforce a message size limit so we do not allocate loads of memory when receiving (accidental or deliberate) garbage --- router/consts.go | 1 + router/crypto.go | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/router/consts.go b/router/consts.go index 1acfc18b..6591236a 100644 --- a/router/consts.go +++ b/router/consts.go @@ -25,6 +25,7 @@ const ( MaxDuration = time.Duration(math.MaxInt64) MaxMissedHeartbeats = 6 HeartbeatTimeout = MaxMissedHeartbeats * SlowHeartbeat + MaxTCPMsgSize = 10 * 1024 * 1024 ) var ( diff --git a/router/crypto.go b/router/crypto.go index 9d9f4d34..939157af 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -361,6 +361,9 @@ func NewLengthPrefixTCPSender(writer io.Writer) *LengthPrefixTCPSender { func (sender *LengthPrefixTCPSender) Send(msg []byte) error { l := len(msg) + if l > MaxTCPMsgSize { + return fmt.Errorf("outgoing message exceeds maximum size: %d > %d", l, MaxTCPMsgSize) + } // We copy the message so we can send it in a single Write // operation, thus making this thread-safe without locking. prefixedMsg := make([]byte, 4+l) @@ -419,6 +422,9 @@ func (receiver *LengthPrefixTCPReceiver) Receive() ([]byte, error) { return nil, err } l := binary.BigEndian.Uint32(lenPrefix) + if l > MaxTCPMsgSize { + return nil, fmt.Errorf("incoming message exceeds maximum size: %d > %d", l, MaxTCPMsgSize) + } msg := make([]byte, l) _, err := io.ReadFull(receiver.reader, msg) return msg, err