[Qemu-devel] [PATCH v2 0/2] net: check payload length and minor updates

[Qemu-devel] [PATCH v2 0/2] net: check payload length and minor updates

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

The 'net_checksum_calculate' routine reads payload length from the
packet. It could exceed the given data length value and lead to OOB
memory access. While fixing that I also came across couple of minor
coding style glitches.

This series fixes both these issues as per review in
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03799.html
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04062.html

Thank you.
--
Prasad J Pandit (2):
  net: check packet payload length
  net: minor indentation updates

 net/checksum.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/2] net: check packet payload length

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

While computing IP checksum, 'net_checksum_calculate' reads
payload length from the packet. It could exceed the given 'data'
buffer size. Add a check to avoid it.

Reported-by: Liu Ling <liuling-it@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 net/checksum.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Update as per review:
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04062.html

diff --git a/net/checksum.c b/net/checksum.c
index 14c0855..bd89083 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -59,6 +59,11 @@ void net_checksum_calculate(uint8_t *data, int length)
     int hlen, plen, proto, csum_offset;
     uint16_t csum;
 
+    /* Ensure data has complete L2 & L3 headers. */
+    if (length < 14 + 20) {
+        return;
+    }
+
     if ((data[14] & 0xf0) != 0x40)
 	return; /* not IPv4 */
     hlen  = (data[14] & 0x0f) * 4;
@@ -76,8 +81,9 @@ void net_checksum_calculate(uint8_t *data, int length)
 	return;
     }
 
-    if (plen < csum_offset+2)
-	return;
+    if (plen < csum_offset + 2 || plen + hlen >= length) {
+        return;
+    }
 
     data[14+hlen+csum_offset]   = 0;
     data[14+hlen+csum_offset+1] = 0;
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 1/2] net: check packet payload length

[Jason Wang]

On 02/24/2016 12:08 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While computing IP checksum, 'net_checksum_calculate' reads
> payload length from the packet. It could exceed the given 'data'
> buffer size. Add a check to avoid it.
>
> Reported-by: Liu Ling <liuling-it@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  net/checksum.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> Update as per review:
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04062.html
>
> diff --git a/net/checksum.c b/net/checksum.c
> index 14c0855..bd89083 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -59,6 +59,11 @@ void net_checksum_calculate(uint8_t *data, int length)
>      int hlen, plen, proto, csum_offset;
>      uint16_t csum;
>  

I must say this is still far from perfect, since it has too assumptions
. But I agree we can fix OOB first.

> +    /* Ensure data has complete L2 & L3 headers. */
> +    if (length < 14 + 20) {
> +        return;
> +    }
> +
>      if ((data[14] & 0xf0) != 0x40)
>  	return; /* not IPv4 */
>      hlen  = (data[14] & 0x0f) * 4;
> @@ -76,8 +81,9 @@ void net_checksum_calculate(uint8_t *data, int length)
>  	return;
>      }
>  
> -    if (plen < csum_offset+2)
> -	return;
> +    if (plen < csum_offset + 2 || plen + hlen >= length) {
> +        return;
> +    }

Should we count mac header here? Did "plen + hlen >= length" imply "14 +
hlen + csum_offset + 1" < length?

Looks not. Consider a TCP packet can report evil plen (e.g 20) but just
have 10 bytes payload in fact. In this case:

hlen = 20, plen = 20, csum_offset = 16, length = 44 which can pass all
the above tests, but 14 + hlen + csum_offset = 50 which is greater than
length.

>  
>      data[14+hlen+csum_offset]   = 0;
>      data[14+hlen+csum_offset+1] = 0;

Re: [Qemu-devel] [PATCH v2 1/2] net: check packet payload length

[P J P]

  Hello Jason,

+-- On Fri, 26 Feb 2016, Jason Wang wrote --+
| Should we count mac header here? Did "plen + hlen >= length" imply "14 +
| hlen + csum_offset + 1" < length?
| 
| Looks not. Consider a TCP packet can report evil plen (e.g 20) but just
| have 10 bytes payload in fact. In this case:
| 
| hlen = 20, plen = 20, csum_offset = 16, length = 44 which can pass all
| the above tests, but 14 + hlen + csum_offset = 50 which is greater than
| length.

  Ummn, not sure. hlen + plen = total length(IP + TCP/UDP), 20+20 != 44. 
'length' is also used to read and allocate requisite buffers in other parts 
from where net_checksum_calculate() is called,

===
   etsec->tx_buffer = g_realloc(etsec->tx_buffer,
                                 etsec->tx_buffer_len + bd->length);
   ...
   /* Update buffer length */
   etsec->tx_buffer_len += bd->length;

   process_tx_fcb(etsec);
     -> net_checksum_calculate(etsec->tx_buffer + 8,
                                etsec->tx_buffer_len - 8);

   OR

   memcpy(tmpbuf, page + txreq.offset, txreq.size);
   net_checksum_calculate(tmpbuf, txreq.size);
===

  - With 'length < 14 + 20', we ensure that L2 & L3 headers are complete.
    44 - 34 = 10, TCP/UDP header is incomplete.

I think 'plen=20 != 10' hints at an error in other parts of the code? Also if 
data buffer has more space than actual data, OOB access may not occur.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 1/2] net: check packet payload length

[Jason Wang]

On 03/01/2016 02:48 PM, P J P wrote:
>   Hello Jason,
>
> +-- On Fri, 26 Feb 2016, Jason Wang wrote --+
> | Should we count mac header here? Did "plen + hlen >= length" imply "14 +
> | hlen + csum_offset + 1" < length?
> | 
> | Looks not. Consider a TCP packet can report evil plen (e.g 20) but just
> | have 10 bytes payload in fact. In this case:
> | 
> | hlen = 20, plen = 20, csum_offset = 16, length = 44 which can pass all
> | the above tests, but 14 + hlen + csum_offset = 50 which is greater than
> | length.
>
>   Ummn, not sure. hlen + plen = total length(IP + TCP/UDP), 20+20 != 44. 
> 'length' is also used to read and allocate requisite buffers in other parts 
> from where net_checksum_calculate() is called,
>
> ===
>    etsec->tx_buffer = g_realloc(etsec->tx_buffer,
>                                  etsec->tx_buffer_len + bd->length);
>    ...
>    /* Update buffer length */
>    etsec->tx_buffer_len += bd->length;
>
>    process_tx_fcb(etsec);
>      -> net_checksum_calculate(etsec->tx_buffer + 8,
>                                 etsec->tx_buffer_len - 8);
>
>    OR
>
>    memcpy(tmpbuf, page + txreq.offset, txreq.size);
>    net_checksum_calculate(tmpbuf, txreq.size);
> ===
>
>   - With 'length < 14 + 20', we ensure that L2 & L3 headers are complete.

How about L4, since we will calculate L4 checksum I believe? And it
looks like the following check:

plen + hlen >= length

only count L3 header plus payload?

>     44 - 34 = 10, TCP/UDP header is incomplete.
>
> I think 'plen=20 != 10' hints at an error in other parts of the code? 

Well, the point is. If we want depend on callers to do all the checks,
we need fix all the callers. Otherwise, we need to do all the check in
net_checksum_calculate() itself (E.g to handle all kinds of evil packets).

> Also if 
> data buffer has more space than actual data, OOB access may not occur.
>
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] [PATCH v2 1/2] net: check packet payload length

[P J P]

  Hello Jason,

+-- On Wed, 2 Mar 2016, Jason Wang wrote --+
| How about L4, since we will calculate L4 checksum I believe? And it
| looks like the following check:
| 
| plen + hlen >= length
| only count L3 header plus payload?

  Yes, I've sent a revised patch v3.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

[Qemu-devel] [PATCH v2 2/2] net: minor indentation updates

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Due indentation and braces were missing at places, added them.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 net/checksum.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Update as per review:
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03799.html

diff --git a/net/checksum.c b/net/checksum.c
index bd89083..c3f1f71 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -38,8 +38,9 @@ uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq)
 
 uint16_t net_checksum_finish(uint32_t sum)
 {
-    while (sum>>16)
-	sum = (sum & 0xFFFF)+(sum >> 16);
+    while (sum >> 16) {
+        sum = (sum & 0xFFFF) + (sum >> 16);
+    }
     return ~sum;
 }
 
@@ -63,22 +64,23 @@ void net_checksum_calculate(uint8_t *data, int length)
     if (length < 14 + 20) {
         return;
     }
+    if ((data[14] & 0xf0) != 0x40) {
+        return; /* not IPv4 */
+    }
 
-    if ((data[14] & 0xf0) != 0x40)
-	return; /* not IPv4 */
     hlen  = (data[14] & 0x0f) * 4;
     plen  = (data[16] << 8 | data[17]) - hlen;
     proto = data[23];
 
     switch (proto) {
     case PROTO_TCP:
-	csum_offset = 16;
-	break;
+        csum_offset = 16;
+        break;
     case PROTO_UDP:
-	csum_offset = 6;
-	break;
+        csum_offset = 6;
+        break;
     default:
-	return;
+        return;
     }
 
     if (plen < csum_offset + 2 || plen + hlen >= length) {
-- 
2.5.0