Security review and some comments

Added some security bugs found while adding the new functionalities.
They are here: #2507 #2508 #2509 #2510

Also added some comments and fixed some typos.

evoting/README.md

@@ -140,3 +140,17 @@ See the [README.md](evoting-admin/README.md) in that directory.
 - Paper: **Helios: Web-based Open-Audit Voting**; *Ben Adida*, 2008
 - Paper: **Decentralizing authorities into scalable strongest-link cothorities**: *Ford et. al.*, 2015
 - Paper: **Secure distributed key generation for discrete-log based cryptosystems**; *Gennaro et. al.*, 1999
+
+# Extension to more than 9 candidates
+
+## Bugs encountered
+
+The following bugs are security bugs.
+If the D-voting implementation is a rewrite of this evoting
+service, then you should definitely check that now it's done
+correctly!
+
+- `hashMap` doesn't hash all it should: https://github.com/dedis/cothority/issues/2508
+- The authentication of the user is very bogus: https://github.com/dedis/cothority/issues/2507
+- `shuffle` and `decrypt` requests sent by the leader are not trustworthy: https://github.com/dedis/cothority/issues/2509
+- Blocks created by `shuffle` and `decrypt` are signed with a useless signature https://github.com/dedis/cothority/issues/2510

evoting/lib/transaction.go

@@ -177,7 +177,7 @@ func (t *Transaction) Verify(genesis skipchain.SkipBlockID, s *skipchain.Service
 			return errors.New("alpha and beta must be non-nil")
 		}
 		if t.Ballot.Alpha.Equal(null) || t.Ballot.Beta.Equal(null) {
-			return errors.New("alpha and beta must be null points")
+			return errors.New("alpha and beta must be non-null points")
 		}
 
 		// t.User is trusted at this point, so make sure that they did not try to sneak
@@ -230,6 +230,8 @@ func (t *Transaction) Verify(genesis skipchain.SkipBlockID, s *skipchain.Service
 		if err != nil {
 			return err
 		}
+		// BUG: This signature verification doesn't prove anything usable. It should verify that the
+		// t.Mix is valid.
 		err = schnorr.Verify(cothority.Suite, proposer.Public, data, t.Mix.Signature)
 		if err != nil {
 			return err

evoting/protocol/decrypt.go

@@ -73,9 +73,9 @@ func (d *Decrypt) Start() error {
 // There are three parts now:
 //  1. Verification of state - if this fails, it's over and `Done` is called
 //  2. Create decryption block and send it to the leader - if it fails,
-//   `Done` is called
+//     `Done` is called
 //  3. Send the decryption block to the skipchain - also will have `Done`
-//   called if it fails
+//     called if it fails
 func (d *Decrypt) HandlePrompt(prompt MessagePromptDecrypt) error {
 	var mixes []*lib.Mix
 	var partials []*lib.Partial
@@ -124,6 +124,10 @@ func (d *Decrypt) HandlePrompt(prompt MessagePromptDecrypt) error {
 			if err != nil {
 				return d.SendTo(d.Root(), &TerminateDecrypt{Error: err.Error()})
 			}
+
+			// BUG: This signature only proves that at some moment, this node
+			// was here. But a malicious other node could change the data however it wishes.
+			// Or an attacking node could simply copy the signature to a new block.
 			sig, err := schnorr.Sign(cothority.Suite, d.Private(), data)
 			if err != nil {
 				return d.SendTo(d.Root(), &TerminateDecrypt{Error: err.Error()})

evoting/protocol/shuffle.go

@@ -2,6 +2,7 @@ package protocol
 
 import (
 	"errors"
+	"fmt"
 
 	"go.dedis.ch/cothority/v3"
 	"go.dedis.ch/kyber/v3/proof"
@@ -18,7 +19,7 @@ import (
 
 /*
 Each participating node creates a verifiable shuffle with a corresponding proof
-of the last last block in election skipchain. This block is either a box of
+of the last block in the election skipchain. This block is either a box of
 encrypted ballot (in case of the root node) or a mix of the previous node. Every
 newly created shuffle is appended to the chain before the next node is prompted
 to create its shuffle. The leaf node notifies the root upon storing its shuffle,
@@ -70,9 +71,9 @@ func (s *Shuffle) Start() error {
 // LG: rewrote the function to correctly call Done - probably should be
 // rewritten even further. There are three `func() error` now with different
 // error-handling. In case of error:
-//   1. we abort and stop processing
-//   2. will return the error, but first call 3.
-//   3. try to call it and abort if error found
+//  1. we abort and stop processing
+//  2. will return the error, but first call 3.
+//  3. try to call it and abort if error found
 func (s *Shuffle) HandlePrompt(prompt MessagePrompt) error {
 	added := 0
 	var mixes []*lib.Mix
@@ -102,7 +103,7 @@ func (s *Shuffle) HandlePrompt(prompt MessagePrompt) error {
 
 		if len(ballots) < 2 {
 			if err := s.SendTo(s.Root(), &TerminateShuffle{
-				Error: "shuffle error: not enough (> 2) ballots to shuffle",
+				Error: "shuffle error: not enough (< 2) ballots to shuffle",
 			}); err != nil {
 				log.Error(err)
 			}
@@ -144,13 +145,18 @@ func (s *Shuffle) HandlePrompt(prompt MessagePrompt) error {
 		g, d, prov := shuffle.Shuffle(cothority.Suite, nil, s.Election.Key, a, b, random.New())
 		proof, err := proof.HashProve(cothority.Suite, "", prov)
 		if err != nil {
-			return err
+			return fmt.Errorf("while shuffling votes: %v", err)
 		}
+
 		mix = &lib.Mix{
 			Ballots: lib.Combine(g, d),
 			Proof:   proof,
 			NodeID:  s.ServerIdentity().ID,
 		}
+
+		// BUG: This signature only proves that at some moment, this node
+		// was here. But a malicious other node could change the data however it wishes.
+		// Or an attacking node could simply copy the signature to a new block.
 		data, err := s.ServerIdentity().Public.MarshalBinary()
 		if err != nil {
 			return err

evoting/service/service.go

@@ -135,7 +135,7 @@ func (s *Service) Link(req *evoting.Link) (*evoting.LinkReply, error) {
 	return &evoting.LinkReply{ID: id}, nil
 }
 
-// Open message hander. Create a new election with accompanying skipchain.
+// Open message handler. Create a new election with accompanying skipchain.
 func (s *Service) Open(req *evoting.Open) (*evoting.OpenReply, error) {
 	master, err := lib.GetMaster(s.skipchain, req.ID)
 	if err != nil {
@@ -367,6 +367,8 @@ func (s *Service) LookupSciper(req *evoting.LookupSciper) (*evoting.LookupSciper
 	return reply, nil
 }
 
+// SECURITY BUG: this authentication is completely bogus and can be replayed by an attacker.
+// See https://github.com/dedis/cothority/issues/2507
 func auth(u uint32, sig []byte, master skipchain.SkipBlockID, pub kyber.Point) error {
 	var message []byte
 	message = append(message, master...)

evoting/service/service_test.go

@@ -56,6 +56,7 @@ var (
 	bufCand2 = []byte{byte(idCand2 & 0xff), byte((idCand2 >> 8) & 0xff), byte((idCand2 >> 16) & 0xff)}
 )
 
+// Tests all parts of the voting procedure, from setting up a node to voting, shuffling, and decryption.
 func TestService(t *testing.T) {
 	local := onet.NewLocalTest(cothority.Suite)
 	defer local.CloseAll()
@@ -308,7 +309,7 @@ func TestService(t *testing.T) {
 }
 
 // This is an end-to end test, just like TestService, so it has a lot of copy-paste
-// stuff. It is useful to have this as it's own test because I wanted to investigate
+// stuff. It is useful to have this as its own test because I wanted to investigate
 // the behaviour of decryption of the badly encrypted points.
 func TestBadEncryption(t *testing.T) {
 	local := onet.NewLocalTest(cothority.Suite)
@@ -433,6 +434,7 @@ func TestBadEncryption(t *testing.T) {
 	}
 }
 
+// Test what happens if a vote is cast once the election is finished.
 func TestAfterEnd(t *testing.T) {
 	local := onet.NewLocalTest(cothority.Suite)
 	defer local.CloseAll()
@@ -506,6 +508,7 @@ func TestAfterEnd(t *testing.T) {
 	require.Error(t, err)
 }
 
+// Test what happens if a vote is cast before the election is started.
 func TestBeforeStart(t *testing.T) {
 	local := onet.NewLocalTest(cothority.Suite)
 	defer local.CloseAll()
@@ -649,6 +652,7 @@ func runAnElection(t *testing.T, local *onet.LocalTest, s *Service, replyLink *e
 	require.Nil(t, local.WaitDone(time.Second))
 }
 
+// Test if we can evolve the roster to modify the list of nodes.
 func TestEvolveRoster(t *testing.T) {
 	if testing.Short() {
 		t.Skip("not using evolveRoster in travis")
@@ -759,6 +763,7 @@ func setupElection(t *testing.T, s0 *Service, rl *evoting.LinkReply, nodeKP *key
 	return replyOpen.ID
 }
 
+// Make sure shuffling is still possible with two failing nodes out of 7.
 func TestShuffleBenignNodeFailure(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping", t.Name(), " in short mode")
@@ -800,6 +805,7 @@ func TestShuffleBenignNodeFailure(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Assert that shuffling is not possible with 3 out of 7 nodes failing.
 func TestShuffleCatastrophicNodeFailure(t *testing.T) {
 	if testing.Short() {
 		t.Skip("not using ShuffleCatastrophic in travis")
@@ -896,6 +902,7 @@ func TestShuffleCatastrophicNodeFailure(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Assert decryption can happen with 2 out of 7 nodes failing.
 func TestDecryptBenignNodeFailure(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping", t.Name(), " in short mode")
@@ -945,6 +952,7 @@ func TestDecryptBenignNodeFailure(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Assert decryption fails with 3 out of 7 nodes failing.
 func TestDecryptCatastrophicNodeFailure(t *testing.T) {
 	if testing.Short() {
 		t.Skip("not using DecryptCatastrophic in travis")
@@ -1017,6 +1025,7 @@ func TestDecryptCatastrophicNodeFailure(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Assert that a momentarily offline node during election can join again for shuffling.
 func TestCastNodeFailureShuffleAllOk(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping", t.Name(), " in short mode")
@@ -1080,6 +1089,7 @@ func TestCastNodeFailureShuffleAllOk(t *testing.T) {
 	nodes[5].Pause()
 	log.Lvl1("Voting with one node paused")
 	vote(idUser2, bufCand1)
+
 	log.Lvl1("Unpausing node again")
 	nodes[5].Unpause()
 
@@ -1095,6 +1105,7 @@ func TestCastNodeFailureShuffleAllOk(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Assert that the LookupSciper method works correctly.
 func TestLookupSciper(t *testing.T) {
 	// Comment this out when you want to run this unit test for dev work.
 	t.Skip("unit tests should not call external servers")

external/Makefile

@@ -15,9 +15,14 @@ do-js: check_protoc
 
 
 check_protoc:
-	@which protoc || (echo "Install protoc version 3.17.3. On Mac, run \"brew install [email protected]\"" && exit 1)
-	@pv=`protoc --version`; \
+	which protoc || $(MAKE) protoc_instructions
+	pv=`protoc --version`; \
 	if [ "$$pv" != "libprotoc 3.17.3" ]; then \
 		echo "Protoc version $$pv is not supported."; \
-		exit 1; \
+		$(MAKE) protoc_instructions; \
 	fi
+
+protoc_instructions:
+	echo "Please download version 3.17.3 from here:"
+	echo "https://github.com/protocolbuffers/protobuf/releases/tag/v3.17.3"
+	exit 1