Empty satellite signature crashing (#1007)

pkg/bwagreement/server.go

@@ -62,8 +62,9 @@ func (s *Server) BandwidthAgreements(ctx context.Context, ba *pb.RenterBandwidth
 		Status: pb.AgreementsSummary_FAIL,
 	}
 
-	if err = s.verifySignature(ctx, ba); err != nil {
+	// storagenode signature is empty
-		return reply, err
+	if len(ba.GetSignature()) == 0 {
+		return reply, BwAgreementError.New("Invalid Storage Node Signature length in the RenterBandwidthAllocation")
 	}
 
 	rbad := &pb.RenterBandwidthAllocation_Data{}
@@ -77,10 +78,19 @@ func (s *Server) BandwidthAgreements(ctx context.Context, ba *pb.RenterBandwidth
 		return reply, BwAgreementError.New("Failed to unmarshal PayerBandwidthAllocation: %+v", err)
 	}
 
+	// satellite signature is empty
+	if len(pba.GetSignature()) == 0 {
+		return reply, BwAgreementError.New("Invalid Satellite Signature length in the PayerBandwidthAllocation")
+	}
+
 	if len(pbad.SerialNumber) == 0 {
 		return reply, BwAgreementError.New("Invalid SerialNumber in the PayerBandwidthAllocation")
 	}
 
+	if err = s.verifySignature(ctx, ba); err != nil {
+		return reply, err
+	}
+
 	serialNum := pbad.GetSerialNumber() + rbad.StorageNodeId.String()
 
 	// get and check expiration
@@ -132,16 +142,25 @@ func (s *Server) verifySignature(ctx context.Context, ba *pb.RenterBandwidthAllo
 		return peertls.ErrUnsupportedKey.New("%T", pubkey)
 	}
 
+	signatureLength := k.Curve.Params().P.BitLen() / 8
+	if len(ba.GetSignature()) < signatureLength {
+		return BwAgreementError.New("Invalid Renter's Signature Length")
+	}
 	// verify Renter's (uplink) signature
 	if ok := cryptopasta.Verify(ba.GetData(), ba.GetSignature(), k); !ok {
 		return BwAgreementError.New("Failed to verify Renter's Signature")
 	}
 
+	// satellite public key
 	k, ok = s.pkey.(*ecdsa.PublicKey)
 	if !ok {
 		return peertls.ErrUnsupportedKey.New("%T", s.pkey)
 	}
 
+	signatureLength = k.Curve.Params().P.BitLen() / 8
+	if len(rbad.GetPayerAllocation().GetSignature()) < signatureLength {
+		return BwAgreementError.New("Inavalid Payer's Signature Length")
+	}
 	// verify Payer's (satellite) signature
 	if ok := cryptopasta.Verify(rbad.GetPayerAllocation().GetData(), rbad.GetPayerAllocation().GetSignature(), k); !ok {
 		return BwAgreementError.New("Failed to verify Payer's Signature")

pkg/bwagreement/test/server_test.go

@@ -79,7 +79,7 @@ func TestSameSerialNumberBandwidthAgreements(t *testing.T) {
 	})
 }
 
-func TestInvalidBandwidthAgreements(t *testing.T) {
+func TestManipulatedBandwidthAgreements(t *testing.T) {
 	satellitedbtest.Run(t, func(t *testing.T, db satellite.DB) {
 		ctx := testcontext.New(t)
 		defer ctx.Cleanup()
@@ -116,11 +116,34 @@ func TestInvalidBandwidthAgreements(t *testing.T) {
 		/* manipulate PayerBandwidthAllocation -> invalid signature */
 
 		/* self signed. Storage node sends a self signed bwagreement to get a higher payout */
+	})
+}
 
-		/* malicious storage node would like to force a crash */
+func TestInvalidBandwidthAgreements(t *testing.T) {
+	satellitedbtest.Run(t, func(t *testing.T, db satellite.DB) {
+		ctx := testcontext.New(t)
+		defer ctx.Cleanup()
 
-		/* corrupted signature. Storage node sends an corrupted signuature to force a satellite crash */
+		satellitePubKey, satellitePrivKey, uplinkPrivKey := generateKeys(ctx, t)
+		server := bwagreement.NewServer(db.BandwidthAgreement(), zap.NewNop(), satellitePubKey)
 
+		pba, err := GeneratePayerBandwidthAllocation(pb.PayerBandwidthAllocation_GET, satellitePrivKey, uplinkPrivKey, false)
+		assert.NoError(t, err)
+
+		rba, err := GenerateRenterBandwidthAllocation(pba, teststorj.NodeIDFromString("Storage node 1"), uplinkPrivKey)
+		assert.NoError(t, err)
+
+		/* Make sure the bwagreement we are using as bluleprint is valid and avoid false positives that way. */
+		reply, err := server.BandwidthAgreements(ctx, rba)
+		assert.NoError(t, err)
+		assert.Equal(t, pb.AgreementsSummary_OK, reply.Status)
+
+		/* Storage node sends an corrupted signuature to force a satellite crash */
+		rba.Signature = []byte("invalid")
+
+		reply, err = server.BandwidthAgreements(ctx, rba)
+		assert.EqualError(t, err, "bwagreement error: Invalid Renter's Signature Length")
+		assert.Equal(t, pb.AgreementsSummary_FAIL, reply.Status)
 	})
 }