CODEWITHSUNDEEP
ccoverity
tcpip
tcpip

Reputation: 482

Coverity static analysis code defect

We use Coverity to detect vulnerabilities in our code. Basically this is the code snippet:

static int vendor_request(
      const struct OFPHDR *oh, 
      size_t length, 
      const struct OFPUTIL_MT **typep
      )
   {
      const struct OFPSM *osr;
      ovs_be32 vendor;

      osr = (const struct OFPSM *) oh;

      memcpy(&vendor,  ((char*)osr + sizeof(struct OFPSM)), sizeof( vendor ));
      if (vendor == htonl(VENDOR_A))
         return (functionA(oh, typep));
      if (vendor == htonl(VENDOR_B))
         return (functionB(oh, length, typep));
      else
         return 0;
   }

Here,

sizeof(struct OFPSM) = 12 bytes.

sizeof(struct OFPHDR) = 8 bytes.

Coverity says:

CID xxxxx (#1 of 2): Out-of-bounds access (OVERRUN)   
1. overrun-buffer-val: Overrunning struct type OFPHDR of 8 bytes by passing it to a function which accesses it at byte offset 12. Pointer osr indexed by constant 12U through dereference in call to memcpy.

Basically struct OFPHDR is a PDU on top of TCP layer, it's size is 8 bytes but it can vary depending upon what type of OFP message it is. Coverity says that I'm dereferencing *oh at byte offset index 12 which is out-bound-access index.

But I don't understand the problem since I'm typecasting OFPHDR to proper structure which is of 12 bytes and then dereferencing it. So, how could this error be avoided?

Upvotes: 2

Views: 11563

Answers (3)

tcpip
tcpip

Reputation: 482

Everyone, thanks for the answers.

@PeterSW: The structs are incompatible yes, but as I mentioned OFPHDR is a PDU on top of TCP layer, it's size is variable. The information which we need to extract(vendor) from that pointer lies on its 12th byte offset.

This is solved by typecasting it to correct structure which has size enough to envelop more than 12 bytes and includes that element(vendor):

struct OFPVSM {
    struct OFPSM osm;
    ovs_be32 vendor;            /* Vendor ID:
    /* Followed by vendor-defined arbitrary additional data. */
};

Here,

sizeof(struct OFPVSM) = 16 bytes.

Solution in git diff format:

-   const struct OFPSM *osr;
+   const struct OFPVSM *osr;

-   osr = (const struct OFPSM *) oh;
+   osr = (const struct OFPVSM*) oh;

Sorry for not mentioning a vital info:

struct OFPSM actually comprises of struct OFPHDR

struct OFPSM{
    struct OFPHDR header;
    ovs_be16 type;
    ovs_be16 flags;
};

"vendor" lies at the end of struct OFPSM.

Upvotes: 1

Brian Cain
Brian Cain

Reputation: 14619

But I don't understand the problem since I'm typecasting OFPHDR to proper structure which is of 12 bytes and then dereferencing it.

Coverity is trying to save you from a path where perhaps you only allocated/read in sizeof OFPHDR bytes and yet you attempt to access beyond that allocation. You can see two reasonable possibilities taking you there: your vendor == htonl(VENDOR_A) logic could be implemented incorrectly or the values that you read from the network were maliciously crafted/in error.

Your cast supposes information about the implementation of the caller that coverity thinks you can't be certain about in vendor_request.

So, how could this error be avoided?

You could avoid it by changing vendor_request like so:

typedef union {
       struct OFPHDR oh;
       struct OFPSM  osm;
} son_of_OFPHDR;

static int vendor_request(
      const son_of_OFPHDR *oh, 
      size_t length, 
      const struct OFPUTIL_MT **typep
      )

This explicitly tells compilers, static checkers, and humans that the oh input may be an OFPHDR or could be an OFPSM.

Everyone who agrees to take a son_of_OFPHDR * has an implicit pledge from callers that memory for the entire structure has been allocated. And everywhere son_of_OFPHDRs show up with automatic storage duration, sufficient memory will be allocated there.

Upvotes: 3

PeterSW
PeterSW

Reputation: 5271

This cast:

osr = (const struct OFPSM *) oh;

is breaking the strict aliasing rules since it is casting to an incompatible type. It's clear they are incompatible since you say:

sizeof(struct OFPSM) = 12 bytes.

sizeof(struct OFPHDR) = 8 bytes.

Upvotes: 3

Related Questions