The len field in the UDP header is 16 bit long. It can therefore store a value of up to (2 ** 16 - 1). This value is currently used for MAX_PAYLOAD. But the UDP len field not only stores the payload length but also the udp header itself. Thus the length of MAX_PAYLOAD has to be reduced by the size of udphdr to make sure that the payload can still be sent.
Reported-by: Hans-Werner Hilse hwhilse@gmail.com Signed-off-by: Sven Eckelmann sven@narfation.org --- alfred.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/alfred.h b/alfred.h index 8fc8ab1..7c308e5 100644 --- a/alfred.h +++ b/alfred.h @@ -26,4 +26,5 @@ #include <net/ethernet.h> #include <netinet/in.h> +#include <netinet/udp.h> #include <stdint.h> #include <time.h> @@ -138,5 +139,5 @@ struct globals { #define debugFree(ptr, num) free(ptr)
-#define MAX_PAYLOAD ((1 << 16) - 1) +#define MAX_PAYLOAD ((1 << 16) - 1 - sizeof(struct udphdr))
extern const struct in6_addr in6addr_localmcast;
It is checked when data is send by checking if the data would fit inside the outgoing UDP packet. But it is not checked if the data would fit after the sending was done. This doesn't have to be true just from the restrictions which can be seen in this function. So just check if the data and its headers would now fit in outgoing buffer before copying the data to the output buffer.
This is not a problem by itself because the data + header in the dataset cannot be larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).
Reported-by: Hans-Werner Hilse hwhilse@gmail.com Signed-off-by: Sven Eckelmann sven@narfation.org --- send.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/send.c b/send.c index 8853970..5a92132 100644 --- a/send.c +++ b/send.c @@ -92,4 +92,9 @@ int push_data(struct globals *globals, struct interface *interface, }
+ /* still too large? - should never happen */ + if (total_length + dataset->data.header.length + sizeof(*data) > + MAX_PAYLOAD - sizeof(*push)) + continue; + data = (struct alfred_data *) (buf + sizeof(*push) + total_length);
On Sunday 31 May 2015 13:35:56 Sven Eckelmann wrote:
It is checked when data is send by checking if the data would fit inside the outgoing UDP packet. But it is not checked if the data would fit after the sending was done. This doesn't have to be true just from the restrictions which can be seen in this function. So just check if the data and its headers would now fit in outgoing buffer before copying the data to the output buffer.
This is not a problem by itself because the data + header in the dataset cannot be larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).
Alternative commit message:
The sending code is automatically transmitting a packet when the next data block would not fit inside the outgoing, aggregated UDP packet. But the code does not check whether the data would then fit inside the new, complete empty push_data packet. It is currently no problem because alfred has the restriction that a dataset never stores a buffer larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0) - sizeof(struct alfred_data)). Therefore, the length check for the empty push_data packet + dataset buffer would never fail.
Nonetheless, make this check explicit to avoid problems when the receiving code is changed or the sending code gets the ability to limit the size of outgoing UDP packets.
The push data code currently sends packets with MAX_PAYLOAD. This allows some checks to be dropped. For example, the check if data must be send out is just a check if the current data still can be copied to the output buffer. But this would also mean that data with a size larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)) may trigger an empty packet.
This is not a problem by itself because currently the data + header in the dataset cannot be larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).
Signed-off-by: Sven Eckelmann sven@narfation.org --- send.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/send.c b/send.c index 5a92132..43c10f4 100644 --- a/send.c +++ b/send.c @@ -83,4 +83,8 @@ int push_data(struct globals *globals, struct interface *interface, if (total_length + dataset->data.header.length + sizeof(*data) > MAX_PAYLOAD - sizeof(*push)) { + /* is there any data to send? */ + if (total_length == 0) + continue; + tlv_length = total_length; tlv_length += sizeof(*push) - sizeof(push->header);
Check if the data and its headers would fit in outgoing buffer before copying the data to the output buffer. This is not a problem by itself because the data + header in the dataset cannot be larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).
Signed-off-by: Sven Eckelmann sven@narfation.org --- unix_sock.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/unix_sock.c b/unix_sock.c index 2c862d5..29c934e 100644 --- a/unix_sock.c +++ b/unix_sock.c @@ -183,4 +183,9 @@ static int unix_sock_req_data_reply(struct globals *globals, int client_sock, continue;
+ /* too large? - should never happen */ + if (dataset->data.header.length + sizeof(*data) > + MAX_PAYLOAD - sizeof(*push)) + continue; + data = push->data; memcpy(data, &dataset->data, sizeof(*data));
The length in the TLV header for push_data is without the TLV header itself. So the length check should instead only check for this size because the other size checks will be done later.
Signed-off-by: Sven Eckelmann sven@narfation.org --- unix_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/unix_sock.c b/unix_sock.c index 29c934e..570c62c 100644 --- a/unix_sock.c +++ b/unix_sock.c @@ -109,5 +109,5 @@ static int unix_sock_add_data(struct globals *globals, len = ntohs(push->header.length);
- if (len < (int)(sizeof(*push) + sizeof(push->header))) + if (len < (int)(sizeof(*push) - sizeof(push->header))) goto err;
The push_data packets which are too small can be dropped earlier in the transaction process. These cannot be parsed later when finishing the transaction and thus it is unnecessary to first enqueue them to the transaction list and allocate extra memory for the management structure.
Signed-off-by: Sven Eckelmann sven@narfation.org --- recv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/recv.c b/recv.c index 16242bc..f459190 100644 --- a/recv.c +++ b/recv.c @@ -189,4 +189,6 @@ static int process_alfred_push_data(struct globals *globals,
len = ntohs(push->header.length); + if (len < (int)(sizeof(*push) - sizeof(push->header))) + goto err;
search.server_addr = mac;
On Sunday 31 May 2015 13:35:55 Sven Eckelmann wrote:
The len field in the UDP header is 16 bit long. It can therefore store a value of up to (2 ** 16 - 1). This value is currently used for MAX_PAYLOAD. But the UDP len field not only stores the payload length but also the udp header itself. Thus the length of MAX_PAYLOAD has to be reduced by the size of udphdr to make sure that the payload can still be sent.
Reported-by: Hans-Werner Hilse hwhilse@gmail.com Signed-off-by: Sven Eckelmann sven@narfation.org
Applied the whole series with the alternative wording for Patch 2.
Thanks! Simon
b.a.t.m.a.n@lists.open-mesh.org
-
Simon Wunderlich -
Sven Eckelmann