Use implicit IV for encryption

[pan-apple]

Problem

The encryption IV can be derived from other fields in message header. It'll save 8 bytes per message.

Summary of Changes

Use Message ID (monotonically increasing number per peer connection), and Source Node ID to derive the IV.

[gerickson]

@emargolis, @dougsteed, @roydsouza

[andy31415]

I believe the baseline intent (in WML) for node id to be optional was:

• connection-oriented connections (e.g. TCP or BLE) the node ID only needs to be sent once
• can be implied and never sent if addressing scheme can deduce node id from ipv6 address

This does not invalidate this approach, however I am wondering: why do we include the node id at all? Do we expect the same encryption key to be shared among devices? or is this for any cases where a shared key is used (some multicast messages)?

[andy31415]

Adding approval bit. Would like to understand at some point why we decided to also use an optional field for IV computation.

[andy31415]

XOR may be better from a security perspecitve than OR

[pan-apple]

I discussed it with @balducci-apple offline before pushing this PR. We didn't see a benefit of XOR over OR in this scenario.

[andy31415]

Here is my concrete attack proposal:

I sniff node ID, say it is 1 for simplicity

Then IV(MessgeeID 1, NodeId 1) is identical with IV(messageid 0x100000001, NodeID1)

So if I sniff a 'door unlock payload' with any message id for node one, I know that sending an identical payload with message id increased by 0x100000001 will get accepted and decrypted.

OR makes me reuse IVs which is bad.

[balducci-apple]

In this case MessageID is a 32 bit value so 0x100000001 is invalid. As MessageID reaches 0xffffffff we'll need to perform a rekey operation (in practice we'll have rekeyed before this point). Once we rekey we can safely set MessageID to 0. In this case the top 4 bytes of the IV are exactly some transform of the source ID and the bottom 4 bytes are exactly the message ID.

[emargolis]

Incorporation of sourceNodeId in the IV value is symbolic and is not really required. We could simplify it to: IV = { 32'b0, messageId }.
But if we incorporate sourceNodeId then XOR operation would be more appropriate to use. OR operation will transform 32 most significant bits of IV to be mostly ones.

[balducci-apple]

Good catch on using XOR over OR in the calculation.

[andy31415]

Marking as changes requested: usage of OR and an unfortunate node ID weakens message id usage (although only for very large numbers).

Specific examples (made up large to force errors):

NodeId 0xFF

makes messageId 0x0000000100000000
be identical to messageid 0x0000000200000000 (or 0 for that matter).

We should not wrap 32 bit that frequently, but this should be XOR or not used at all. Even with XOR if keys are reused I worry that one node id inverts the id of another ... so why bother. Do we have anyone to consult about cryptography (not my domain)

[pan-apple]

Marking as changes requested: usage of OR and an unfortunate node ID weakens message id usage (although only for very large numbers).

Specific examples (made up large to force errors):

NodeId 0xFF

makes messageId 0x0000000100000000
be identical to messageid 0x0000000200000000 (or 0 for that matter).

We should not wrap 32 bit that frequently, but this should be XOR or not used at all. Even with XOR if keys are reused I worry that one node id inverts the id of another ... so why bother. Do we have anyone to consult about cryptography (not my domain)

Won't it make the IV 0x000000FF00000000?

[dougsteed]

It is of course extremely important that the same IV not ever be repeated for a different message with the same key. Provided this can be guaranteed through the combination of message id/node id this will be fine. Need to be careful if there is any scenario where the message id space could somehow be reset with the same key being used.

[andy31415]

Apparently my main misunderstanding was that messageid is 32-bit. This was not obvious from uint64_t iv = GetMessageId() which seemed like message id would be 64 bit,starting my whole dillema.

Discussed with @pan-apple to try to make it clearer (assert/comment/xor/anything that makes me read the code and claim it is obviously correct).

[pan-apple]

It is of course extremely important that the same IV not ever be repeated for a different message with the same key. Provided this can be guaranteed through the combination of message id/node id this will be fine. Need to be careful if there is any scenario where the message id space could somehow be reset with the same key being used.

@dougsteed , I agree. @andy31415 and I talked offline, and main confusion was due to size of message ID. Currently it is 4 bytes, but uint64_t IV = header.GetMessageId(); made it look like it is 8 bytes long. I'll add a comment and update the code to make sure we are using 4 bytes of message ID here (by & it with 0xffffffff)

[balducci-apple]

In this case MessageID is a 32 bit value so 0x100000001 is invalid. As MessageID reaches 0xffffffff we'll need to perform a rekey operation (in practice we'll have rekeyed before this point). Once we rekey we can safely set MessageID to 0. In this case the top 4 bytes of the IV are exactly some transform of the source ID and the bottom 4 bytes are exactly the message ID.

[CLAassistant]

All committers have signed the CLA.