From 632fa7914a2e6dbf1812581e0e769c93189771ca Mon Sep 17 00:00:00 2001 From: Jimmy Hu Date: Fri, 18 Jan 2019 19:10:55 +0800 Subject: misc: Add gosec to check security issues (#424) * Add gosec to tools * Run security check to ci * Fix secrity issues --- .circleci/config.yml | 11 +++++++++++ .gitignore | 1 + GNUmakefile | 11 ++++++++++- bin/install_tools.sh | 3 +++ cmd/dexcon-simulation-with-scheduler/main.go | 5 ++++- cmd/dexcon-simulation/main.go | 1 + common/utils.go | 9 ++++++++- core/configuration-chain.go | 10 ++++++++-- core/crypto/dkg/dkg.go | 11 +++++++++-- core/crypto/ecdsa/ecdsa.go | 4 +++- core/dkg-tsig-protocol.go | 7 ++++--- core/test/fake-transport.go | 5 ++++- core/test/governance.go | 4 +++- core/test/tcp-transport.go | 15 ++++++++++---- simulation/app.go | 2 ++ simulation/config/config.go | 2 +- simulation/node.go | 29 ++++++++++++++++------------ 17 files changed, 100 insertions(+), 30 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 72d9f53..69b0db0 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -66,6 +66,12 @@ jobs: - run: make lint - run: make vet + security: + executor: go1_11 + steps: + - init_workspace + - run: make check-security + unit_test: executor: go1_11 environment: @@ -99,11 +105,16 @@ workflows: - lint: requires: - dep + - security: + requires: + - dep - unit_test: requires: + - security - lint - integration_test: requires: + - security - lint - build: requires: diff --git a/.gitignore b/.gitignore index f8cb90c..6149f1c 100644 --- a/.gitignore +++ b/.gitignore @@ -24,4 +24,5 @@ simulation/kubernetes/*.log simulation/kubernetes/config.toml simulation/kubernetes/validator.yaml .dep +gosec.log diff --git a/GNUmakefile b/GNUmakefile index 0608cbe..64926a1 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -77,7 +77,7 @@ $(foreach component, $(COMPONENTS), $(eval $(call BUILD_RULE,$(component)))) pre-build: dep docker-dep -pre-submit: dep check-format lint test vet +pre-submit: dep check-format lint vet check-security test dep: @bin/install_eth_dep.sh @@ -98,6 +98,15 @@ lint: vet: @go vet `go list ./... | grep -v 'vendor'` +check-security: + @rm -f gosec.log + @gosec -quiet -out gosec.log ./... || true + @if [ -a gosec.log ]; then \ + cat gosec.log; \ + echo 'Error: security issue found'; \ + exit 1; \ + fi + test-short: @for pkg in `$(TEST_TARGET)`; do \ diff --git a/bin/install_tools.sh b/bin/install_tools.sh index ca0a04b..bef77f7 100755 --- a/bin/install_tools.sh +++ b/bin/install_tools.sh @@ -6,3 +6,6 @@ fi if ! which golint >/dev/null 2>&1; then go get -u golang.org/x/lint/golint fi +if ! which gosec >/dev/null 2>&1; then + go get github.com/securego/gosec/cmd/gosec/... +fi diff --git a/cmd/dexcon-simulation-with-scheduler/main.go b/cmd/dexcon-simulation-with-scheduler/main.go index 8d59825..d06e6e0 100644 --- a/cmd/dexcon-simulation-with-scheduler/main.go +++ b/cmd/dexcon-simulation-with-scheduler/main.go @@ -105,7 +105,9 @@ func main() { test.NewStopByConfirmedBlocks(blockPerNode, apps, dbs)) now := time.Now().UTC() for _, v := range nodes { - v.Bootstrap(sch, now) + if err := v.Bootstrap(sch, now); err != nil { + panic(err) + } } // Run the simulation. sch.Run(cfg.Scheduler.WorkerNum) @@ -126,6 +128,7 @@ func main() { if err := pprof.WriteHeapProfile(f); err != nil { log.Fatal("could not write memory profile: ", err) } + // #nosec G104 f.Close() } diff --git a/cmd/dexcon-simulation/main.go b/cmd/dexcon-simulation/main.go index a9a8b10..d79f60e 100644 --- a/cmd/dexcon-simulation/main.go +++ b/cmd/dexcon-simulation/main.go @@ -84,6 +84,7 @@ func main() { if err := pprof.WriteHeapProfile(f); err != nil { log.Fatal("could not write memory profile: ", err) } + // #nosec G104 f.Close() } } diff --git a/common/utils.go b/common/utils.go index 7e89c05..63d25a3 100644 --- a/common/utils.go +++ b/common/utils.go @@ -2,13 +2,20 @@ package common import ( "math/rand" + "time" ) +var random *rand.Rand + +func init() { + random = rand.New(rand.NewSource(time.Now().Unix())) +} + // NewRandomHash returns a random Hash-like value. func NewRandomHash() Hash { x := Hash{} for i := 0; i < HashLength; i++ { - x[i] = byte(rand.Int() % 256) + x[i] = byte(random.Int() % 256) } return x } diff --git a/core/configuration-chain.go b/core/configuration-chain.go index fbd691f..bec47f4 100644 --- a/core/configuration-chain.go +++ b/core/configuration-chain.go @@ -163,7 +163,10 @@ func (cc *configurationChain) runDKG(round uint64) error { return nil } // Phase 2(T = 0): Exchange DKG secret key share. - cc.dkg.processMasterPublicKeys(mpks) + if err := cc.dkg.processMasterPublicKeys(mpks); err != nil { + cc.logger.Error("Failed to process master public key", + "error", err) + } cc.mpkReady = true for _, prvShare := range cc.pendingPrvShare { if err := cc.dkg.processPrivateShare(prvShare); err != nil { @@ -184,7 +187,10 @@ func (cc *configurationChain) runDKG(round uint64) error { // Phase 5(T = 2λ): Propose Anti nack complaint. cc.logger.Debug("Calling Governance.DKGComplaints", "round", round) complaints := cc.gov.DKGComplaints(round) - cc.dkg.processNackComplaints(complaints) + if err := cc.dkg.processNackComplaints(complaints); err != nil { + cc.logger.Error("Failed to process NackComplaint", + "error", err) + } cc.dkgLock.Unlock() <-ticker.Tick() cc.dkgLock.Lock() diff --git a/core/crypto/dkg/dkg.go b/core/crypto/dkg/dkg.go index 5be1684..e43ebc8 100644 --- a/core/crypto/dkg/dkg.go +++ b/core/crypto/dkg/dkg.go @@ -45,7 +45,9 @@ const cryptoType = "bls" var publicKeyLength int func init() { - bls.Init(curve) + if err := bls.Init(curve); err != nil { + panic(err) + } pubKey := &bls.PublicKey{} publicKeyLength = len(pubKey.Serialize()) @@ -276,6 +278,7 @@ func (pubs *PublicKeyShares) Clone() *PublicKeyShares { // NewID creates a ew ID structure. func NewID(id []byte) ID { var blsID bls.ID + // #nosec G104 blsID.SetLittleEndian(id) return blsID } @@ -284,6 +287,7 @@ func NewID(id []byte) ID { // It returns err if the byte slice is not valid. func BytesID(id []byte) (ID, error) { var blsID bls.ID + // #nosec G104 err := blsID.SetLittleEndian(id) return blsID, err } @@ -325,6 +329,7 @@ func (prvs *PrivateKeyShares) SetParticipants(IDs IDs) { prvs.shares = make([]PrivateKey, len(IDs)) prvs.shareIndex = make(map[ID]int, len(IDs)) for idx, ID := range IDs { + // #nosec G104 prvs.shares[idx].privateKey.Set(prvs.masterPrivateKey, &ID) prvs.shareIndex[ID] = idx } @@ -411,7 +416,9 @@ func (pubs *PublicKeyShares) Share(ID ID) (*PublicKey, error) { if err := pk.publicKey.Set(pubs.masterPublicKey, &ID); err != nil { return nil, err } - pubs.AddShare(ID, &pk) + if err := pubs.AddShare(ID, &pk); err != nil { + return nil, err + } return &pk, nil } diff --git a/core/crypto/ecdsa/ecdsa.go b/core/crypto/ecdsa/ecdsa.go index 82e4dca..9da5f4f 100644 --- a/core/crypto/ecdsa/ecdsa.go +++ b/core/crypto/ecdsa/ecdsa.go @@ -29,7 +29,9 @@ import ( const cryptoType = "ecdsa" func init() { - crypto.RegisterSigToPub(cryptoType, SigToPub) + if err := crypto.RegisterSigToPub(cryptoType, SigToPub); err != nil { + panic(err) + } } // PrivateKey represents a private key structure used in geth and implments diff --git a/core/dkg-tsig-protocol.go b/core/dkg-tsig-protocol.go index 7076ef3..02e3ae0 100644 --- a/core/dkg-tsig-protocol.go +++ b/core/dkg-tsig-protocol.go @@ -172,7 +172,7 @@ func newDKGProtocol( } func (d *dkgProtocol) processMasterPublicKeys( - mpks []*typesDKG.MasterPublicKey) error { + mpks []*typesDKG.MasterPublicKey) (err error) { d.idMap = make(map[types.NodeID]dkg.ID, len(mpks)) d.mpkMap = make(map[types.NodeID]*dkg.PublicKeyShares, len(mpks)) d.prvSharesReceived = make(map[types.NodeID]struct{}, len(mpks)) @@ -187,7 +187,8 @@ func (d *dkgProtocol) processMasterPublicKeys( for _, mpk := range mpks { share, ok := d.masterPrivateShare.Share(mpk.DKGID) if !ok { - return ErrIDShareNotFound + err = ErrIDShareNotFound + continue } d.recv.ProposeDKGPrivateShare(&typesDKG.PrivateShare{ ReceiverID: mpk.ProposerID, @@ -195,7 +196,7 @@ func (d *dkgProtocol) processMasterPublicKeys( PrivateShare: *share, }) } - return nil + return } func (d *dkgProtocol) proposeNackComplaints() { diff --git a/core/test/fake-transport.go b/core/test/fake-transport.go index fe19fdf..ed9871b 100644 --- a/core/test/fake-transport.go +++ b/core/test/fake-transport.go @@ -104,6 +104,7 @@ func (t *FakeTransport) Broadcast(endpoints map[types.NodeID]struct{}, } go func(nID types.NodeID) { time.Sleep(latency.Delay()) + // #nosec G104 t.Send(nID, msg) }(ID) } @@ -135,7 +136,9 @@ func (t *FakeTransport) Join( if t.serverChannel, ok = serverEndpoint.(chan *TransportEnvelope); !ok { return nil, fmt.Errorf("accept channel of *TransportEnvelope when join") } - t.Report(t) + if err := t.Report(t); err != nil { + panic(err) + } // Wait for peers info. for { envelope := <-t.recvChannel diff --git a/core/test/governance.go b/core/test/governance.go index 05ceb62..5cf9732 100644 --- a/core/test/governance.go +++ b/core/test/governance.go @@ -255,7 +255,9 @@ func (g *Governance) broadcastPendingStateChanges() { if err != nil { panic(err) } - g.networkModule.Broadcast(packedStateChanges(packed)) + if err := g.networkModule.Broadcast(packedStateChanges(packed)); err != nil { + panic(err) + } } // State allows to access embed State instance. diff --git a/core/test/tcp-transport.go b/core/test/tcp-transport.go index d32935b..566549f 100644 --- a/core/test/tcp-transport.go +++ b/core/test/tcp-transport.go @@ -142,7 +142,9 @@ const handshakeMsg = "Welcome to DEXON network for test." func (t *TCPTransport) serverHandshake(conn net.Conn) ( nID types.NodeID, err error) { - conn.SetDeadline(time.Now().Add(3 * time.Second)) + if err := conn.SetDeadline(time.Now().Add(3 * time.Second)); err != nil { + panic(err) + } msg := &tcpMessage{ NodeID: t.nID, Type: "handshake", @@ -172,7 +174,9 @@ func (t *TCPTransport) serverHandshake(conn net.Conn) ( func (t *TCPTransport) clientHandshake(conn net.Conn) ( nID types.NodeID, err error) { - conn.SetDeadline(time.Now().Add(3 * time.Second)) + if err := conn.SetDeadline(time.Now().Add(3 * time.Second)); err != nil { + panic(err) + } var payload []byte if payload, err = t.read(conn); err != nil { return @@ -497,7 +501,9 @@ func (t *TCPTransport) listenerRoutine(listener *net.TCPListener) { default: } - listener.SetDeadline(time.Now().Add(5 * time.Second)) + if err := listener.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { + panic(err) + } conn, err := listener.Accept() if err != nil { // Check if the connection is closed. @@ -632,6 +638,7 @@ func (t *TCPTransportClient) Join( if nID == t.nID { break } + // #nosec G104 ln.Close() } if err != nil { @@ -656,7 +663,7 @@ func (t *TCPTransportClient) Join( } } // The port is used, generate another port randomly. - t.localPort = 1024 + rand.Int()%1024 + t.localPort = 1024 + rand.Int()%1024 // #nosec G404 } fmt.Println("Connecting to server", "endpoint", serverEndpoint) diff --git a/simulation/app.go b/simulation/app.go index c72a0a5..9f399ce 100644 --- a/simulation/app.go +++ b/simulation/app.go @@ -168,6 +168,7 @@ func (a *simApp) TotalOrderingDelivered( BlockHash: blockHashes, ConfirmLatency: latencies, } + // #nosec G104 a.netModule.Report(blockList) a.DeliverID++ } @@ -239,6 +240,7 @@ func (a *simApp) BlockDelivered( Type: blockTimestamp, Payload: jsonPayload, } + // #nosec G104 a.netModule.Report(msg) } diff --git a/simulation/config/config.go b/simulation/config/config.go index 482f9cd..428b128 100644 --- a/simulation/config/config.go +++ b/simulation/config/config.go @@ -157,7 +157,7 @@ func GenerateDefault(path string) error { // Read reads the config from a file. func Read(path string) (*Config, error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 if err != nil { return nil, err } diff --git a/simulation/node.go b/simulation/node.go index 517253d..437d4b7 100644 --- a/simulation/node.go +++ b/simulation/node.go @@ -129,7 +129,10 @@ func (n *node) run( hashes := make(common.Hashes, 0, len(peers)) for _, pubKey := range peers { nID := types.NewNodeID(pubKey) - n.gov.State().RequestChange(test.StateAddNode, pubKey) + if err := + n.gov.State().RequestChange(test.StateAddNode, pubKey); err != nil { + panic(err) + } hashes = append(hashes, nID.Hash) } n.prepareConfigs() @@ -152,7 +155,9 @@ readyLoop: continue } n.logger.Info("register config change", "change", c) - c.RegisterChange(n.gov) + if err := c.RegisterChange(n.gov); err != nil { + panic(err) + } } default: panic(fmt.Errorf("receive unexpected server notification: %v", ntf)) @@ -199,21 +204,21 @@ MainLoop: func (n *node) prepareConfigs() { // Prepare configurations. cConfig := n.cfg.Node.Consensus - n.gov.State().RequestChange(test.StateChangeK, cConfig.K) - n.gov.State().RequestChange(test.StateChangePhiRatio, cConfig.PhiRatio) - n.gov.State().RequestChange(test.StateChangeNumChains, cConfig.NumChains) + n.gov.State().RequestChange(test.StateChangeK, cConfig.K) // #nosec G104 + n.gov.State().RequestChange(test.StateChangePhiRatio, cConfig.PhiRatio) // #nosec G104 + n.gov.State().RequestChange(test.StateChangeNumChains, cConfig.NumChains) // #nosec G104 n.gov.State().RequestChange( - test.StateChangeNotarySetSize, cConfig.NotarySetSize) - n.gov.State().RequestChange(test.StateChangeDKGSetSize, cConfig.DKGSetSize) + test.StateChangeNotarySetSize, cConfig.NotarySetSize) // #nosec G104 + n.gov.State().RequestChange(test.StateChangeDKGSetSize, cConfig.DKGSetSize) // #nosec G104 n.gov.State().RequestChange(test.StateChangeLambdaBA, time.Duration( - cConfig.LambdaBA)*time.Millisecond) + cConfig.LambdaBA)*time.Millisecond) // #nosec G104 n.gov.State().RequestChange(test.StateChangeLambdaDKG, time.Duration( - cConfig.LambdaDKG)*time.Millisecond) + cConfig.LambdaDKG)*time.Millisecond) // #nosec G104 n.gov.State().RequestChange(test.StateChangeRoundInterval, time.Duration( - cConfig.RoundInterval)*time.Millisecond) + cConfig.RoundInterval)*time.Millisecond) // #nosec G104 n.gov.State().RequestChange(test.StateChangeMinBlockInterval, time.Duration( - cConfig.MinBlockInterval)*time.Millisecond) - n.gov.State().ProposeCRS(0, crypto.Keccak256Hash([]byte(cConfig.GenesisCRS))) + cConfig.MinBlockInterval)*time.Millisecond) // #nosec G104 + n.gov.State().ProposeCRS(0, crypto.Keccak256Hash([]byte(cConfig.GenesisCRS))) // #nosec G104 // These rounds are not safe to be registered as pending state change // requests. for i := uint64(0); i <= core.ConfigRoundShift+1; i++ { -- cgit