From dc63edcef0c8a0a80b7de40fba3bbe9b609868a2 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 16 Feb 2017 17:58:09 +0100 Subject: [PATCH] whisper: filter id type changed to enhance security --- cmd/wnode/main.go | 2 +- whisper/whisperv5/api.go | 28 +++++++++--------- whisper/whisperv5/api_test.go | 8 +++--- whisper/whisperv5/filter.go | 47 ++++++++++++++++++++++++------- whisper/whisperv5/filter_test.go | 27 ++++++++++-------- whisper/whisperv5/peer_test.go | 2 +- whisper/whisperv5/whisper.go | 8 +++--- whisper/whisperv5/whisper_test.go | 4 +-- 8 files changed, 78 insertions(+), 48 deletions(-) diff --git a/cmd/wnode/main.go b/cmd/wnode/main.go index 89e862c29c..9a9bd4b440 100644 --- a/cmd/wnode/main.go +++ b/cmd/wnode/main.go @@ -66,7 +66,7 @@ var ( asymKey *ecdsa.PrivateKey nodeid *ecdsa.PrivateKey topic whisper.TopicType - filterID uint32 + filterID string symPass string msPassword string ) diff --git a/whisper/whisperv5/api.go b/whisper/whisperv5/api.go index 115f642736..b3f441303b 100644 --- a/whisper/whisperv5/api.go +++ b/whisper/whisperv5/api.go @@ -151,9 +151,9 @@ func (api *PublicWhisperAPI) DeleteSymKey(name string) error { // NewWhisperFilter creates and registers a new message filter to watch for inbound whisper messages. // Returns the ID of the newly created Filter. -func (api *PublicWhisperAPI) NewFilter(args WhisperFilterArgs) (uint32, error) { +func (api *PublicWhisperAPI) NewFilter(args WhisperFilterArgs) (string, error) { if api.whisper == nil { - return 0, whisperOffLineErr + return "", whisperOffLineErr } filter := Filter{ @@ -171,25 +171,25 @@ func (api *PublicWhisperAPI) NewFilter(args WhisperFilterArgs) (uint32, error) { if len(args.Topics) == 0 { info := "NewFilter: at least one topic must be specified" glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } if len(args.KeyName) != 0 && len(filter.KeySym) == 0 { info := "NewFilter: key was not found by name: " + args.KeyName glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } if len(args.To) == 0 && len(filter.KeySym) == 0 { info := "NewFilter: filter must contain either symmetric or asymmetric key" glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } if len(args.To) != 0 && len(filter.KeySym) != 0 { info := "NewFilter: filter must not contain both symmetric and asymmetric key" glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } if len(args.To) > 0 { @@ -197,13 +197,13 @@ func (api *PublicWhisperAPI) NewFilter(args WhisperFilterArgs) (uint32, error) { if !ValidatePublicKey(dst) { info := "NewFilter: Invalid 'To' address" glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } filter.KeyAsym = api.whisper.GetIdentity(string(args.To)) if filter.KeyAsym == nil { info := "NewFilter: non-existent identity provided" glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } } @@ -211,7 +211,7 @@ func (api *PublicWhisperAPI) NewFilter(args WhisperFilterArgs) (uint32, error) { if !ValidatePublicKey(filter.Src) { info := "NewFilter: Invalid 'From' address" glog.V(logger.Error).Infof(info) - return 0, errors.New(info) + return "", errors.New(info) } } @@ -220,12 +220,12 @@ func (api *PublicWhisperAPI) NewFilter(args WhisperFilterArgs) (uint32, error) { } // UninstallFilter disables and removes an existing filter. -func (api *PublicWhisperAPI) UninstallFilter(filterId uint32) { +func (api *PublicWhisperAPI) UninstallFilter(filterId string) { api.whisper.Unwatch(filterId) } // GetFilterChanges retrieves all the new messages matched by a filter since the last retrieval. -func (api *PublicWhisperAPI) GetFilterChanges(filterId uint32) []*WhisperMessage { +func (api *PublicWhisperAPI) GetFilterChanges(filterId string) []*WhisperMessage { f := api.whisper.GetFilter(filterId) if f != nil { newMail := f.Retrieve() @@ -235,7 +235,7 @@ func (api *PublicWhisperAPI) GetFilterChanges(filterId uint32) []*WhisperMessage } // GetMessages retrieves all the known messages that match a specific filter. -func (api *PublicWhisperAPI) GetMessages(filterId uint32) []*WhisperMessage { +func (api *PublicWhisperAPI) GetMessages(filterId string) []*WhisperMessage { all := api.whisper.Messages(filterId) return toWhisperMessages(all) } @@ -282,7 +282,7 @@ func (api *PublicWhisperAPI) Post(args PostArgs) error { } filter := api.whisper.GetFilter(args.FilterID) - if filter == nil && args.FilterID > 0 { + if filter == nil && len(args.FilterID) > 0 { info := fmt.Sprintf("Post: wrong filter id %d", args.FilterID) glog.V(logger.Error).Infof(info) return errors.New(info) @@ -374,7 +374,7 @@ type PostArgs struct { Payload hexutil.Bytes `json:"payload"` WorkTime uint32 `json:"worktime"` PoW float64 `json:"pow"` - FilterID uint32 `json:"filterID"` + FilterID string `json:"filterID"` PeerID hexutil.Bytes `json:"peerID"` } diff --git a/whisper/whisperv5/api_test.go b/whisper/whisperv5/api_test.go index f7deab39c4..79285559f3 100644 --- a/whisper/whisperv5/api_test.go +++ b/whisper/whisperv5/api_test.go @@ -42,7 +42,7 @@ func TestBasic(t *testing.T) { t.Fatalf("wrong version: %d.", ver) } - mail := api.GetFilterChanges(1) + mail := api.GetFilterChanges("non-existent-id") if len(mail) != 0 { t.Fatalf("failed GetFilterChanges: premature result") } @@ -212,7 +212,7 @@ func TestUnmarshalPostArgs(t *testing.T) { "payload":"0x7061796C6F61642073686F756C642062652070736575646F72616E646F6D", "worktime":777, "pow":3.1416, - "filterID":64, + "filterID":"test-filter-id", "peerID":"0xf26e7779" }`) @@ -249,7 +249,7 @@ func TestUnmarshalPostArgs(t *testing.T) { if a.PoW != 3.1416 { t.Fatalf("wrong pow: %f.", a.PoW) } - if a.FilterID != 64 { + if a.FilterID != "test-filter-id" { t.Fatalf("wrong FilterID: %d.", a.FilterID) } if !bytes.Equal(a.PeerID[:], a.Topic[:]) { @@ -257,7 +257,7 @@ func TestUnmarshalPostArgs(t *testing.T) { } } -func waitForMessage(api *PublicWhisperAPI, id uint32, target int) bool { +func waitForMessage(api *PublicWhisperAPI, id string, target int) bool { for i := 0; i < 64; i++ { all := api.GetMessages(id) if len(all) >= target { diff --git a/whisper/whisperv5/filter.go b/whisper/whisperv5/filter.go index a386aa1646..8dd1f04a77 100644 --- a/whisper/whisperv5/filter.go +++ b/whisper/whisperv5/filter.go @@ -18,6 +18,8 @@ package whisperv5 import ( "crypto/ecdsa" + crand "crypto/rand" + "fmt" "sync" "github.com/ethereum/go-ethereum/common" @@ -39,20 +41,45 @@ type Filter struct { } type Filters struct { - id uint32 // can contain any value except zero - watchers map[uint32]*Filter + watchers map[string]*Filter whisper *Whisper mutex sync.RWMutex } func NewFilters(w *Whisper) *Filters { return &Filters{ - watchers: make(map[uint32]*Filter), + watchers: make(map[string]*Filter), whisper: w, } } -func (fs *Filters) Install(watcher *Filter) uint32 { +func (fs *Filters) generateRandomID() (id string) { + var err error + buf := make([]byte, 20) + for i := 0; i < 3; i++ { + _, err = crand.Read(buf) + if err != nil { + continue + } + if !validateSymmetricKey(buf) { + err = fmt.Errorf("error in generateRandomID: crypto/rand failed to generate random data") + continue + } + id = common.Bytes2Hex(buf) + if fs.watchers[id] != nil { + err = fmt.Errorf("error in generateRandomID: generated same ID twice") + continue + } + return id + } + + // at this point all the attempts to generate the random ID failed. + // this poses a serious danger, since whisper very much relies + // on random numbers. the program must be terminated. + panic(err) +} + +func (fs *Filters) Install(watcher *Filter) string { if watcher.Messages == nil { watcher.Messages = make(map[common.Hash]*ReceivedMessage) } @@ -60,21 +87,21 @@ func (fs *Filters) Install(watcher *Filter) uint32 { fs.mutex.Lock() defer fs.mutex.Unlock() - fs.id++ - fs.watchers[fs.id] = watcher - return fs.id + id := fs.generateRandomID() + fs.watchers[id] = watcher + return id } -func (fs *Filters) Uninstall(id uint32) { +func (fs *Filters) Uninstall(id string) { fs.mutex.Lock() defer fs.mutex.Unlock() delete(fs.watchers, id) } -func (fs *Filters) Get(i uint32) *Filter { +func (fs *Filters) Get(id string) *Filter { fs.mutex.RLock() defer fs.mutex.RUnlock() - return fs.watchers[i] + return fs.watchers[id] } func (fs *Filters) NotifyWatchers(env *Envelope, p2pMessage bool) { diff --git a/whisper/whisperv5/filter_test.go b/whisper/whisperv5/filter_test.go index 5bf607cccf..259b22a0d7 100644 --- a/whisper/whisperv5/filter_test.go +++ b/whisper/whisperv5/filter_test.go @@ -43,7 +43,7 @@ func InitDebugTest(i int64) { type FilterTestCase struct { f *Filter - id uint32 + id string alive bool msgCnt int } @@ -100,14 +100,13 @@ func TestInstallFilters(t *testing.T) { filters := NewFilters(w) tst := generateTestCases(t, SizeTestFilters) - var j uint32 + var j string for i := 0; i < SizeTestFilters; i++ { j = filters.Install(tst[i].f) tst[i].id = j - } - - if j < SizeTestFilters-1 { - t.Fatalf("seed %d: wrong index %d", seed, j) + if len(j) != 40 { + t.Fatalf("seed %d: wrong filter id size [%d]", seed, len(j)) + } } for _, testCase := range tst { @@ -519,17 +518,21 @@ func TestWatchers(t *testing.T) { var i int var j uint32 var e *Envelope + var x, firstID string w := New() filters := NewFilters(w) tst := generateTestCases(t, NumFilters) for i = 0; i < NumFilters; i++ { tst[i].f.Src = nil - j = filters.Install(tst[i].f) - tst[i].id = j + x = filters.Install(tst[i].f) + tst[i].id = x + if len(firstID) == 0 { + firstID = x + } } - last := j + lastID := x var envelopes [NumMessages]*Envelope for i = 0; i < NumMessages; i++ { @@ -571,9 +574,9 @@ func TestWatchers(t *testing.T) { // another round with a cloned filter clone := cloneFilter(tst[0].f) - filters.Uninstall(last) + filters.Uninstall(lastID) total = 0 - last = NumFilters - 1 + last := NumFilters - 1 tst[last].f = clone filters.Install(clone) for i = 0; i < NumFilters; i++ { @@ -640,7 +643,7 @@ func TestWatchers(t *testing.T) { t.Fatalf("failed with seed %d: total: got %d, want 0.", seed, total) } - f := filters.Get(1) + f := filters.Get(firstID) if f == nil { t.Fatalf("failed to get the filter with seed %d.", seed) } diff --git a/whisper/whisperv5/peer_test.go b/whisper/whisperv5/peer_test.go index cc98ba2d67..fc4f29ceef 100644 --- a/whisper/whisperv5/peer_test.go +++ b/whisper/whisperv5/peer_test.go @@ -79,7 +79,7 @@ type TestNode struct { shh *Whisper id *ecdsa.PrivateKey server *p2p.Server - filerId uint32 + filerId string } var result TestData diff --git a/whisper/whisperv5/whisper.go b/whisper/whisperv5/whisper.go index a027fd84b3..87552b15ef 100644 --- a/whisper/whisperv5/whisper.go +++ b/whisper/whisperv5/whisper.go @@ -272,16 +272,16 @@ func (w *Whisper) GetSymKey(name string) []byte { // Watch installs a new message handler to run in case a matching packet arrives // from the whisper network. -func (w *Whisper) Watch(f *Filter) uint32 { +func (w *Whisper) Watch(f *Filter) string { return w.filters.Install(f) } -func (w *Whisper) GetFilter(id uint32) *Filter { +func (w *Whisper) GetFilter(id string) *Filter { return w.filters.Get(id) } // Unwatch removes an installed message handler. -func (w *Whisper) Unwatch(id uint32) { +func (w *Whisper) Unwatch(id string) { w.filters.Uninstall(id) } @@ -575,7 +575,7 @@ func (w *Whisper) Envelopes() []*Envelope { } // Messages retrieves all the decrypted messages matching a filter id. -func (w *Whisper) Messages(id uint32) []*ReceivedMessage { +func (w *Whisper) Messages(id string) []*ReceivedMessage { result := make([]*ReceivedMessage, 0) w.poolMu.RLock() defer w.poolMu.RUnlock() diff --git a/whisper/whisperv5/whisper_test.go b/whisper/whisperv5/whisper_test.go index c2ae35a3ee..312dacfc4b 100644 --- a/whisper/whisperv5/whisper_test.go +++ b/whisper/whisperv5/whisper_test.go @@ -44,7 +44,7 @@ func TestWhisperBasic(t *testing.T) { if uint64(w.Version()) != ProtocolVersion { t.Fatalf("failed whisper Version: %v.", shh.Version) } - if w.GetFilter(0) != nil { + if w.GetFilter("non-existent") != nil { t.Fatalf("failed GetFilter.") } @@ -69,7 +69,7 @@ func TestWhisperBasic(t *testing.T) { if len(mail) != 0 { t.Fatalf("failed w.Envelopes().") } - m := w.Messages(0) + m := w.Messages("non-existent") if len(m) != 0 { t.Fatalf("failed w.Messages.") }