search

agl / pond

Pond
BSD 3-Clause "New" or "Revised" License
912 stars 109 forks source link

No retval check of unmarshal in server.go:revocation() [server crash] #37

Closed asn-d6 closed 10 years ago

asn-d6 commented 10 years ago

Hello,

I might be wrong but I think that two code issues can be combined to crash the pond server.

a) In server.go:newAccount() we write req.Group to that file, without checking that it's a proper group key (like the Group() method does). This is not bad on its own, but it would be more defensive if the key is checked upon insertion to the file.

b) In server.go:revocation() we do: group := account.Group() groupCopy, _ := new(bbssig.Group).Unmarshal(group.Marshal()) groupCopy.Update(revocation) without checking the retval of Group() or Unmarshal(). I believe that if a new account is created with a corrupted key, and then a revocation is executed for that account, the Group() operation will fail and cause a nil dereference or something in the Unmarshal. If that doens't do it, then the Unmarshal will fail and cause the same in the Update(). In any case, servers might crash. Maybe.

agl commented 10 years ago

Thanks! The problem isn't the Unmarshal call in revocation(), rather the lack of checking of account.Group(). I've also added a check in newAccount to ensure that bad groups don't enter the system in the first place.