CPP: Add query for CWE-805: Buffer Access with Incorrect Length Value using some functions

[ihsinme]

This query is looking for a simple error condition in the argument. It seemed to me that in working with this problem, the functions of working with ssl were undeservedly forgotten.

It is worth noting that this request also has the potential to be expanded by simply adding new features.

CVE-2004-2003

[MathiasVP]

Hi @ihsinme,

Thanks for another contribution! Seeing as we still have quite a backlog to review from you it'll probably take some time before we get to it.

[ihsinme]

Thanks for the contribution. How does this query relate to cpp/overrunning-write and the BufferWrite library?

if you mean how it differs then it is:

1. functions set
2. simplicity , I only consider the situation sizeof to const

I noticed that some functions overlap ((( I remove them.

[jketema]

I think the question is: do we want to introduce this query, or might it be better to find ways of extending the one we already have with more functions? If we extend the one we already have, are there cases this query would pick up that the one we already have doesn't, or vice versa?

[ihsinme]

I think you know better.

but my query is just expanding, it actually needs a mechanism to look up the functions contained in the main call sizeof(var) and var, and added the name of that function and the argument number.

[ihsinme]

Besides the qhelp problem this looks ok. As I mentioned in the other PR I'm reviewing, more descriptive variables names would help, b, s, bpos and spos are not very descriptive. I can live with v and at as those are very local scope in the query, and cannot be confused with anything else.

you're right. what do you think about:

  exists(ArrayType array, int bpos, int spos |
    numberArgument(fc.getTarget(), bpos, spos) and
    fc.getArgument(spos).getValue().toInt() > array.getByteSize() and
    fc.getArgument(bpos).(VariableAccess).getTarget().getADeclarationEntry().getType() = array
  )

Using some class-based approach might also have been useful here, especially to group names that logically belong to the same library X509, SSL, BIO, EVP, BN, ...

I thought about it, especially when the first big group was formed to include argument positions. but then he abandoned this idea, since many groups containing one or two functions appeared. if you still think so, I'll probably move the names to classes and add instanceof to the classes.

Were you planning to submit this for a security bounty?

yes I plan

[jketema]

you're right. what do you think about:

Not very helpful, as I need to dig into the defined predicate to figure out what bpos and spos are. More helpful would be to name those bufferArgumentPosition and sizeArgumentPosition (or bufArgPos and sizeArgPos, or something like that).

[jketema]

Were you planning to submit this for a security bounty?

yes I plan

You'll need to submit this before we merge this.

[ihsinme]

Were you planning to submit this for a security bounty?

yes I plan

You'll need to submit this before we merge this.

Do I need to do this for this PR only, or do I have to generate pre-merge requests for all PRs?

[jketema]

Do I need to do this for this PR only, or do I have to generate pre-merge requests for all PRs?

Preferably for all.

[jketema]

@ihsinme The formatting of the query file doesn't follow our guidelines, could you please reformat? Thanks.

[jketema]

@ihsinme I had totally forgotten about this, but basic performance tips can be found here: https://codeql.github.com/docs/writing-codeql-queries/troubleshooting-query-performance/

[ihsinme]

@jketema thanks, I watched it. I think your fix is closer to Parameterized annotations

[jketema]

Correct. I don't think the "troubleshooting query performance" page would have helped a lot in this particular case, but trying the tips mentioned there are generally a good starting point.

[ihsinme]

good afternoon @jketema. any news on this PR?

[jketema]

Please ping security lab about this. Note that people may be on holiday, so response might be slow.

[github-actions[bot]]

QHelp previews:

cpp/ql/src/experimental/Security/CWE/CWE-805/BufferAccessWithIncorrectLengthValue.qhelp

Buffer access with incorrect length value

Using a size argument that is larger than the buffer size will result in an out-of-bounds memory access and possibly overflow. You need to limit the value of the length argument.

Example

The following example shows the use of a function with and without an error in the size argument.

...
char buf[256];
X509_NAME_oneline(X509_get_subject_name(peer),buf,sizeof(buf)); // GOOD
...
char buf[256];
X509_NAME_oneline(X509_get_subject_name(peer),buf,1024); // BAD
...

References

• CERT Coding Standard: ARR38-C. Guarantee that library functions do not form invalid pointers - SEI CERT C Coding Standard - Confluence.
• Common Weakness Enumeration: CWE-805.