C++: Promote cpp/invalid-pointer-deref out of experimental

[MathiasVP]

This PR moves the cpp/invalid-pointer-deref query out of experimental and into the security-extended suite.

Documentation friends: I've added a bunch of unnecessary whitespace in e776178 that will ensure the relevant files show up in the diff. Once you're done reviewing the files I'll remove the whitespace again.

[github-actions]

QHelp previews:

cpp/ql/src/Security/CWE/CWE-193/InvalidPointerDeref.qhelp

Invalid pointer dereference

The program performs an out-of-bounds read or write operation, which can cause program instability. In addition, attackers may take advantage of the situation, and implement techniques to use this vulnerability to execute arbitrary code.

Recommendation

Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.

Example

The first example allocates a buffer of size size and creates a local variable that stores the location that is one byte past the end of the allocation. This local variable is then dereferenced, which results in an out-of-bounds write. The second example subtracts one from the end variable before dereferencing it. This subtraction ensures that the write correctly updates the final byte of the allocation.

void *malloc(unsigned);
unsigned get_size();
void write_data(const unsigned char*, const unsigned char*);

int main(int argc, char* argv[]) {
  unsigned size = get_size();
  
  {
    unsigned char *begin = (unsigned char*)malloc(size);
    if(!begin) return -1;

    unsigned char* end = begin + size;
    write_data(begin, end);
    *end = '\0'; // BAD: Out-of-bounds write
  }

  {
    unsigned char *begin = (unsigned char*)malloc(size);
    if(!begin) return -1;

    unsigned char* end = begin + size;
    write_data(begin, end);
    *(end - 1) = '\0'; // GOOD: writing to the last byte
  }

}

References

• CERT C Coding Standard: ARR30-C. Do not form or use out-of-bounds pointers or array subscripts.
• OWASP: Buffer Overflow.
• Common Weakness Enumeration: CWE-119.
• Common Weakness Enumeration: CWE-125.
• Common Weakness Enumeration: CWE-193.
• Common Weakness Enumeration: CWE-787.

[mchammer01]

I'll review this for Docs.

[mchammer01]

LGTM ✨
Made a few comments and suggestions. Feel free to ignore the ones you don't agree with 😅

[jketema]

This seems to slowdown DCA significantly and seems to cause a persistent OOM in OpenJDK?

[MathiasVP]

I'm not super concerned about the slowdown since Product has already told us that people are expecting a slowdown when running security-extended. But obviously the OOM is a problem (and not something we've seen in other runs, I think). So I'll investigate that 👀

[jketema]

I'm not super concerned about the slowdown since Product has already told us that people are expecting a slowdown when running security-extended.

The slowdown is around 17% on average though, which is quite a lot.

[MathiasVP]

Indeed. I'll see if I can spot anything obvious we can do about it 🤔

[jketema]

It might help already if we can share the "bounded" predicates with the cpp/overrun-write query?

[MathiasVP]

It might help already if we can share the "bounded" predicates with the cpp/overrun-write query?

Yeah, that's a good idea.

[MathiasVP]

It might help already if we can share the "bounded" predicates with the cpp/overrun-write query?

Yeah, that's a good idea.

Fixed in 123e587.

I've successfully run the security-extended suite locally with --ram=3000 so I think this is simply another instance of OOM issues on the runner 😭.

[jketema]

Unfortunately no performance improvement after sharing the predicate 😞 Also still OOMs on OpenJDK. Even though it has OOMs it's also one of the projects that shows the biggest slowdown, so the clue might be in there?

I've successfully run the security-extended suite locally with --ram=3000 so I think this is simply another instance of OOM issues on the runner 😭.

This is a pretty bad test for this, as on the runner it's probably a 137 exit code, which means the container in which everything is run is getting killed.

[MathiasVP]

Unfortunately no performance improvement after sharing the predicate 😞 Also still OOMs on OpenJDK. Even though it has OOMs it's also one of the projects that shows the biggest slowdown, so the clue might be in there?

Maybe. In terms of increased memory usage there's nothing immediately to point to. A bunch of extra work, sure (because we're running a new query), but nothing that would have increased memory pressure.

This is a pretty bad test for this, as on the runner it's probably a 137 exit code, which means the container in which everything is run is getting killed.

I see. Do you have an idea for how I can test this better locally? 😭

[jketema]

This is a pretty bad test for this, as on the runner it's probably a 137 exit code, which means the container in which everything is run is getting killed.

I see. Do you have an idea for how I can test this better locally? 😭

Not really. This has been one of the challenges with these errors.