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

[Markus Armbruster]

P J P <address@hidden> writes:

> +-- On Wed, 17 Feb 2016, Markus Armbruster wrote --+
> |> From: Prasad J Pandit <address@hidden>
> |>
> |> 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 <address@hidden>
> |> Signed-off-by: Prasad J Pandit <address@hidden>
> |> ---
> |>  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.