[FAB-3450] Prevent panic on msg signing

Currently, the gossip code panics if it's unable to sign a message
or to marshal it bytes, because the assumption was
that if the signing failed then something is terrible wrong with the
peer and it's not recoverable
If turning a message into bytes fails, it's because of something
not recoverable too.

However, a user posted a stack trace in which gossip tried constructing
a message that its envelope has a payload of 2GB.

Now, the cause for a creation of such a message was fixed but in theory
we may somehow have a situation where a large message is constructed.

In order to prevent these kind of problems, and the crash of the peer,
we need to make the Signer return an error instead of panicking.

Change-Id: Ife8f4e137c92c167dadc7bbe1bcdf45f93b2b38e
Signed-off-by: Yacov Manevich <[email protected]>

Author: yacovm

gossip/comm/comm_impl.go

@@ -433,7 +433,10 @@ func (c *commImpl) authenticateRemotePeer(stream stream) (*proto.ConnectionInfo,
 		return nil, errors.New("No TLS certificate")
 	}
 
-	cMsg = c.createConnectionMsg(c.PKIID, c.selfCertHash, c.peerIdentity, signer)
+	cMsg, err = c.createConnectionMsg(c.PKIID, c.selfCertHash, c.peerIdentity, signer)
+	if err != nil {
+		return nil, err
+	}
 
 	c.logger.Debug("Sending", cMsg, "to", remoteAddress)
 	stream.Send(cMsg.Envelope)
@@ -580,7 +583,7 @@ func readWithTimeout(stream interface{}, timeout time.Duration, address string)
 	}
 }
 
-func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte, cert api.PeerIdentityType, signer proto.Signer) *proto.SignedGossipMessage {
+func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte, cert api.PeerIdentityType, signer proto.Signer) (*proto.SignedGossipMessage, error) {
 	m := &proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: 0,
@@ -595,8 +598,8 @@ func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte,
 	sMsg := &proto.SignedGossipMessage{
 		GossipMessage: m,
 	}
-	sMsg.Sign(signer)
-	return sMsg
+	_, err := sMsg.Sign(signer)
+	return sMsg, err
 }
 
 type stream interface {

gossip/comm/comm_test.go

@@ -156,7 +156,7 @@ func handshaker(endpoint string, comm Comm, t *testing.T, connMutator msgMutator
 
 	pkiID := common.PKIidType(endpoint)
 	assert.NoError(t, err, "%v", err)
-	msg := c.createConnectionMsg(pkiID, clientCertHash, []byte(endpoint), func(msg []byte) ([]byte, error) {
+	msg, _ := c.createConnectionMsg(pkiID, clientCertHash, []byte(endpoint), func(msg []byte) ([]byte, error) {
 		mac := hmac.New(sha256.New, hmacKey)
 		mac.Write(msg)
 		return mac.Sum(nil), nil
@@ -423,7 +423,7 @@ func TestCloseConn(t *testing.T) {
 	assert.NoError(t, err, "%v", err)
 	c := &commImpl{}
 	tlsCertHash := certHashFromRawCert(tlsCfg.Certificates[0].Certificate[0])
-	connMsg := c.createConnectionMsg(common.PKIidType("pkiID"), tlsCertHash, api.PeerIdentityType("pkiID"), func(msg []byte) ([]byte, error) {
+	connMsg, _ := c.createConnectionMsg(common.PKIidType("pkiID"), tlsCertHash, api.PeerIdentityType("pkiID"), func(msg []byte) ([]byte, error) {
 		mac := hmac.New(sha256.New, hmacKey)
 		mac.Write(msg)
 		return mac.Sum(nil), nil
@@ -711,13 +711,14 @@ func TestPresumedDead(t *testing.T) {
 }
 
 func createGossipMsg() *proto.SignedGossipMessage {
-	return (&proto.GossipMessage{
+	msg, _ := (&proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: uint64(rand.Int()),
 		Content: &proto.GossipMessage_DataMsg{
 			DataMsg: &proto.DataMessage{},
 		},
 	}).NoopSign()
+	return msg
 }
 
 func remotePeer(port int) *RemotePeer {

gossip/comm/mock/mock_comm.go

@@ -85,10 +85,11 @@ func NewCommMock(id string, members map[string]*socketMock) comm.Comm {
 
 // Respond sends a GossipMessage to the origin from which this ReceivedMessage was sent from
 func (packet *packetMock) Respond(msg *proto.GossipMessage) {
+	sMsg, _ := msg.NoopSign()
 	packet.src.socket <- &packetMock{
 		src: packet.dst,
 		dst: packet.src,
-		msg: msg.NoopSign(),
+		msg: sMsg,
 	}
 }
 

gossip/comm/mock/mock_comm_test.go

@@ -44,12 +44,13 @@ func TestMockComm(t *testing.T) {
 	comm2 := NewCommMock(second.endpoint, members)
 	defer comm2.Stop()
 
-	comm2.Send((&proto.GossipMessage{
+	sMsg, _ := (&proto.GossipMessage{
 		Content: &proto.GossipMessage_StateRequest{&proto.RemoteStateRequest{
 			StartSeqNum: 1,
 			EndSeqNum:   3,
 		}},
-	}).NoopSign(), &comm.RemotePeer{"first", common.PKIidType("first")})
+	}).NoopSign()
+	comm2.Send(sMsg, &comm.RemotePeer{"first", common.PKIidType("first")})
 
 	msg := <-msgCh
 
@@ -73,15 +74,16 @@ func TestMockComm_PingPong(t *testing.T) {
 	rcvChA := peerA.Accept(all)
 	rcvChB := peerB.Accept(all)
 
-	peerA.Send((&proto.GossipMessage{
+	sMsg, _ := (&proto.GossipMessage{
 		Content: &proto.GossipMessage_DataMsg{
 			&proto.DataMessage{
 				&proto.Payload{
 					SeqNum: 1,
 					Data:   []byte("Ping"),
 				},
 			}},
-	}).NoopSign(), &comm.RemotePeer{"peerB", common.PKIidType("peerB")})
+	}).NoopSign()
+	peerA.Send(sMsg, &comm.RemotePeer{"peerB", common.PKIidType("peerB")})
 
 	msg := <-rcvChB
 	dataMsg := msg.GetGossipMessage().GetDataMsg()

gossip/comm/msg.go

@@ -38,7 +38,12 @@ func (m *ReceivedMessageImpl) GetSourceEnvelope() *proto.Envelope {
 
 // Respond sends a msg to the source that sent the ReceivedMessageImpl
 func (m *ReceivedMessageImpl) Respond(msg *proto.GossipMessage) {
-	m.conn.send(msg.NoopSign(), func(e error) {})
+	sMsg, err := msg.NoopSign()
+	if err != nil {
+		m.conn.logger.Error("Failed creating SignedGossipMessage:", err)
+		return
+	}
+	m.conn.send(sMsg, func(e error) {})
 }
 
 // GetGossipMessage returns the inner GossipMessage

gossip/discovery/discovery_impl.go

@@ -172,9 +172,17 @@ func (d *gossipDiscoveryImpl) Connect(member NetworkMember, id identifier) {
 				Endpoint:         member.Endpoint,
 				PKIid:            id.ID,
 			}
-			req := d.createMembershipRequest(id.SelfOrg).NoopSign()
+			req, err := d.createMembershipRequest(id.SelfOrg).NoopSign()
+			if err != nil {
+				d.logger.Warning("Failed creating SignedGossipMessage:", err)
+				continue
+			}
 			req.Nonce = util.RandomUInt64()
-			req.NoopSign()
+			req, err = req.NoopSign()
+			if err != nil {
+				d.logger.Warning("Failed adding NONCE to SignedGossipMessage", err)
+				continue
+			}
 			go d.sendUntilAcked(peer, req)
 			return
 		}
@@ -221,19 +229,16 @@ func (d *gossipDiscoveryImpl) sendUntilAcked(peer *NetworkMember, message *proto
 	}
 }
 
-func (d *gossipDiscoveryImpl) somePeerIsKnown() bool {
-	d.lock.RLock()
-	defer d.lock.RUnlock()
-	return len(d.aliveLastTS) != 0
-}
-
 func (d *gossipDiscoveryImpl) InitiateSync(peerNum int) {
 	if d.toDie() {
 		return
 	}
 	var peers2SendTo []*NetworkMember
-	memReq := d.createMembershipRequest(true).NoopSign()
-
+	memReq, err := d.createMembershipRequest(true).NoopSign()
+	if err != nil {
+		d.logger.Warning("Failed creating SignedGossipMessage:", err)
+		return
+	}
 	d.lock.RLock()
 
 	n := d.aliveMembership.Size()
@@ -404,7 +409,11 @@ func (d *gossipDiscoveryImpl) sendMemResponse(targetMember *proto.Member, intern
 		InternalEndpoint: internalEndpoint,
 	}
 
-	memResp := d.createMembershipResponse(targetPeer)
+	aliveMsg := d.createAliveMessage(true)
+	if aliveMsg == nil {
+		return
+	}
+	memResp := d.createMembershipResponse(aliveMsg, targetPeer)
 	if memResp == nil {
 		errMsg := `Got a membership request from a peer that shouldn't have sent one: %v, closing connection to the peer as a result.`
 		d.logger.Warningf(errMsg, targetMember)
@@ -414,18 +423,22 @@ func (d *gossipDiscoveryImpl) sendMemResponse(targetMember *proto.Member, intern
 
 	defer d.logger.Debug("Exiting, replying with", memResp)
 
-	d.comm.SendToPeer(targetPeer, (&proto.GossipMessage{
+	msg, err := (&proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: nonce,
 		Content: &proto.GossipMessage_MemRes{
 			MemRes: memResp,
 		},
-	}).NoopSign())
+	}).NoopSign()
+	if err != nil {
+		d.logger.Warning("Failed creating SignedGossipMessage:", err)
+		return
+	}
+	d.comm.SendToPeer(targetPeer, msg)
 }
 
-func (d *gossipDiscoveryImpl) createMembershipResponse(targetMember *NetworkMember) *proto.MembershipResponse {
+func (d *gossipDiscoveryImpl) createMembershipResponse(aliveMsg *proto.SignedGossipMessage, targetMember *NetworkMember) *proto.MembershipResponse {
 	shouldBeDisclosed, omitConcealedFields := d.disclosurePolicy(targetMember)
-	aliveMsg := d.createAliveMessage(true)
 
 	if !shouldBeDisclosed(aliveMsg) {
 		return nil
@@ -594,24 +607,29 @@ func (d *gossipDiscoveryImpl) periodicalReconnectToDead() {
 }
 
 func (d *gossipDiscoveryImpl) sendMembershipRequest(member *NetworkMember, includeInternalEndpoint bool) {
-	d.comm.SendToPeer(member, d.createMembershipRequest(includeInternalEndpoint))
+	req, err := d.createMembershipRequest(includeInternalEndpoint).NoopSign()
+	if err != nil {
+		d.logger.Error("Failed creating SignedGossipMessage:", err)
+		return
+	}
+	d.comm.SendToPeer(member, req)
 }
 
-func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bool) *proto.SignedGossipMessage {
+func (d *gossipDiscoveryImpl) createMembershipRequest(includeInternalEndpoint bool) *proto.GossipMessage {
 	req := &proto.MembershipRequest{
 		SelfInformation: d.createAliveMessage(includeInternalEndpoint).Envelope,
 		// TODO: sending the known peers is not secure because the remote peer might shouldn't know
 		// TODO: about the known peers. I'm deprecating this until a secure mechanism will be implemented.
 		// TODO: See FAB-2570 for tracking this issue.
 		Known: [][]byte{},
 	}
-	return (&proto.GossipMessage{
+	return &proto.GossipMessage{
 		Tag:   proto.GossipMessage_EMPTY,
 		Nonce: uint64(0),
 		Content: &proto.GossipMessage_MemReq{
 			MemReq: req,
 		},
-	}).NoopSign()
+	}
 }
 
 func (d *gossipDiscoveryImpl) copyLastSeen(lastSeenMap map[string]*timestamp) []NetworkMember {
@@ -693,7 +711,11 @@ func (d *gossipDiscoveryImpl) periodicalSendAlive() {
 	for !d.toDie() {
 		d.logger.Debug("Sleeping", getAliveTimeInterval())
 		time.Sleep(getAliveTimeInterval())
-		d.comm.Gossip(d.createAliveMessage(true))
+		msg := d.createAliveMessage(true)
+		if msg == nil {
+			return
+		}
+		d.comm.Gossip(msg)
 	}
 }
 
@@ -726,9 +748,13 @@ func (d *gossipDiscoveryImpl) createAliveMessage(includeInternalEndpoint bool) *
 		},
 	}
 
+	envp := d.crypt.SignMessage(msg2Gossip, internalEndpoint)
+	if envp == nil {
+		return nil
+	}
 	signedMsg := &proto.SignedGossipMessage{
 		GossipMessage: msg2Gossip,
-		Envelope:      d.crypt.SignMessage(msg2Gossip, internalEndpoint),
+		Envelope:      envp,
 	}
 
 	if !includeInternalEndpoint {