CPP: Add query for CWE-125 Out-of-bounds Read with different interpretation of the string when use mbtowc #9089

ihsinme · 2022-05-09T13:18:07Z

in this request, I'm looking for confusion when working with function mbtowc. as practice has shown, we can often run abroad.

CVE-2019-19221

MathiasVP · 2022-05-19T10:52:03Z

Hi @ihsinme,

Thanks for another contribution. We'll review it very soon!

geoffw0

The qhelp looks fine, I want to clear up my understanding of the tests, then I will review the QL.

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousUseMbtowc.qhelp

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/test.cpp

geoffw0 · 2022-05-24T09:01:01Z

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/test.cpp

+  int len = 9;
+  for (wchar_t wc; (ret = mbtowc(&wc, ptr, 16)) > 0; len-=ret) { // GOOD


Should this be:

Suggested change

int len = 9;

for (wchar_t wc; (ret = mbtowc(&wc, ptr, 16)) > 0; len-=ret) { // GOOD

int len = 10;

for (wchar_t wc; (ret = mbtowc(&wc, ptr, len)) > 0; len-=ret) { // GOOD

(if not, what is the intent here?)

in this and the next situation(goodTest2,goodTest3,goodTest4), there is an indication of the presence of zero at the end of the buffer, so the overflow will not occur. But if the read value is less than the maximum length of this character, then you can get unpredictable results, like badTest1,badTest2 here.
therefore it is necessary to specify some maximum character size value.

I think I understand now, the rules / assumptions are:

if the input is not null terminated, we must specify the size of the input to prevent overflow.

if the input is null terminated, overflow is not a concern because the 0 will prevent it.

but if the size parameter is less than MB_LEN_MAX, mbtowc could fail, so the size parameter should be >= MB_LEN_MAX (or MB_CUR_MAX).

the return value of mbtowc should only be used to update the input pointer, not the output pointer.

thanks for the clear wording.
If I could write it right away, it would be easier to understand.
I'm sorry.

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/test.cpp

geoffw0 · 2022-05-24T09:28:37Z

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/test.cpp

+      break;
+    len-=ret;
+    ptr+=ret;
+    wc++;


I don't understand this example. I first assumed wc is an array of wide characters and wc_len is its length - but then why are we passing it (via len = wc_len) it into mbtowc as the length of the array of byte characters, ptr?

in the case of using this buffer length ptr, I just looked at CVE for a long time and wanted to reduce the FP.

If you want I can change this name?

Ah I think I see. In the CVE they allocate a wide string with length equal to the length of the input string, because each character of input produces at most one wide character of output. This might be clearer if you allocate wc here rather than taking it as a parameter, something like:

wc_len = len; // sufficient because each character of input produces at most one wide character of output wchar_t *wc = new wchar_t[wc_len];

do you want to see it in this particular example or in all of them?

It looks like the same applies to goodTest5 (this case), badTest3, badTest4, badTest5, badTest6 and badTest7. I think.

I'm sorry to be late with the reply.
I fixed it.

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-125/semmle/tests/test.cpp

…c.qhelp Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>

ihsinme · 2022-07-04T08:22:17Z

@geoffw0 look when you have time.
I expanded the query.
added

for functions "mbstowcs","_mbstowcs_l","mbsrtowcs","MultiByteToWideChar","WideCharToMultiByte"

undefined behavior when crossing buffers
undefined behavior with a null destination buffer
situations of absence of zero termination
memory size calculation error

for work with character conversion

direct copy errors
error when establishing termination

show me the work for all test files so that i can add the correct result.

geoffw0 · 2022-07-12T16:24:16Z

show me the work for all test files so that i can add the correct result.

I've added the results from the test runner here: ihsinme#1

C++: Accept test results.

geoffw0

This query is doing too many different things now, and some of them don't correspond with the query name or qhelp provided. Its difficult to review and will be difficult to maintain and use. I think it needs more focus, or breaking up into smaller queries.

A quick variant analysis run found one result in the wild, that was for the case: "Access beyond the allocated memory is possible, the length can change without changing the pointer." (in pwsafe).

geoffw0 · 2022-07-12T17:43:28Z

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousUseMbtowc.ql

+    arrayExpr.getArrayBase().(VariableAccess).getTarget().getADeclarationEntry().getType() =
+      arrayType and


Can this be simplified to:

arrayExpr.getArrayBase().getType() = arrayType and

you need to think, probably this will add to the detection
situation a[]={1,2,3}, I'll see if we don't lose anything, I'll fix it.

Thank you.

On test examples, I did not find a difference and made your suggestion into the code.

geoffw0 · 2022-07-12T17:50:09Z

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousUseMbtowc.ql

+      ) and
+      arrayType.getArraySize() != arrayType.getByteSize() and
+      msg =
+        "The size of the array element is greater than one byte, so the offset will point outside the array."


This case seems reasonable.

this is an attempt to add index confusion detection when element size is not 1

geoffw0 · 2022-07-12T17:50:18Z

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousUseMbtowc.ql

+        loop.getCondition().(LTExpr).getRightOperand() = sizeofArray
+      ) and
+      msg =
+        "This buffer may contain multibyte characters, so attempting to copy may result in part of the last character being lost."


I can't tell what's going on here.

I thought you saw the post here github/securitylab#688 (comment) and noticed the apparent symmetry with the test file, so I didn't explicitly state it. this is an attempt to add the situation described in https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2008/c5t35k3c(v=vs.90).

geoffw0 · 2022-07-12T17:53:21Z

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousUseMbtowc.ql

-      "Access beyond the allocated memory is possible, the length can change without changing the pointer."
-    or
-    exists(AssignPointerAddExpr aetmp |
+      "This buffer may contain multibyte characters, so an attempt to copy may result in an overflow."


same as above

ihsinme · 2022-07-17T12:45:03Z

You're right, I missed the changes for qhelp and the name. how do you look at the following correction options.

DangerousUseMbtowc to DangerousWorksWithMultibyteOrWideCharacters

Using function <code>mbtowc</code> with an invalid length argument can result in an out-of-bounds access error or unexpected result. If you are sure you are working with a null-terminated string, use the length macros, if not, use the correctly computed length.

Using a function to convert multibyte or wide characters with an invalid length argument may result in an out-of-range access error or unexpected results.

I also agree that the query has gotten bigger (although it still considers one error class), but I hope you guide me and I'll combine the predicates into either classes or a common import.

ihsinme · 2022-08-03T06:35:45Z

good afternoon @geoffw0.
any news on this PR?

geoffw0 · 2022-08-08T13:04:43Z

Hi @ihsinme .

I have no further suggestions for improvements here.

It looks like the checks have failed because DangerousUseMbtowc.ql needs autoformatting. Once that's fixed I will merge.

…racters.cpp

…haracters.qhelp

…deCharacters.expected

…byteOrWideCharacters.qlref

…acters.ql

ihsinme · 2022-08-08T16:48:17Z

Hi @geoffw0.
I added corrections, including the text corrections I wrote about above.

Can you start checking?

cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql

+  exists(FunctionCall fc |
+    fc = exp and
+    (
+      exists(Loop lptmp | lptmp = fc.getEnclosingStmt().getParentStmt*()) and


cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql

+}
+
+/** Holds if expression is constant or operator call `sizeof`. */
+predicate argConstOrSizeof(Expr exp) {


cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql

+          mbFunction.getTarget().getName().matches("_mbs%") or
+          mbFunction.getTarget().getName().matches("mbs%") or
+          mbFunction.getTarget().getName().matches("_mbc%") or
+          mbFunction.getTarget().getName().matches("mbc%")


geoffw0

Thanks.

The checks have all passed. I'll merge this now.

create new branchihsinme-patch-87 in fork

a7c69ba

ihsinme requested a review from a team as a code owner May 9, 2022 13:18

github-actions bot added C++ documentation labels May 9, 2022

geoffw0 self-requested a review May 24, 2022 08:25

geoffw0 self-assigned this May 24, 2022

geoffw0 reviewed May 24, 2022

View reviewed changes

ihsinme and others added 4 commits May 25, 2022 09:38

Update cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousUseMbtow…

57127a5

…c.qhelp Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>

Update DangerousUseMbtowc.expected

872dd0d

Update test.cpp

77e4d05

Update DangerousUseMbtowc.ql

9d12f1b

ihsinme mentioned this pull request Jun 9, 2022

[CPP]: Add query for CWE-125 Out-of-bounds Read with different interpretation of the string when use mbtowc github/securitylab#688

Closed

2 tasks

ihsinme added 5 commits July 4, 2022 11:10

Update DangerousUseMbtowc.ql

f53adca

Create test1.cpp

6d800de

Create test2.cpp

1ce42dc

Create test3.cpp

4e28887

Update DangerousUseMbtowc.ql

8967f57

C++: Accept test results.

f29104c

geoffw0 mentioned this pull request Jul 12, 2022

C++: Accept test results. ihsinme/codeql#1

Merged

ihsinme added 3 commits July 12, 2022 20:18

Merge pull request #1 from geoffw0/test123

1291f33

C++: Accept test results.

Update DangerousUseMbtowc.ql

98af52f

Update DangerousUseMbtowc.expected

e77a989

geoffw0 reviewed Jul 13, 2022

View reviewed changes

Update DangerousUseMbtowc.ql

96e2205

ihsinme added 8 commits August 8, 2022 18:35

Update DangerousUseMbtowc.qhelp

02bea35

Rename DangerousUseMbtowc.cpp to DangerousWorksWithMultibyteOrWideCha…

5ee4993

…racters.cpp

Rename DangerousUseMbtowc.qhelp to DangerousWorksWithMultibyteOrWideC…

ef04b8f

…haracters.qhelp

Rename DangerousUseMbtowc.expected to DangerousWorksWithMultibyteOrWi…

bce395f

…deCharacters.expected

Update and rename DangerousUseMbtowc.qlref to DangerousWorksWithMulti…

9b5154f

…byteOrWideCharacters.qlref

Rename DangerousUseMbtowc.ql to DangerousWorksWithMultibyteOrWideChar…

7cbf79b

…acters.ql

Update DangerousWorksWithMultibyteOrWideCharacters.qhelp

212b103

Update DangerousWorksWithMultibyteOrWideCharacters.ql

4fdf4b2

github-advanced-security bot found potential problems Aug 9, 2022

View reviewed changes

geoffw0 approved these changes Aug 9, 2022

View reviewed changes

geoffw0 merged commit db8a310 into github:main Aug 9, 2022

		int len = 9;
		for (wchar_t wc; (ret = mbtowc(&wc, ptr, 16)) > 0; len-=ret) { // GOOD

		arrayExpr.getArrayBase().(VariableAccess).getTarget().getADeclarationEntry().getType() =
		arrayType and

CPP: Add query for CWE-125 Out-of-bounds Read with different interpretation of the string when use mbtowc #9089

CPP: Add query for CWE-125 Out-of-bounds Read with different interpretation of the string when use mbtowc #9089

Uh oh!

Conversation

ihsinme commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MathiasVP commented May 19, 2022

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ihsinme commented Jul 4, 2022

Uh oh!

geoffw0 commented Jul 12, 2022

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihsinme commented Jul 17, 2022

Uh oh!

ihsinme commented Aug 3, 2022

Uh oh!

geoffw0 commented Aug 8, 2022

Uh oh!

ihsinme commented Aug 8, 2022

Uh oh!

Check warning

Check warning

Check notice

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ihsinme commented May 9, 2022 •

edited

Loading