CPP: Add query for CWE-190: Integer Overflow or Wraparound when using transform after operation

[ihsinme]

This query looks for integer overflow situations that are not covered by the cpp/integer-multiplication-cast-to-long query. the first part is belated and meaningless transformations like s=(size_t)(i*i), which would still happen according to the law of expression. It's possible that the developer didn't use the conversion location correctly. the second part is the conversion of types that are the same size but different in sign, and then use the result in pointer arithmetic.

CVE-2021-43618

facebook/zstd#2424

[MathiasVP]

Hi @ihsinme,

Thanks for another contribution. I really like this one :) Will review it very soon!

[MathiasVP]

Here are my initial comments.

[MathiasVP]

Can you add a comment around here that explains why this query catches things that cpp/integer-multiplication-cast-to-long doesn't catch?

[ihsinme]

how do you view this text?

// Search for an explicit conversion of the result of multiplication in the situation of multiplying types of a smaller size than the result 
// or casting the result of multiplication of comparable types to signed ones.
// In these situations, it is necessary to use the conversion not of the result, but of the arguments.

[MathiasVP]

That still doesn't explain (at least not to me) what the difference between this query and cpp/integer-multiplication-cast-to-long is. As far as I remember, the goal of cpp/integer-multiplication-cast-to-long is also to detect cases like:

int i1 = ...;
int i2 = ...;
long j = i * i;

where there's an implicit cast on the result of the multiplication, but where the cast should really be on the arguments.

Is the difference just that this query requires the cast to be explicit, while cpp/integer-multiplication-cast-to-long requires the cast to be implicit?

[ihsinme]

yes, the idea of the request is that the developer uses an explicit override, which would have happened anyway, therefore, this is either stylistically incorrect or overflow.

[MathiasVP]

I see. Thanks for clearing that up. Could you extend the description you wrote here to say that this query finds cases similar to the ones found by cpp/integer-multiplication-cast-to-long, but requires the cast to be explicit?

[MathiasVP]

We have a range analysis library that can rule out cases that obviously doesn't overflow. See how it's used here: https://github.com/github/codeql/blob/main/cpp/ql/src/Likely%20Bugs/Arithmetic/SignedOverflowCheck.ql#L30 (there's only a convertedExprMightOverflow predicate that checks for both overflow and underflow at the same time). Would that be useful to rule out false positives in some cases?

[ihsinme]

I am considering this idea.
in my experience of using Range there are too many surprises of uncertainty.

in the first part, I hope for a difference in the sizes of the types of arguments and results, in the second time, that the signed result is 1 bit less than the arguments.

I propose to return to this point if other cleaning options are exhausted?

[MathiasVP]

in my experience of using Range there are too many surprises of uncertainty.

I'd be interested in hearing what you mean by this.

I propose to return to this point if other cleaning options are exhausted?

That's fine.

[ihsinme]

thanks for the comments.
I will try to fix everything as soon as possible.

separately sorry for repeated mistakes.

[MathiasVP]

Here are the CI failures:

Executing tests in /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.
2022-03-03T16:33:32.9863889Z --- expected
2022-03-03T16:33:32.9864202Z +++ actual
2022-03-03T16:33:32.9864550Z @@ -1,4 +1,4 @@
2022-03-03T16:33:32.9865409Z -| test.cpp:9:8:9:12 | ... * ... | possible signed overflow followed by offset of the pointer out of bounds |
2022-03-03T16:33:32.9866305Z -| test.cpp:13:24:13:28 | ... * ... | this transformation is applied after multiplication |
2022-03-03T16:33:32.9876401Z -| test.cpp:16:28:16:32 | ... * ... | this transformation is applied after multiplication |
2022-03-03T16:33:32.9877278Z -| test.cpp:19:22:19:26 | ... * ... | this transformation is applied after multiplication |
2022-03-03T16:33:32.9878102Z +| test.cpp:9:8:9:12 | ... * ... | Possible signed overflow followed by offset of the pointer out of bounds. |
2022-03-03T16:33:32.9878849Z +| test.cpp:13:24:13:28 | ... * ... | This transformation is applied after multiplication. |
2022-03-03T16:33:32.9879567Z +| test.cpp:16:28:16:32 | ... * ... | This transformation is applied after multiplication. |
2022-03-03T16:33:32.9880271Z +| test.cpp:19:22:19:26 | ... * ... | This transformation is applied after multiplication. |
2022-03-03T16:33:32.9891524Z ##[error][2999/5383 comp 6.6s eval 171ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.qlref

[MathiasVP]

LGTM!