[PATCH] Respect WG protocol reserved bytes

[PATCH] Respect WG protocol reserved bytes

[Laura Zelenku]

Packet that respects WG protocol contains Type on first byte followed by
three reserved bytes. Because wireguard-go implementation uses element
pools it is required to make sure that reserved bytes are cleared for
outgoing traffic (can get dirty by "bad" clients). Clearing reserved
bytes is also for backwards compatibility.

Signed-off-by: Laura Zelenku <laura.zelenku@wandera.com>
---
 device/noise-protocol.go | 12 ++++++++----
 device/receive.go        |  4 ++--
 device/send.go           |  6 ++++--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/device/noise-protocol.go b/device/noise-protocol.go
index 0212b7d..b5ef72b 100644
--- a/device/noise-protocol.go
+++ b/device/noise-protocol.go
@@ -82,7 +82,8 @@ const (
  */
 
 type MessageInitiation struct {
-       Type      uint32
+       Type      uint8
+       Reserved  [3]byte
        Sender    uint32
        Ephemeral NoisePublicKey
        Static    [NoisePublicKeySize + poly1305.TagSize]byte
@@ -92,7 +93,8 @@ type MessageInitiation struct {
 }
 
 type MessageResponse struct {
-       Type      uint32
+       Type      uint8
+       Reserved  [3]byte
        Sender    uint32
        Receiver  uint32
        Ephemeral NoisePublicKey
@@ -102,14 +104,16 @@ type MessageResponse struct {
 }
 
 type MessageTransport struct {
-       Type     uint32
+       Type     uint8
+       Reserved [3]byte
        Receiver uint32
        Counter  uint64
        Content  []byte
 }
 
 type MessageCookieReply struct {
-       Type     uint32
+       Type     uint8
+       Reserved [3]byte
        Receiver uint32
        Nonce    [chacha20poly1305.NonceSizeX]byte
        Cookie   [blake2s.Size128 + poly1305.TagSize]byte
diff --git a/device/receive.go b/device/receive.go
index b1959c6..e0d57bc 100644
--- a/device/receive.go
+++ b/device/receive.go
@@ -22,7 +22,7 @@ import (
 )
 
 type QueueHandshakeElement struct {
-       msgType  uint32
+       msgType  uint8
        packet   []byte
        endpoint conn.Endpoint
        buffer   *[MaxMessageSize]byte
@@ -121,7 +121,7 @@ func (device *Device) RoutineReceiveIncoming(IP int, bind conn.Bind) {
                // check size of packet
 
                packet := buffer[:size]
-               msgType := binary.LittleEndian.Uint32(packet[:4])
+               msgType := packet[0]
 
                var okay bool
 
diff --git a/device/send.go b/device/send.go
index a437cf1..dc4a8e2 100644
--- a/device/send.go
+++ b/device/send.go
@@ -373,11 +373,13 @@ func (device *Device) RoutineEncryption() {
                // populate header fields
                header := elem.buffer[:MessageTransportHeaderSize]
 
-               fieldType := header[0:4]
+               fieldType := header[0:1]
+               fieldReserved := header[1:4]
                fieldReceiver := header[4:8]
                fieldNonce := header[8:16]
 
-               binary.LittleEndian.PutUint32(fieldType, MessageTransportType)
+               fieldType[0] = byte(MessageTransportType)
+               copy(fieldReserved, []byte{}) // clear reserved bytes
                binary.LittleEndian.PutUint32(fieldReceiver, elem.keypair.remoteIndex)
                binary.LittleEndian.PutUint64(fieldNonce, elem.nonce)
 
-- 
2.28.0



-- 
*IMPORTANT NOTICE*: This email, its attachments and any rights attaching 
hereto are confidential and intended exclusively for the person to whom the 
email is addressed. If you are not the intended recipient, do not read, 
copy, disclose or use the contents in any way. Wandera accepts no liability 
for any loss, damage or consequence resulting directly or indirectly from 
the use of this email and attachments.

Re: [PATCH] Respect WG protocol reserved bytes

[Aaron Jones]

[-- Attachment #1.1: Type: text/plain, Size: 886 bytes --]

On 17/03/2021 07:55, Laura Zelenku wrote:
> Packet that respects WG protocol contains Type on first byte followed by
> three reserved bytes. Because wireguard-go implementation uses element
> pools it is required to make sure that reserved bytes are cleared for
> outgoing traffic (can get dirty by "bad" clients). Clearing reserved
> bytes is also for backwards compatibility.

Encoding the message type as a little-endian 32-bit integer already
takes care of setting the reserved bytes to zero; e.g. for a packet of
message type 1 (handshake initiation), its little-endian 32-bit encoding
is the following sequence of bytes: [ 0x01 0x00 0x00 0x00 ].

This is also the approach used for checking message types on the
receiving end, so packets whose reserved bytes are non-zero are already
discarded as being those of unknown types of message.

Regards,
Aaron Jones


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Respect WG protocol reserved bytes

[Laura Zelenku]

If the client send some data in reserved bytes you will have logs full of errors because the message gets type from 4 bytes instead of 1 byte (like it is in protocol description).
I would like implementation respects protocol - https://www.wireguard.com/papers/wireguard.pdf . Yes, in our project we use reserved bytes.

I know that when there are zeros in reserved bytes, everything is correct. But if you receive some non-zero value in reserved bytes?

Laura

> On 17. 3. 2021, at 13:35, Aaron Jones <me@aaronmdjones.net> wrote:
> 
> On 17/03/2021 07:55, Laura Zelenku wrote:
>> Packet that respects WG protocol contains Type on first byte followed by
>> three reserved bytes. Because wireguard-go implementation uses element
>> pools it is required to make sure that reserved bytes are cleared for
>> outgoing traffic (can get dirty by "bad" clients). Clearing reserved
>> bytes is also for backwards compatibility.
> 
> Encoding the message type as a little-endian 32-bit integer already
> takes care of setting the reserved bytes to zero; e.g. for a packet of
> message type 1 (handshake initiation), its little-endian 32-bit encoding
> is the following sequence of bytes: [ 0x01 0x00 0x00 0x00 ].
> 
> This is also the approach used for checking message types on the
> receiving end, so packets whose reserved bytes are non-zero are already
> discarded as being those of unknown types of message.
> 
> Regards,
> Aaron Jones
> 


-- 
*IMPORTANT NOTICE*: This email, its attachments and any rights attaching 
hereto are confidential and intended exclusively for the person to whom the 
email is addressed. If you are not the intended recipient, do not read, 
copy, disclose or use the contents in any way. Wandera accepts no liability 
for any loss, damage or consequence resulting directly or indirectly from 
the use of this email and attachments.

Re: [PATCH] Respect WG protocol reserved bytes

[Jason A. Donenfeld]

On 3/17/21, Laura Zelenku <laura.zelenku@wandera.com> wrote:
> If the client send some data in reserved bytes you will have logs full of
> errors because the message gets type from 4 bytes instead of 1 byte (like it
> is in protocol description).
> I would like implementation respects protocol -
> https://www.wireguard.com/papers/wireguard.pdf . Yes, in our project we use
> reserved bytes.
>
> I know that when there are zeros in reserved bytes, everything is correct.
> But if you receive some non-zero value in reserved bytes?
>

Aaron is right.

Those bytes MUST be set to zero. Otherwise you're now implementing a
different protocol. Do not use reserved bytes. They are not reserved
for you.