No verification of senders/signers being part of current configuration

[ranchalp]

[Originally detected for PBFT view change message]
The View Change messages sent by both the primary and other nodes are sent to the crypto module for verification, but whether the signers/sender are part of the current configuration is not verified neither by the view change nor by the crypto module. The crypto module implements a function to delete keys from the set of valid keys for verification, but this function does not seem to be used anywhere. This means that an old replica could forge view change messages and cause disagreements.

Note that this is an observation made after reviewing the code for PBFT's view change, and not 100% sure if perhaps another module (for example, the transport module) restricts received messages to the current configuration. But the transferable authentication of the view change messages sent by the primary to the rest of the nodes can definitely be forged (they are all sent from the primary and neither the orderers nor the crypto module verify the current configuration on its signers).

Two options to resolve this:
a) Start deleting public keys from old configurations. We should take care with this in case there are some messages that we should take into consideration. This is in my opinion dirtier because of this concern.
b) Verify the sender and all the signatures are part of the current configuration. This should be fairly easy, and mostly as simple as verifying this at the beggining of applyMsgReceived (or at least as easy as that except for specific messages that we need to allow old-members or someone else to send/sign). On top of that, check on instances that use transferable authentication (the newView message only at the moment AFAIK).

[EDIT: found error spreads throughout modules and beyond transferable authentication]
Scope of this issue:

• availability module: all messages and transferred authentication of signatures of certificates (check that sender of message and all signers of certificate are members)
• checkpoint module: all messages and transferred authentication of signatures of certificates (check that sender of message and all signers of certificate are members)
• orderer's module: all messages and transferred authentication of signatures of certificates (check that sender of message and all signers of certificate are members)
• ISS module: a bit trickier, as iss handles multiple memberships. TODO come back here when auditing ISS

I think no other module is vulnerable to this issue (or at least not for safety/liveness, perhaps yes for performance).

[ranchalp]

If it turns out that this is a problem beyond transferable authentication, this issue should be checked and resolved across other modules, as it would be a serious security concern or malicious nodes whose public key is stored by the crypto module but may not be replicas at the time of the attack.

[matejpavlovic]

Yes this is indeed an issue. I'd also go with option b).

[ranchalp]

Fixed by PR #395 for all except ISS module, leave open until fixed for that module too.

[ranchalp]

Marked ISS module fixed as per #403, although the issue is not completely resolved but specified in dedicated issue (see #402).