status-im / status-console-client

Status messaging console user interface
Mozilla Public License 2.0
10 stars 2 forks source link

Data sync code does not authenticate messages #110

Open cammellos opened 5 years ago

cammellos commented 5 years ago

When syncing with mvds, we pass to other peers the Payload of a whisper received message. This is not enough to validate a message, as it does not contain any signature of the message, therefore no authentication nor integrity checks can be made.

To overcome this there are a few ways, the first ones that come to mind is either we include a signature in our data, so that it is transferred in-band with regards of our protocol, or we pass the whole whisper message, rather than just the payload.

The benefits of the first is that our protocol is then self-contained and can be moved to any transport, although timestamp is also being used in some cases, which is provided by whisper, at the expenses of bandwidth (32 bytes of signature).

The benefits of the second is that is currently more comprehensive, but at that point you might have issues with message sizing (as you would be wrapping a whisper message inside another whisper message, and it is possible that the wrapped message exceed whisper limit)

decanus commented 5 years ago

So I think it makes more sense to include signatures in our data. So in the messages that are then wrapped in a data sync message record. The reason for this being is it makes no assumption on lower level transport and could still be used if we move away from whisper imo.

oskarth commented 5 years ago

@decanus do you want to add this to the spec and update mvds?

Also a question. Wouldn't this to some extent put security properties such as Integrity and Authentication on the data sync level? As opposed to at the secure transport layer. I'm noticing this because it's different from how e.g. Briar designed their layers. Contrast https://code.briarproject.org/briar/briar-spec/blob/master/protocols/BSP.md and https://code.briarproject.org/briar/briar-spec/blob/master/protocols/BTP.md Is this correct? Any thoughts on why and what this means?

cammellos commented 5 years ago

Wouldn't this to some extent put security properties such as Integrity and Authentication on the data sync level?

We are not planning to add this to data-sync, which will not be having to deal with I/Auth, it will be added to the application/encryption layer of the protocol.

Briar has the same, https://code.briarproject.org/briar/briar/blob/master/briar-core/src/main/java/org/briarproject/briar/blog/BlogPostValidator.java#L99 , a message is Data + Signature

The issue is that currently we use whisper for authentication/integrity, but the data sync layer is not relaying complete whisper messages (if it did there would be no problem, but it would introduce some other issues in terms of size, ttl etc), but only the payload of the whisper message, which does not include the signature. So we either push that in a layer below, so that is always included (which layer is to be decided and depends on what we want to do), or we send the whole whisper message (which will be a whisper message wrapped in another whisper message, at which point you might get some weird cases)

decanus commented 5 years ago

@oskarth this doesn’t require any changes to MVDS. It only requires a change to the status message.

oskarth commented 5 years ago

Gotcha thanks