From 8b7f580cbf529aa8c015d02b44d98493ace74c65 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Mon, 3 Feb 2020 10:15:20 -0500 Subject: [PATCH] don't log all errors Also, return errors when validating handshake payloads and remote peer IDs. These were being logged but not returned, so the handshake would complete successfully even if the payload signature was invalid --- p2p/security/noise/ik_handshake.go | 10 ++-------- p2p/security/noise/protocol.go | 10 ---------- p2p/security/noise/xx_handshake.go | 19 +++++-------------- 3 files changed, 7 insertions(+), 32 deletions(-) diff --git a/p2p/security/noise/ik_handshake.go b/p2p/security/noise/ik_handshake.go index 2978e957..f7859ffa 100644 --- a/p2p/security/noise/ik_handshake.go +++ b/p2p/security/noise/ik_handshake.go @@ -24,14 +24,12 @@ func (s *secureSession) ik_sendHandshakeMessage(payload []byte, initial_stage bo err := s.writeLength(len(encMsgBuf)) if err != nil { - log.Error("ik_sendHandshakeMessage initiator=%v err=%s", s.initiator, err) return fmt.Errorf("ik_sendHandshakeMessage write length err=%s", err) } // send message _, err = s.insecure.Write(encMsgBuf) if err != nil { - log.Error("ik_sendHandshakeMessage initiator=%v err=%s", s.initiator, err) return fmt.Errorf("ik_sendHandshakeMessage write to conn err=%s", err) } @@ -61,13 +59,11 @@ func (s *secureSession) ik_recvHandshakeMessage(initial_stage bool) (buf []byte, log.Debugf("ik_recvHandshakeMessage initiator=%v msgbuf=%v", s.initiator, msgbuf) if err != nil { - log.Errorf("ik_recvHandshakeMessage initiator=%v decode err=%s", s.initiator, err) return buf, nil, false, fmt.Errorf("ik_recvHandshakeMessage decode msg fail: %s", err) } s.ik_ns, plaintext, valid = ik.RecvMessage(s.ik_ns, msgbuf) if !valid { - log.Errorf("ik_recvHandshakeMessage initiator=%v err=%s", s.initiator, "validation fail") return buf, nil, false, fmt.Errorf("ik_recvHandshakeMessage validation fail") } @@ -99,7 +95,6 @@ func (s *secureSession) runHandshake_ik(ctx context.Context, payload []byte) ([] // stage 0 // err := s.ik_sendHandshakeMessage(payload, true) if err != nil { - log.Errorf("runHandshake_ik stage=0 initiator=true send err=%s", err) return nil, fmt.Errorf("runHandshake_ik stage=0 initiator=true err=%s", err) } @@ -125,20 +120,19 @@ func (s *secureSession) runHandshake_ik(ctx context.Context, payload []byte) ([] // set remote libp2p public key err = s.setRemotePeerInfo(nhp.GetIdentityKey()) if err != nil { - log.Errorf("runHandshake_ik stage=1 initiator=true set remote peer info err=%s", err) return buf, fmt.Errorf("runHandshake_ik stage=1 initiator=true err=read remote libp2p key fail") } // assert that remote peer ID matches libp2p key err = s.setRemotePeerID(s.RemotePublicKey()) if err != nil { - log.Errorf("runHandshake_ik stage=1 initiator=true set remote peer id err=%s", err) + return buf, fmt.Errorf("runHandshake_ik stage=1 initiator=true err=%s", err) } // verify payload is signed by libp2p key err = s.verifyPayload(nhp, remoteNoiseKey) if err != nil { - log.Errorf("runHandshake_ik stage=1 initiator=true verify payload err=%s", err) + return buf, fmt.Errorf("runHandshake_ik stage=1 initiator=true verify payload err=%s", err) } } else { diff --git a/p2p/security/noise/protocol.go b/p2p/security/noise/protocol.go index 628d0cfe..78c24b66 100644 --- a/p2p/security/noise/protocol.go +++ b/p2p/security/noise/protocol.go @@ -170,7 +170,6 @@ func (s *secureSession) runHandshake(ctx context.Context) error { noise_pub := s.noiseKeypair.publicKey signedPayload, err := s.localKey.Sign(append([]byte(payload_string), noise_pub[:]...)) if err != nil { - log.Errorf("runHandshake signing payload err=%s", err) return fmt.Errorf("runHandshake signing payload err=%s", err) } @@ -180,7 +179,6 @@ func (s *secureSession) runHandshake(ctx context.Context) error { payload.IdentitySig = signedPayload payloadEnc, err := proto.Marshal(payload) if err != nil { - log.Errorf("runHandshake marshal payload err=%s", err) return fmt.Errorf("runHandshake proto marshal payload err=%s", err) } @@ -195,12 +193,9 @@ func (s *secureSession) runHandshake(ctx context.Context) error { // we're either a responder or an initiator with a known static key for the remote peer, try IK buf, err := s.runHandshake_ik(ctx, payloadEnc) if err != nil { - log.Error("runHandshake ik err=%s", err) - // IK failed, pipe to XXfallback err = s.runHandshake_xx(ctx, true, payloadEnc, buf) if err != nil { - log.Error("runHandshake xx err=err", err) return fmt.Errorf("runHandshake xx err=%s", err) } @@ -212,7 +207,6 @@ func (s *secureSession) runHandshake(ctx context.Context) error { // unknown static key for peer, try XX err := s.runHandshake_xx(ctx, false, payloadEnc, nil) if err != nil { - log.Error("runHandshake xx err=%s", err) return err } @@ -262,13 +256,11 @@ func (s *secureSession) Read(buf []byte) (int, error) { ciphertext := make([]byte, l) _, err = io.ReadFull(s.insecure, ciphertext) if err != nil { - log.Error("read ciphertext err", err) return 0, err } plaintext, err := s.Decrypt(ciphertext) if err != nil { - log.Error("decrypt err", err) return 0, err } @@ -328,13 +320,11 @@ func (s *secureSession) Write(in []byte) (int, error) { writeChunk := func(in []byte) (int, error) { ciphertext, err := s.Encrypt(in) if err != nil { - log.Error("encrypt error", err) return 0, err } err = s.writeLength(len(ciphertext)) if err != nil { - log.Error("write length err: ", err) return 0, err } diff --git a/p2p/security/noise/xx_handshake.go b/p2p/security/noise/xx_handshake.go index a0a32bdd..dccbb33d 100644 --- a/p2p/security/noise/xx_handshake.go +++ b/p2p/security/noise/xx_handshake.go @@ -24,13 +24,11 @@ func (s *secureSession) xx_sendHandshakeMessage(payload []byte, initial_stage bo err := s.writeLength(len(encMsgBuf)) if err != nil { - log.Error("xx_sendHandshakeMessage initiator=%v err=%s", s.initiator, err) return fmt.Errorf("xx_sendHandshakeMessage write length err=%s", err) } _, err = s.insecure.Write(encMsgBuf) if err != nil { - log.Error("xx_sendHandshakeMessage initiator=%v err=%s", s.initiator, err) return fmt.Errorf("xx_sendHandshakeMessage write to conn err=%s", err) } @@ -66,7 +64,6 @@ func (s *secureSession) xx_recvHandshakeMessage(initial_stage bool) (buf []byte, s.xx_ns, plaintext, valid = xx.RecvMessage(s.xx_ns, msgbuf) if !valid { - log.Error("xx_recvHandshakeMessage initiator=%v err=validation fail", s.initiator) return buf, nil, false, fmt.Errorf("xx_recvHandshakeMessage validation fail") } @@ -137,7 +134,6 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl s.xx_ns, plaintext, valid = xx.RecvMessage(s.xx_ns, msgbuf) if !valid { - log.Errorf("xx_recvHandshakeMessage initiator=%v", s.initiator, "error", "validation fail") return fmt.Errorf("runHandshake_xx validation fail") } @@ -160,22 +156,21 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl // set remote libp2p public key err = s.setRemotePeerInfo(nhp.GetIdentityKey()) if err != nil { - log.Errorf("runHandshake_xx stage=2 initiator=true set remote peer info err=%s", err) return fmt.Errorf("runHandshake_xx stage=2 initiator=true read remote libp2p key fail") } // assert that remote peer ID matches libp2p public key pid, err := peer.IDFromPublicKey(s.RemotePublicKey()) if pid != s.remotePeer { - log.Errorf("runHandshake_xx stage=2 initiator=true check remote peer id err: expected %x got %x", s.remotePeer, pid) + return fmt.Errorf("runHandshake_xx stage=2 initiator=true check remote peer id err: expected %x got %x", s.remotePeer, pid) } else if err != nil { - log.Errorf("runHandshake_xx stage 2 initiator check remote peer id err %s", err) + return fmt.Errorf("runHandshake_xx stage 2 initiator check remote peer id err %s", err) } // verify payload is signed by libp2p key err = s.verifyPayload(nhp, s.xx_ns.RemoteKey()) if err != nil { - log.Errorf("runHandshake_xx stage=2 initiator=true verify payload err=%s", err) + return fmt.Errorf("runHandshake_xx stage=2 initiator=true verify payload err=%s", err) } if s.noisePipesSupport { @@ -205,7 +200,6 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl var msgbuf *xx.MessageBuffer msgbuf, err = xx.Decode0(initialMsg) if err != nil { - log.Errorf("runHandshake_xx recv msg err", err) return err } @@ -214,7 +208,6 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl s.xx_ns, plaintext, valid = xx.RecvMessage(s.xx_ns, &xx_msgbuf) if !valid { - log.Errorf("runHandshake_xx initiator=false recv msg err=%s", "validation fail") return fmt.Errorf("runHandshake_xx validation fail") } } @@ -249,14 +242,13 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl // set remote libp2p public key err = s.setRemotePeerInfo(nhp.GetIdentityKey()) if err != nil { - log.Errorf("runHandshake_xx stage=2 initiator=false set remote peer info err=%s", err) return fmt.Errorf("runHandshake_xx stage=2 initiator=false read remote libp2p key fail") } - // assert that remote peer ID matches libp2p key + // set remote libp2p public key from payload err = s.setRemotePeerID(s.RemotePublicKey()) if err != nil { - log.Errorf("runHandshake_xx stage=2 initiator=false set remote peer id err=%s", err) + return fmt.Errorf("runHandshake_xx stage=2 initiator=false set remote peer id err=%s", err) } s.remote.noiseKey = s.xx_ns.RemoteKey() @@ -264,7 +256,6 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl // verify payload is signed by libp2p key err = s.verifyPayload(nhp, s.remote.noiseKey) if err != nil { - log.Errorf("runHandshake_xx stage=2 initiator=false verify payload err=%s", err) return fmt.Errorf("runHandshake_xx stage=2 initiator=false err=%s", err) }