From 253b8d1287613cababa457689dcd7f71d2a77870 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Mon, 1 Oct 2018 10:25:49 -0700 Subject: [PATCH 1/3] C++ : cpp/incorrect-string-type-conversion Cast between semantically different string types: char* from/to wchar_t* NOTE: Please let me know if you want to use a different CWE than CWE-704 --- .gitignore | 1 + .../CWE/CWE-704/WcharCharConversion.cpp | 3 ++ .../CWE/CWE-704/WcharCharConversion.qhelp | 25 ++++++++++++ .../CWE/CWE-704/WcharCharConversion.ql | 38 ++++++++++++++++++ .../CWE/CWE-704/WcharCharConversion.cpp | 40 +++++++++++++++++++ .../CWE/CWE-704/WcharCharConversion.expected | 8 ++++ .../CWE/CWE-704/WcharCharConversion.qlref | 1 + 7 files changed, 116 insertions(+) create mode 100644 cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp create mode 100644 cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp create mode 100644 cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.qlref diff --git a/.gitignore b/.gitignore index 7e82b2f488ca..671c5aefe0b1 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,4 @@ /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json +/.vs/ql/v15/.suo diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp new file mode 100644 index 000000000000..cca43de59b34 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp @@ -0,0 +1,3 @@ +LPWSTR pSrc; + +pSrc = (LPWSTR)"a"; \ No newline at end of file diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp new file mode 100644 index 000000000000..e3656e7101ac --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp @@ -0,0 +1,25 @@ + + + + +

This rule indicates a potentially incorrect cast from/to an ANSI string (char *) to/from a Unicode string (wchar_t *).

+

This cast might yield strings that are not correctly terminated; including potential buffer overruns when using such strings with some dangerous APIs.

+ + + +

Do not explicitly casting ANSI strings to/from Unicode strings.

+ + + +

In the following example, an ANSI string literal ("a") is casted as a Unicode string.

+ + +

To fix this issue, prepend the literal with the letter "L" (L"a") to define it as a Unicode string.

+ + + + + + diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql new file mode 100644 index 000000000000..1178079286bf --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql @@ -0,0 +1,38 @@ +/** + * @name Cast between semantically different string types: char* from/to wchar_t* + * @description This rule indicates a potentially incorrect cast from/to an ANSI string (char *) to/from a Unicode string (wchar_t *). + * This cast might yield strings that are not correctly terminated; + * including potential buffer overruns when using such strings with some dangerous APIs. + * @kind problem + * @id cpp/incorrect-string-type-conversion + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-704 + * external/microsoft/c/c6276 + */ +import cpp + +class WideCharPointerType extends PointerType { + WideCharPointerType() { + this.getBaseType() instanceof WideCharType + } +} + +from Expr e1, Cast e2 +where + e2 = e1.getConversion() + and + ( + exists( WideCharPointerType w, CharPointerType c | + w = e1.getType().getUnspecifiedType().(PointerType) + and c = e2.getType().getUnspecifiedType().(PointerType) + ) + or exists + ( + WideCharPointerType w, CharPointerType c | + w = e2.getType().getUnspecifiedType().(PointerType) + and c = e1.getType().getUnspecifiedType().(PointerType) + ) + ) +select e1, "Conversion from " + e1.getType().toString() + " to " + e2.getType().toString() + ". Use of invalid string can lead to undefined behavior." \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp new file mode 100644 index 000000000000..9cadc6c84740 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp @@ -0,0 +1,40 @@ +#define NULL 0 +#define CONST const +typedef wchar_t WCHAR; // wc, 16-bit UNICODE character +typedef char CHAR; + +typedef WCHAR *LPWSTR; +typedef CONST WCHAR *LPCWSTR; + +typedef CHAR *LPSTR; +typedef CONST CHAR *LPCSTR; + +void fconstChar(LPCSTR p) {} +void fChar(LPSTR p) {} +void fconstWChar(LPCWSTR p) {} +void fWChar(LPWSTR p) {} + +void Test() +{ + char *lpChar = NULL; + wchar_t *lpWchar = NULL; + + lpChar = (LPSTR)L"a"; // BUG + lpWchar = (LPWSTR)"a"; // BUG + + lpChar = (char*)lpWchar; // BUG + lpWchar = (wchar_t*)lpChar; // BUG + + fconstChar((LPCSTR)lpWchar); // BUG + fChar((LPSTR)lpWchar); // BUG + fconstWChar((LPCWSTR)lpChar); // BUG + fWChar((LPWSTR)lpChar); // BUG + + lpChar = (LPSTR)"a"; // Valid + lpWchar = (LPWSTR)L"a"; // Valid + + fconstChar((LPCSTR)lpChar); // Valid + fChar(lpChar); // Valid + fconstWChar((LPCWSTR)lpWchar); // Valid + fWChar(lpWchar); // Valid +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected new file mode 100644 index 000000000000..8a2af894c39c --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected @@ -0,0 +1,8 @@ +| WcharCharConversion.cpp:22:18:22:21 | array to pointer conversion | Conversion from const wchar_t * to LPSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:23:20:23:22 | array to pointer conversion | Conversion from const char * to LPWSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:25:18:25:24 | lpWchar | Conversion from wchar_t * to char *. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:26:22:26:27 | lpChar | Conversion from char * to wchar_t *. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:28:21:28:27 | lpWchar | Conversion from wchar_t * to LPCSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:29:15:29:21 | lpWchar | Conversion from wchar_t * to LPSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:30:23:30:28 | lpChar | Conversion from char * to LPCWSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:31:17:31:22 | lpChar | Conversion from char * to LPWSTR. Use of invalid string can lead to undefined behavior. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.qlref new file mode 100644 index 000000000000..4e3b6775188e --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-704/WcharCharConversion.ql \ No newline at end of file From 230724c0851502d8934bf088969c4464f2e93609 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Tue, 2 Oct 2018 11:17:23 -0700 Subject: [PATCH 2/3] Updates based on feedback --- .gitignore | 1 + .../CWE/CWE-704/WcharCharConversion.cpp | 4 +-- .../CWE/CWE-704/WcharCharConversion.qhelp | 18 ++++++++--- .../CWE/CWE-704/WcharCharConversion.ql | 17 +++-------- .../CWE/CWE-704/WcharCharConversion.cpp | 30 ++++++++----------- .../CWE/CWE-704/WcharCharConversion.expected | 13 ++++---- 6 files changed, 38 insertions(+), 45 deletions(-) diff --git a/.gitignore b/.gitignore index 671c5aefe0b1..72882ff86a83 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,4 @@ /.vs/ProjectSettings.json /.vs/ql/v15/.suo +/.vs/VSWorkspaceState.json diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp index cca43de59b34..0f4a819cc85b 100644 --- a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.cpp @@ -1,3 +1,3 @@ -LPWSTR pSrc; +wchar_t* pSrc; -pSrc = (LPWSTR)"a"; \ No newline at end of file +pSrc = (wchar_t*)"a"; // casting a byte-string literal "a" to a wide-character string \ No newline at end of file diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp index e3656e7101ac..270b2141f40d 100644 --- a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.qhelp @@ -4,22 +4,32 @@ -

This rule indicates a potentially incorrect cast from/to an ANSI string (char *) to/from a Unicode string (wchar_t *).

+

This rule indicates a potentially incorrect cast from an byte string (char *) to a wide-character string (wchar_t *).

This cast might yield strings that are not correctly terminated; including potential buffer overruns when using such strings with some dangerous APIs.

 -

Do not explicitly casting ANSI strings to/from Unicode strings.

+

Do not explicitly cast byte strings to wide-character strings.

+

For string literals, prepend the literal string with the letter "L" to indicate that the string is a wide-character string (wchar_t *).

+

For converting a byte literal to a wide-character string literal, you would need to use the appropriate conversion function for the platform you are using. Please see the references section for options according to your platform.

 -

In the following example, an ANSI string literal ("a") is casted as a Unicode string.

+

In the following example, an byte string literal ("a") is cast to a wide-character string.

-

To fix this issue, prepend the literal with the letter "L" (L"a") to define it as a Unicode string.

+

To fix this issue, prepend the literal with the letter "L" (L"a") to define it as a wide-character string.

 +
  • + General resources: + std::mbstowcs +
    • +
  • + Microsoft specific resources: + Security Considerations: International Features +
    • diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql index 1178079286bf..42687a7b07c3 100644 --- a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql @@ -21,18 +21,9 @@ class WideCharPointerType extends PointerType { from Expr e1, Cast e2 where - e2 = e1.getConversion() - and - ( - exists( WideCharPointerType w, CharPointerType c | - w = e1.getType().getUnspecifiedType().(PointerType) - and c = e2.getType().getUnspecifiedType().(PointerType) - ) - or exists - ( - WideCharPointerType w, CharPointerType c | - w = e2.getType().getUnspecifiedType().(PointerType) - and c = e1.getType().getUnspecifiedType().(PointerType) - ) + e2 = e1.getConversion() and + exists(WideCharPointerType w, CharPointerType c | + w = e2.getType().getUnspecifiedType().(PointerType) and + c = e1.getType().getUnspecifiedType().(PointerType) ) select e1, "Conversion from " + e1.getType().toString() + " to " + e2.getType().toString() + ". Use of invalid string can lead to undefined behavior." \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp index 9cadc6c84740..d03b26f04e2a 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.cpp @@ -9,32 +9,26 @@ typedef CONST WCHAR *LPCWSTR; typedef CHAR *LPSTR; typedef CONST CHAR *LPCSTR; -void fconstChar(LPCSTR p) {} -void fChar(LPSTR p) {} void fconstWChar(LPCWSTR p) {} void fWChar(LPWSTR p) {} void Test() { - char *lpChar = NULL; - wchar_t *lpWchar = NULL; + char *lpChar = NULL; + wchar_t *lpWchar = NULL; + LPCSTR lpcstr = "b"; - lpChar = (LPSTR)L"a"; // BUG - lpWchar = (LPWSTR)"a"; // BUG + lpWchar = (LPWSTR)"a"; // BUG + lpWchar = (LPWSTR)lpcstr; // BUG - lpChar = (char*)lpWchar; // BUG - lpWchar = (wchar_t*)lpChar; // BUG + lpWchar = (wchar_t*)lpChar; // BUG - fconstChar((LPCSTR)lpWchar); // BUG - fChar((LPSTR)lpWchar); // BUG - fconstWChar((LPCWSTR)lpChar); // BUG - fWChar((LPWSTR)lpChar); // BUG + fconstWChar((LPCWSTR)lpChar); // BUG + fWChar((LPWSTR)lpChar); // BUG - lpChar = (LPSTR)"a"; // Valid - lpWchar = (LPWSTR)L"a"; // Valid + lpChar = (LPSTR)"a"; // Valid + lpWchar = (LPWSTR)L"a"; // Valid - fconstChar((LPCSTR)lpChar); // Valid - fChar(lpChar); // Valid - fconstWChar((LPCWSTR)lpWchar); // Valid - fWChar(lpWchar); // Valid + fconstWChar((LPCWSTR)lpWchar); // Valid + fWChar(lpWchar); // Valid } \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected index 8a2af894c39c..73787d4f6eb9 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-704/WcharCharConversion.expected @@ -1,8 +1,5 @@ -| WcharCharConversion.cpp:22:18:22:21 | array to pointer conversion | Conversion from const wchar_t * to LPSTR. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:23:20:23:22 | array to pointer conversion | Conversion from const char * to LPWSTR. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:25:18:25:24 | lpWchar | Conversion from wchar_t * to char *. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:26:22:26:27 | lpChar | Conversion from char * to wchar_t *. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:28:21:28:27 | lpWchar | Conversion from wchar_t * to LPCSTR. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:29:15:29:21 | lpWchar | Conversion from wchar_t * to LPSTR. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:30:23:30:28 | lpChar | Conversion from char * to LPCWSTR. Use of invalid string can lead to undefined behavior. | -| WcharCharConversion.cpp:31:17:31:22 | lpChar | Conversion from char * to LPWSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:21:20:21:22 | array to pointer conversion | Conversion from const char * to LPWSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:22:20:22:25 | lpcstr | Conversion from LPCSTR to LPWSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:24:22:24:27 | lpChar | Conversion from char * to wchar_t *. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:26:23:26:28 | lpChar | Conversion from char * to LPCWSTR. Use of invalid string can lead to undefined behavior. | +| WcharCharConversion.cpp:27:17:27:22 | lpChar | Conversion from char * to LPWSTR. Use of invalid string can lead to undefined behavior. | From 3873cbdde016f4a0d42bb5c107b54fb034b7d4d8 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Wed, 3 Oct 2018 15:32:34 -0700 Subject: [PATCH 3/3] Chnaging the @name & @description. --- cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql index 42687a7b07c3..b351a8069565 100644 --- a/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql +++ b/cpp/ql/src/Security/CWE/CWE-704/WcharCharConversion.ql @@ -1,8 +1,8 @@ /** - * @name Cast between semantically different string types: char* from/to wchar_t* - * @description This rule indicates a potentially incorrect cast from/to an ANSI string (char *) to/from a Unicode string (wchar_t *). - * This cast might yield strings that are not correctly terminated; - * including potential buffer overruns when using such strings with some dangerous APIs. + * @name Cast from char* to wchar_t* + * @description Casting a byte string to a wide-character string is likely + * to yield a string that is incorrectly terminated or aligned. + * This can lead to undefined behavior, including buffer overruns. * @kind problem * @id cpp/incorrect-string-type-conversion * @problem.severity error