[Qemu-devel] [PATCH] net: check packet payload length

[Qemu-devel] [PATCH] 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/checksum.c b/net/checksum.c
index 14c0855..e51698c 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -76,8 +76,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] net: check packet payload length

[Markus Armbruster]

P J P <ppandit@redhat.com> writes:

> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/checksum.c b/net/checksum.c
> index 14c0855..e51698c 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -76,8 +76,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;

Function takes data + length, then doesn't use length *groan*.

The function stores the checksum if

* this is an IPv4 TCP or UDP packet, and

* the packet is long enough to include the checksum (but may still be
  shorter than the TCP header), and

* (new with your patch) the buffer holds the complete packet

Else it does nothing.

Doing nothing for packets other than IPv4 TCP/UDP is obviously
intentional.

Is calling this function with a partial IPv4 TCP/UDP packet legitimate?
If not, should we assert plen + hlen <= length?  Or == length, even?

If partial packet is okay, what about a partial header?

If either is legit, are the callers that can do it prepared for the
checksum not to be computed?

Style nitpick: we generally omit obviously superfluous parenthesis.

Re: [Qemu-devel] [PATCH] net: check packet payload length

[P J P]

+-- On Wed, 17 Feb 2016, Markus Armbruster wrote --+
| Is calling this function with a partial IPv4 TCP/UDP packet legitimate? 
| If partial packet is okay, what about a partial header?

  Partial? That shouldn't harm I guess.

| If not, should we assert plen + hlen <= length?  Or == length, even?

  The proposed patch would handle that, no? Ie return if it's >= length. 
Couple of places they check to ensure that length is > minimum packet length.

| If either is legit, are the callers that can do it prepared for the
| checksum not to be computed?

  IIUC checksum is not always computed, only if it's requested.

| Style nitpick: we generally omit obviously superfluous parenthesis.

  Okay. Should I resend it?

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

Re: [Qemu-devel] [PATCH] net: check packet payload length

[P J P]

+-- On Thu, 18 Feb 2016, P J P wrote --+
| +-- On Wed, 17 Feb 2016, Markus Armbruster wrote --+
| | Is calling this function with a partial IPv4 TCP/UDP packet legitimate? 
| | If partial packet is okay, what about a partial header?
| 
|   Partial? That shouldn't harm I guess.

  For partial TCP/UDP headers it does return with no checksum.

    if (plen < csum_offset+2)
        return;
 
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: check packet payload length

[Markus Armbruster]

P J P <ppandit@redhat.com> writes:

> +-- On Wed, 17 Feb 2016, Markus Armbruster 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 | 5 +++--
> |>  1 file changed, 3 insertions(+), 2 deletions(-)
> |>
> |> diff --git a/net/checksum.c b/net/checksum.c
> |> index 14c0855..e51698c 100644
> |> --- a/net/checksum.c
> |> +++ b/net/checksum.c
> |> @@ -76,8 +76,9 @@ void net_checksum_calculate(uint8_t *data, int length)
      void net_checksum_calculate(uint8_t *data, int length)
      {
          int hlen, plen, proto, csum_offset;
          uint16_t csum;

          if ((data[14] & 0xf0) != 0x40)

Buffer overrun when length <= 14.

              return; /* not IPv4 */
          hlen  = (data[14] & 0x0f) * 4;
          plen  = (data[16] << 8 | data[17]) - hlen;
          proto = data[23];

Buffer overrun when length <= 23.

I think we should check that we got at least an IPv4 header without
options (length >= 14 + 20) before accessing said header.

          switch (proto) {
          case PROTO_TCP:
              csum_offset = 16;
              break;
          case PROTO_UDP:
              csum_offset = 6;
              break;
          default:
> |>  	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;
          csum = net_checksum_tcpudp(plen, proto, data+14+12, data+14+hlen);
          data[14+hlen+csum_offset]   = csum >> 8;
          data[14+hlen+csum_offset+1] = csum & 0xff;
      }
> |
> | Function takes data + length, then doesn't use length *groan*.
> |
> | The function stores the checksum if
> |
> | * this is an IPv4 TCP or UDP packet, and
> |
> | * the packet is long enough to include the checksum (but may still be
> |   shorter than the TCP header), and
> |
> | * (new with your patch) the buffer holds the complete packet

Less wrong would be:

* we got a complete IPv4 header, and

* this is an IPv4 TCP or UDP packet, and

* the buffer holds the complete packet

The first condition is needed only so we can safely check the other
ones.

> | Else it does nothing.
> |
> | Doing nothing for packets other than IPv4 TCP/UDP is obviously
> | intentional.
> |
> | Is calling this function with a partial IPv4 TCP/UDP packet legitimate? 
> | If partial packet is okay, what about a partial header?
>
>   Partial? That shouldn't harm I guess.
>
> | If not, should we assert plen + hlen <= length?  Or == length, even?
>
>   The proposed patch would handle that, no? Ie return if it's >= length. 
> Couple of places they check to ensure that length is > minimum packet length.

Returning without doing anything may or may not be safe.  It depends on
the callers.  You patch narrowly fixes a buffer overrun on certain kinds
of partial packets.  My review asks whether partial packets can happen,
and if yes, whether the code still works then.

If it doesn't, then fixing that may be out of scope for your patch, but
it's still very much in scope for our review of this code.

> | If either is legit, are the callers that can do it prepared for the
> | checksum not to be computed?
>
>   IIUC checksum is not always computed, only if it's requested.

It all depends on the callers.  If the callers *intentionally* pass in
partial packets, and are prepared to do whatever it takes to get
whatever checksums are needed (say reassembling packets), then this
function doing nothing for partial packets isn't wrong.

But I don't like that much, because its complex.  Let me show you.

    /*
     * Compute and store checksum of IPv4 TCP or UDP packet.
     * @data holds @length bytes.
     * If @data holds a complete IPv4 TCP packet, compute its checksum
     * and store it in the TCP header.
     * Else if @data holds a complete IPv4 UDP packet, compute its
     * checksum and store it in the UDP header.
     * Else do nothing.
     * A caller that needs the checksum to be stored must not pass
     * partial packets.
     */
    void net_checksum_calculate(uint8_t *data, int length)

As so often, writing up the function contract reveals its complexity.

To figure out whether the function did anything, the caller needs to
duplicate its conditions for doing anything, unless it already knows it
passes only complete packets.

Here's a more restrictive version:

    /*
     * Compute and store checksum of IPv4 TCP or UDP packet.
     * @data holds @length bytes.  It must be a complete packet.
     * If this is an IPv4 TCP packet, compute its checksum and store it
     * in the TCP header.
     * Else if this is a complete IPv4 UDP packet, compute its checksum
     * and store it in the UDP header.
     * Else do nothing.
     */
    void net_checksum_calculate(uint8_t *data, int length)

I find it simpler.

Now, you're just the poor guy trying to patch security holes all over
the place, and asking you to improve our interfaces while there wouldn't
be fair.  That's why this part of my review is directed more to the
maintainer than to you.  I just stumbled over your patch, had a look,
and spotted more problems.  What to do with that is for the maintainer
to decide.

> | Style nitpick: we generally omit obviously superfluous parenthesis.
>
>   Okay. Should I resend it?

If the other buffer overrun I mentioned above needs fixing: yes.
Else: depends on the maintainer.

Re: [Qemu-devel] [PATCH] net: check packet payload length

[P J P]

  Hello Markus,

+-- On Thu, 18 Feb 2016, Markus Armbruster wrote --+
| 
|           if ((data[14] & 0xf0) != 0x40)
| Buffer overrun when length <= 14.
| 
|           proto = data[23];
| Buffer overrun when length <= 23.
| 
| I think we should check that we got at least an IPv4 header without
| options (length >= 14 + 20) before accessing said header.

  Right. Some callers do have checks in place to ensure length is >= minium 
packet length. Still it'll help to add an assert(length >= 14+20);

|     /*
|      * Compute and store checksum of IPv4 TCP or UDP packet.
|      * @data holds @length bytes.  It must be a complete packet.
|      * If this is an IPv4 TCP packet, compute its checksum and store it
|      * in the TCP header.
|      * Else if this is a complete IPv4 UDP packet, compute its checksum
|      * and store it in the UDP header.
|      * Else do nothing.
|      */
|     void net_checksum_calculate(uint8_t *data, int length)
| 
| I find it simpler.
| 
| If the other buffer overrun I mentioned above needs fixing: yes.
| Else: depends on the maintainer.

  Okay, I'll wait for Jason's inputs and will send a revised patch including 
your suggestions above.

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

Re: [Qemu-devel] [PATCH] net: check packet payload length

[Jason Wang]

On 02/18/2016 06:33 PM, P J P wrote:
>   Hello Markus,
>
> +-- On Thu, 18 Feb 2016, Markus Armbruster wrote --+
> | 
> |           if ((data[14] & 0xf0) != 0x40)
> | Buffer overrun when length <= 14.
> | 
> |           proto = data[23];
> | Buffer overrun when length <= 23.
> | 
> | I think we should check that we got at least an IPv4 header without
> | options (length >= 14 + 20) before accessing said header.
>
>   Right. Some callers do have checks in place to ensure length is >= minium 
> packet length. Still it'll help to add an assert(length >= 14+20);

Let's avoid adding assert() here since it could be triggered by guest.

>
> |     /*
> |      * Compute and store checksum of IPv4 TCP or UDP packet.
> |      * @data holds @length bytes.  It must be a complete packet.
> |      * If this is an IPv4 TCP packet, compute its checksum and store it
> |      * in the TCP header.
> |      * Else if this is a complete IPv4 UDP packet, compute its checksum
> |      * and store it in the UDP header.
> |      * Else do nothing.
> |      */
> |     void net_checksum_calculate(uint8_t *data, int length)
> | 
> | I find it simpler.
> | 
> | If the other buffer overrun I mentioned above needs fixing: yes.
> | Else: depends on the maintainer.
>
>   Okay, I'll wait for Jason's inputs and will send a revised patch including 
> your suggestions above.
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

I think you need audit all the callers to see if the issue mentioned by
Markus existed first.

Re: [Qemu-devel] [PATCH] net: check packet payload length

[P J P]

  Hello Jason,

+-- On Tue, 23 Feb 2016, Jason Wang wrote --+
| Let's avoid adding assert() here since it could be triggered by guest.

  Okay.

| I think you need audit all the callers to see if the issue mentioned by
| Markus existed first.

  Yes, I did. As mentioned earlier, some have check for minimum packet size, 
others check for length > 0.

net_checksum_calculate is called as:

    -  hw/net/cadence_gem.c
         if (tx_desc_get_length(desc) == 0))
            DB_PRINT("Invalid TX descriptor @ 0x%x\n",
         }
         total_bytes += tx_desc_get_length(desc);
           net_checksum_calculate(tx_packet, total_bytes);

    -  hw/net/fsl_etsec/rings.c
         if (etsec->tx_buffer_len != 0
             net_checksum_calculate(etsec->tx_buffer + 8,
                                      etsec->tx_buffer_len - 8);

    -  hw/net/virtio-net.c
         if (size > 27 && size < 1500) &&
         (buf[34] == 0 && buf[35] == 67)) {
             net_checksum_calculate(buf, size);

    -  hw/net/xen_nic.c
         if (txreq.size < 14) {                                              
             xen_be_printf(&netdev->xendev, 0, "bad packet size:
         }
         net_checksum_calculate(tmpbuf, txreq.size);
  
It might be possible to send a packet with (length < 14+20), and cause an OOB 
read in net_checksum_calculate,

    if ((data[14] & 0xf0) != 0x40)
        return; /* not IPv4 */
    hlen  = (data[14] & 0x0f) * 4;
    plen  = (data[16] << 8 | data[17]) - hlen;
    proto = data[23];

I'll send a revised patch with a check for minimum 'data' length to include 
complete layer-2(14) + layer-3(20) headers.

+    if (len < 14+20)
+        return;


Thank you.
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] net: check packet payload length

[Jason Wang]

On 02/23/2016 07:11 PM, P J P wrote:
>   Hello Jason,
>
> +-- On Tue, 23 Feb 2016, Jason Wang wrote --+
> | Let's avoid adding assert() here since it could be triggered by guest.
>
>   Okay.
>
> | I think you need audit all the callers to see if the issue mentioned by
> | Markus existed first.
>
>   Yes, I did. As mentioned earlier, some have check for minimum packet size, 
> others check for length > 0.
>
> net_checksum_calculate is called as:
>
>     -  hw/net/cadence_gem.c
>          if (tx_desc_get_length(desc) == 0))
>             DB_PRINT("Invalid TX descriptor @ 0x%x\n",
>          }
>          total_bytes += tx_desc_get_length(desc);
>            net_checksum_calculate(tx_packet, total_bytes);
>
>     -  hw/net/fsl_etsec/rings.c
>          if (etsec->tx_buffer_len != 0
>              net_checksum_calculate(etsec->tx_buffer + 8,
>                                       etsec->tx_buffer_len - 8);
>
>     -  hw/net/virtio-net.c
>          if (size > 27 && size < 1500) &&
>          (buf[34] == 0 && buf[35] == 67)) {

Are we sure size of buf is >= 35 here?

>              net_checksum_calculate(buf, size);
>
>     -  hw/net/xen_nic.c
>          if (txreq.size < 14) {                                              
>              xen_be_printf(&netdev->xendev, 0, "bad packet size:
>          }
>          net_checksum_calculate(tmpbuf, txreq.size);
>   
> It might be possible to send a packet with (length < 14+20), and cause an OOB 
> read in net_checksum_calculate,
>
>     if ((data[14] & 0xf0) != 0x40)
>         return; /* not IPv4 */
>     hlen  = (data[14] & 0x0f) * 4;
>     plen  = (data[16] << 8 | data[17]) - hlen;
>     proto = data[23];
>
> I'll send a revised patch with a check for minimum 'data' length to include 
> complete layer-2(14) + layer-3(20) headers.
>
> +    if (len < 14+20)
> +        return;
>
>
> Thank you.

Ok.

> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] [PATCH] net: check packet payload length

[P J P]

+-- On Wed, 24 Feb 2016, Jason Wang wrote --+
| >     -  hw/net/virtio-net.c
| >          if (size > 27 && size < 1500) &&
| >          (buf[34] == 0 && buf[35] == 67)) {
| 
| Are we sure size of buf is >= 35 here?

  No; I'm yet to see why it checks for > 27. If (27 < size < 34) then we have 
another OOB access there.

| > +    if (len < 14+20)
| > +        return;
| 
| Ok.

I have sent a revised v2 of this patch.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F