From 85283d63cec21a342b7367e0a1ed2f2e2b387c93 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Fri, 12 Oct 2018 15:57:01 -0700 Subject: [PATCH 1/7] C++ : NULL application name with an unquoted path in call to CreateProcess Calling a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces. --- .gitignore | 1 + cpp/config/suites/security/cwe-428 | 3 + cpp/config/suites/security/default | 1 + .../CWE/CWE-428/UnsafeCreateProcessCall.cpp | 11 + .../CWE/CWE-428/UnsafeCreateProcessCall.qhelp | 46 ++ .../CWE/CWE-428/UnsafeCreateProcessCall.ql | 135 +++++ .../CWE/CWE-428/UnsafeCreateProcessCall.cpp | 503 ++++++++++++++++++ .../CWE-428/UnsafeCreateProcessCall.expected | 13 + .../CWE/CWE-428/UnsafeCreateProcessCall.qlref | 1 + 9 files changed, 714 insertions(+) create mode 100644 cpp/config/suites/security/cwe-428 create mode 100644 cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp create mode 100644 cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp create mode 100644 cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.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/config/suites/security/cwe-428 b/cpp/config/suites/security/cwe-428 new file mode 100644 index 000000000000..bef395b717a9 --- /dev/null +++ b/cpp/config/suites/security/cwe-428 @@ -0,0 +1,3 @@ +# CWE-428: Unquoted Search Path or Element ++ semmlecode-cpp-queries/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql: /CWE/CWE-428 + @name NULL application name with an unquoted path in call to CreateProcess (CWE-428) diff --git a/cpp/config/suites/security/default b/cpp/config/suites/security/default index 4225ae9808b1..b61734396ff6 100644 --- a/cpp/config/suites/security/default +++ b/cpp/config/suites/security/default @@ -18,6 +18,7 @@ @import "cwe-327" @import "cwe-367" @import "cwe-416" +@import "cwe-428" @import "cwe-457" @import "cwe-468" @import "cwe-676" diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp new file mode 100644 index 000000000000..62d2af1d17ae --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp @@ -0,0 +1,11 @@ +STARTUPINFOW si; +PROCESS_INFORMATION pi; + +// ... + +CreateProcessW( // BUG + NULL, // lpApplicationName + (LPWSTR)L"C:\\Program Files\\MyApp", // lpCommandLine + NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); + +// ... \ No newline at end of file diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp new file mode 100644 index 000000000000..9717c416e557 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp @@ -0,0 +1,46 @@ + + + + +

This query indicates that there is a call to a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces.

+ + + +

Do not use NULL for the lpApplicationName argument to the CreateProcess* function.

+

If you pass NULL for lpApplicationName, use quotation marks around the executable path in lpCommandLine.

+ + + +

In the following example, CreateProcessW is called with a NULL value for lpApplicationName, +and the value for lpCommandLine that represent the application path is not quoted and has spaces int.

+

If an attacker has access to the file system, it is possible to elevate privileges by creating a file such as "C:\Program.exe" that will be executed instead of the intended application.

+ + +

To fix this issue, specify a valid string for lpApplicationName, or quote the path for lpCommandLine. For example:

+

(LPWSTR)L"\"C:\\Program Files\\MyApp\"", // lpCommandLine

+ + + +
  • + CreateProcessA function (Microsoft documentation). +
    • +
  • + CreateProcessW function (Microsoft documentation). +
    • +
  • + CreateProcessAsUserA function (Microsoft documentation). +
    • +
  • + CreateProcessAsUserW function (Microsoft documentation). +
    • +
  • + CreateProcessWithLogonW function (Microsoft documentation). +
    • +
  • + CreateProcessWithTokenW function (Microsoft documentation). +
    • +     + +     diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql new file mode 100644 index 000000000000..7ff62a1cd868 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql @@ -0,0 +1,135 @@ +/** + * @name NULL application name with an unquoted path in call to CreateProcess + * @description Calling a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces. + * @id cpp/unsafe-create-process-call + * @kind problem + * @problem.severity error + * @precision medium + * @tags security + * external/cwe/cwe-428 + * external/microsoft/C6277 + */ + +import cpp +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.dataflow.DataFlow2 + +/** + * A function call to CreateProcess (either wide-char or single byte string versions) + */ +class CreateProcessFunctionCall extends FunctionCall { + CreateProcessFunctionCall() { + ( + this.getTarget().hasGlobalName("CreateProcessA") or + this.getTarget().hasGlobalName("CreateProcessW") or + this.getTarget().hasGlobalName("CreateProcessWithTokenW") or + this.getTarget().hasGlobalName("CreateProcessWithLogonW") or + this.getTarget().hasGlobalName("CreateProcessAsUserA") or + this.getTarget().hasGlobalName("CreateProcessAsUserW") + ) + } + + int getApplicationNameArgumentId() { + if( + this.getTarget().hasGlobalName("CreateProcessA") or + this.getTarget().hasGlobalName("CreateProcessW") + ) then ( result = 0 ) + else if ( + this.getTarget().hasGlobalName("CreateProcessWithTokenW") + ) then ( result = 2 ) + else if ( + this.getTarget().hasGlobalName("CreateProcessWithLogonW") + ) then ( result = 4 ) + else if( + this.getTarget().hasGlobalName("CreateProcessAsUserA") or + this.getTarget().hasGlobalName("CreateProcessAsUserW") + ) then ( result = 1 ) + else (result = -1 ) + } + + int getCommandLineArgumentId() { + if( + this.getTarget().hasGlobalName("CreateProcessA") or + this.getTarget().hasGlobalName("CreateProcessW") + ) then ( result = 1 ) + else if ( + this.getTarget().hasGlobalName("CreateProcessWithTokenW") + ) then ( result = 3 ) + else if ( + this.getTarget().hasGlobalName("CreateProcessWithLogonW") + ) then ( result = 5 ) + else if( + this.getTarget().hasGlobalName("CreateProcessAsUserA") or + this.getTarget().hasGlobalName("CreateProcessAsUserW") + ) then ( result = 2 ) + else (result = -1 ) + } +} + +/** + * Dataflow that detects a call to CreateProcess with a NULL value for lpApplicationName argument + */ +class NullAppNameCreateProcessFunctionConfiguration extends DataFlow::Configuration { + NullAppNameCreateProcessFunctionConfiguration() { + this = "NullAppNameCreateProcessFunctionConfiguration" + } + + override predicate isSource(DataFlow::Node source) { + nullValue(source.asExpr()) + } + + override predicate isSink(DataFlow::Node sink) { + exists( + CreateProcessFunctionCall call, Expr val | + val = sink.asExpr() | + val = call.getArgument(call.getApplicationNameArgumentId()) + ) + } +} + +/** + * Dataflow that detects a call to CreateProcess with an unquoted commandLine argument + */ +class QuotedCommandInCreateProcessFunctionConfiguration extends DataFlow2::Configuration { + QuotedCommandInCreateProcessFunctionConfiguration() { + this = "QuotedCommandInCreateProcessFunctionConfiguration" + } + + override predicate isSource(DataFlow2::Node source) { + exists( string s | + s = source.asExpr().getValue().toString() + and + not isQuotedApplicationNameOnCmd(s) + ) + } + + override predicate isSink(DataFlow2::Node sink) { + exists( + CreateProcessFunctionCall call, Expr val | + val = sink.asExpr() | + val = call.getArgument(call.getCommandLineArgumentId()) + ) + } +} + +bindingset[s] +predicate isQuotedApplicationNameOnCmd(string s){ + s.regexpMatch("\"([^\"])*\"(\\s|.)*") +} + +from CreateProcessFunctionCall call, string msg1, string msg2 +where + exists( Expr source, Expr appName, + NullAppNameCreateProcessFunctionConfiguration nullAppConfig | + appName = call.getArgument(call.getApplicationNameArgumentId()) + and nullAppConfig.hasFlow(DataFlow2::exprNode(source), DataFlow2::exprNode(appName)) + and msg1 = call.toString() + " with lpApplicationName == NULL (" + appName + ")" + ) + and + exists( Expr source, Expr cmd, + QuotedCommandInCreateProcessFunctionConfiguration quotedConfig | + cmd = call.getArgument(call.getCommandLineArgumentId()) + and quotedConfig.hasFlow(DataFlow2::exprNode(source), DataFlow2::exprNode(cmd)) + and msg2 = " and with an unquoted lpCommandLine (" + cmd + ") may result in a security vulnerability if the path contains spaces." + ) +select call, msg1 + " " + msg2 \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp new file mode 100644 index 000000000000..20508ee5f1b7 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp @@ -0,0 +1,503 @@ +// semmle-extractor-options: --microsoft +#define NULL 0 +#define FALSE 0 +#define far +#define LOGON_WITH_PROFILE 0x00000001 + +typedef char CHAR; +typedef unsigned short WCHAR; +typedef int BOOL; +#define CONST const +typedef CHAR *PCHAR, *LPCH, *PCH; +typedef CONST CHAR *LPCCH, *PCCH; +typedef CHAR *NPSTR, *LPSTR, *PSTR; +typedef CONST PSTR *PCZPSTR; +typedef CONST CHAR *LPCSTR, *PCSTR; +typedef WCHAR *PWCHAR, *LPWCH, *PWCH; +typedef CONST WCHAR *LPCWCH, *PCWCH; +typedef WCHAR *NWPSTR, *LPWSTR, *PWSTR; +typedef PWSTR *PZPWSTR; +typedef CONST PWSTR *PCZPWSTR; +typedef CONST WCHAR *LPCWSTR, *PCWSTR; +typedef unsigned long DWORD; +typedef void far *LPVOID; +typedef unsigned short WORD; +typedef unsigned char BYTE; +typedef BYTE far *LPBYTE; +typedef void *HANDLE; + +typedef struct _SECURITY_ATTRIBUTES { + DWORD nLength; + LPVOID lpSecurityDescriptor; + BOOL bInheritHandle; +} SECURITY_ATTRIBUTES, *PSECURITY_ATTRIBUTES, *LPSECURITY_ATTRIBUTES; + +typedef struct _PROCESS_INFORMATION { + HANDLE hProcess; + HANDLE hThread; + DWORD dwProcessId; + DWORD dwThreadId; +} PROCESS_INFORMATION, *PPROCESS_INFORMATION, *LPPROCESS_INFORMATION; + +typedef struct _STARTUPINFOA { + DWORD cb; + LPSTR lpReserved; + LPSTR lpDesktop; + LPSTR lpTitle; + DWORD dwX; + DWORD dwY; + DWORD dwXSize; + DWORD dwYSize; + DWORD dwXCountChars; + DWORD dwYCountChars; + DWORD dwFillAttribute; + DWORD dwFlags; + WORD wShowWindow; + WORD cbReserved2; + LPBYTE lpReserved2; + HANDLE hStdInput; + HANDLE hStdOutput; + HANDLE hStdError; +} STARTUPINFOA, *LPSTARTUPINFOA; +typedef struct _STARTUPINFOW { + DWORD cb; + LPWSTR lpReserved; + LPWSTR lpDesktop; + LPWSTR lpTitle; + DWORD dwX; + DWORD dwY; + DWORD dwXSize; + DWORD dwYSize; + DWORD dwXCountChars; + DWORD dwYCountChars; + DWORD dwFillAttribute; + DWORD dwFlags; + WORD wShowWindow; + WORD cbReserved2; + LPBYTE lpReserved2; + HANDLE hStdInput; + HANDLE hStdOutput; + HANDLE hStdError; +} STARTUPINFOW, *LPSTARTUPINFOW; + +typedef STARTUPINFOW STARTUPINFO; +typedef LPSTARTUPINFOW LPSTARTUPINFO; + + +BOOL +CreateProcessA( + LPCSTR lpApplicationName, + LPSTR lpCommandLine, + LPSECURITY_ATTRIBUTES lpProcessAttributes, + LPSECURITY_ATTRIBUTES lpThreadAttributes, + BOOL bInheritHandles, + DWORD dwCreationFlags, + LPVOID lpEnvironment, + LPCSTR lpCurrentDirectory, + LPSTARTUPINFOA lpStartupInfo, + LPPROCESS_INFORMATION lpProcessInformation +); + +BOOL +CreateProcessW( + LPCWSTR lpApplicationName, + LPWSTR lpCommandLine, + LPSECURITY_ATTRIBUTES lpProcessAttributes, + LPSECURITY_ATTRIBUTES lpThreadAttributes, + BOOL bInheritHandles, + DWORD dwCreationFlags, + LPVOID lpEnvironment, + LPCWSTR lpCurrentDirectory, + LPSTARTUPINFOW lpStartupInfo, + LPPROCESS_INFORMATION lpProcessInformation +); + +#define CreateProcess CreateProcessW + +BOOL +CreateProcessWithTokenW( + HANDLE hToken, + DWORD dwLogonFlags, + LPCWSTR lpApplicationName, + LPWSTR lpCommandLine, + DWORD dwCreationFlags, + LPVOID lpEnvironment, + LPCWSTR lpCurrentDirectory, + LPSTARTUPINFOW lpStartupInfo, + LPPROCESS_INFORMATION lpProcessInformation +); + +BOOL +CreateProcessWithLogonW( + LPCWSTR lpUsername, + LPCWSTR lpDomain, + LPCWSTR lpPassword, + DWORD dwLogonFlags, + LPCWSTR lpApplicationName, + LPWSTR lpCommandLine, + DWORD dwCreationFlags, + LPVOID lpEnvironment, + LPCWSTR lpCurrentDirectory, + LPSTARTUPINFOW lpStartupInfo, + LPPROCESS_INFORMATION lpProcessInformation +); + +BOOL +CreateProcessAsUserA( + HANDLE hToken, + LPCSTR lpApplicationName, + LPSTR lpCommandLine, + LPSECURITY_ATTRIBUTES lpProcessAttributes, + LPSECURITY_ATTRIBUTES lpThreadAttributes, + BOOL bInheritHandles, + DWORD dwCreationFlags, + LPVOID lpEnvironment, + LPCSTR lpCurrentDirectory, + LPSTARTUPINFOA lpStartupInfo, + LPPROCESS_INFORMATION lpProcessInformation +); + +BOOL +CreateProcessAsUserW( + HANDLE hToken, + LPCWSTR lpApplicationName, + LPWSTR lpCommandLine, + LPSECURITY_ATTRIBUTES lpProcessAttributes, + LPSECURITY_ATTRIBUTES lpThreadAttributes, + BOOL bInheritHandles, + DWORD dwCreationFlags, + LPVOID lpEnvironment, + LPCWSTR lpCurrentDirectory, + LPSTARTUPINFOW lpStartupInfo, + LPPROCESS_INFORMATION lpProcessInformation +); + +#define CreateProcessAsUser CreateProcessAsUserW + +void positiveTestCases() +{ + LPCWSTR lpCommandLine = (LPCWSTR)L"C:\\Program Files\\MyApp"; + HANDLE h = 0; + LPWSTR lpApplicationName = NULL; + + // CreatePorcessA + CreateProcessA( //BUG + NULL, + (LPSTR)"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcessW + CreateProcessW( //BUG + NULL, + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcess + CreateProcess( //BUG + NULL, + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // lpCommandLine as hardcoded variable + CreateProcess( //BUG + NULL, + (LPWSTR)lpCommandLine, + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessWithTokenW + CreateProcessWithTokenW( //BUG + h, + LOGON_WITH_PROFILE, + NULL, + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); + + // CreateProcessWithLogonW + CreateProcessWithLogonW( //BUG + (LPCWSTR)L"UserName", + (LPCWSTR)L"CONTOSO", + (LPCWSTR)L"", + LOGON_WITH_PROFILE, + NULL, + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUserA + CreateProcessAsUserA( //BUG + h, + NULL, + (LPSTR)"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUserW + CreateProcessAsUserW( //BUG + h, + NULL, + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUser + CreateProcessAsUser( //BUG + h, + NULL, + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcess with a hardcoded variable for application Name (NULL) + CreateProcess( //BUG + lpApplicationName, + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); +} + +void PositiveTestCasesWithCmdLineParameter(LPWSTR lpCommandLine) +{ + // lpCommandLine as variable + CreateProcess( //BUG - Depends on the caller + NULL, + lpCommandLine, + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); +} + +void PositiveTestCasesWithCmdLineParameter_caller() +{ + PositiveTestCasesWithCmdLineParameter((LPWSTR)L"C:\\Program Files\\MyApp"); +} + +// NOTE: This function will not be flagged as having a bug by this rule. +// but as it is, the function can still be misused +void FalseNegativeTestCasesWithCmdLineParameter(LPWSTR lpCommandLine) +{ + // lpCommandLine as variable + CreateProcess( //Depends on the caller, this time the caller will quote + NULL, + lpCommandLine, + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); +} + +void FalseNegativeTestCasesWithCmdLineParameter_caller() +{ + // No bug - escaped command line + // But compare with "PositiveTestCasesWithCmdLineParameter" + FalseNegativeTestCasesWithCmdLineParameter((LPWSTR)L"\"C:\\Program Files\\MyApp\""); +} + +void PositiveTestCasesWithAppNameParameter(LPWSTR lpApplicationName) +{ + HANDLE h = 0; + + CreateProcessWithTokenW( //BUG - Depends on the caller. In this case the caller sends NULL + h, + LOGON_WITH_PROFILE, + lpApplicationName, + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); +} + +void PositiveTestCasesWithAppNameParameter_caller() +{ + PositiveTestCasesWithAppNameParameter(NULL); +} + +// NOTE: This function will not be flagged as having a bug by this rule. +// but as it is, the function can still be misused +void FalseNegativeTestCasesWithAppNameParameter(LPWSTR lpApplicationName) +{ + HANDLE h = 0; + + CreateProcessWithTokenW( // Depends on the caller. In this case the caller sends an ApplicatioName + h, + LOGON_WITH_PROFILE, + lpApplicationName, + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); +} + +void FalseNegativeTestCasesWithAppNameParameter_caller() +{ + // No bug - escaped command line + // But compare with "PositiveTestCasesWithAppNameParameter" + FalseNegativeTestCasesWithAppNameParameter((LPWSTR)L"MyApp.exe"); +} + +bool MayReturnFalse() +{ + // return ((rand() % 2) == 0); + return true; +} + +void TestCaseProbablyBug() +{ + LPCWSTR lpApplicationName = NULL; + + if (!MayReturnFalse()) + { + lpApplicationName = (LPCWSTR)L"app.exe"; + } + + CreateProcessWithLogonW( // BUG (Probably - depends on a condition that may be false) + (LPCWSTR)L"UserName", + (LPCWSTR)L"CONTOSO", + (LPCWSTR)L"", + LOGON_WITH_PROFILE, + (LPWSTR)lpApplicationName, + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); + + if (lpApplicationName) + { + delete[] lpApplicationName; + } +} + +void negativeTestCases_quotedCommandLine() +{ + LPCWSTR lpCommandLine = (LPCWSTR)L"\"C:\\Program Files\\MyApp\" with additional params"; + HANDLE h = 0; + LPWSTR lpApplicationName = NULL; + + // CreatePorcessA + CreateProcessA( + NULL, + (LPSTR)"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcessW + CreateProcessW( + NULL, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcess + CreateProcess( + NULL, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // lpCommandLine as hardcoded variable + CreateProcess( + NULL, + (LPWSTR)lpCommandLine, + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessWithTokenW + CreateProcessWithTokenW( + h, + LOGON_WITH_PROFILE, + NULL, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + 0, NULL, NULL, NULL, NULL); + + // CreateProcessWithLogonW + CreateProcessWithLogonW( + (LPCWSTR)L"UserName", + (LPCWSTR)L"CONTOSO", + (LPCWSTR)L"", + LOGON_WITH_PROFILE, + NULL, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUserA + CreateProcessAsUserA( + h, + NULL, + (LPSTR)"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUserW + CreateProcessAsUserW( + h, + NULL, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUser + CreateProcessAsUser( + h, + NULL, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcess with a hardcoded variable for application Name (NULL) + CreateProcess( + lpApplicationName, + (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); +} + +void negativeTestCases_AppNameSet() +{ + LPCWSTR lpCommandLine = (LPCWSTR)L"C:\\Program Files\\MyApp"; + HANDLE h = 0; + LPCWSTR lpApplicationName = (LPCWSTR)L"MyApp.exe"; + + // CreatePorcessA + CreateProcessA( + (LPSTR)"MyApp.exe", + (LPSTR)"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcessW + CreateProcessW( + (LPWSTR)L"MyApp.exe", + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcess + CreateProcess( + (LPWSTR)L"MyApp.exe", + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // lpCommandLine as hardcoded variable + CreateProcess( + (LPWSTR)L"MyApp.exe", + (LPWSTR)lpCommandLine, + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessWithTokenW + CreateProcessWithTokenW( + h, + LOGON_WITH_PROFILE, + (LPWSTR)L"MyApp.exe", + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); + + // CreateProcessWithLogonW + CreateProcessWithLogonW( + (LPCWSTR)L"UserName", + (LPCWSTR)L"CONTOSO", + (LPCWSTR)L"", + LOGON_WITH_PROFILE, + (LPWSTR)L"MyApp.exe", + (LPWSTR)L"C:\\Program Files\\MyApp", + 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUserA + CreateProcessAsUserA( + h, + (LPSTR)"MyApp.exe", + (LPSTR)"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUserW + CreateProcessAsUserW( + h, + (LPWSTR)L"MyApp.exe", + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreateProcessAsUser + CreateProcessAsUser( + h, + (LPWSTR)L"MyApp.exe", + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // CreatePorcess with a hardcoded variable for application Name (NULL) + CreateProcess( + (LPWSTR)lpApplicationName, + (LPWSTR)L"C:\\Program Files\\MyApp", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); +} diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected new file mode 100644 index 000000000000..1f4519cabb01 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected @@ -0,0 +1,13 @@ +| UnsafeCreateProcessCall.cpp:184:5:184:18 | call to CreateProcessA | call to CreateProcessA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:190:5:190:18 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:196:5:196:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:202:5:202:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:208:5:208:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:216:5:216:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:226:5:226:24 | call to CreateProcessAsUserA | call to CreateProcessAsUserA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:233:5:233:24 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:240:5:240:23 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:247:5:247:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:256:5:256:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:289:5:289:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:338:5:338:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.qlref new file mode 100644 index 000000000000..f2012f0c678d --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-428/UnsafeCreateProcessCall.ql \ No newline at end of file From bc398733b3754618c9428a012b4697ac90b3db8f Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Mon, 15 Oct 2018 13:38:00 -0700 Subject: [PATCH 2/7] Update .gitignore --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index cb811b53cb25..b5c88ce04abf 100644 --- a/.gitignore +++ b/.gitignore @@ -12,4 +12,3 @@ # Visual studio temporaries, except a file used by QL4VS .vs/* !.vs/VSWorkspaceSettings.json - From b8f8c995297a2426f1a04ea13451d7386b114355 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Mon, 15 Oct 2018 13:39:46 -0700 Subject: [PATCH 3/7] Update UnsafeCreateProcessCall.qhelp --- cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp index 9717c416e557..ee3a44cd149c 100644 --- a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp @@ -4,7 +4,7 @@ -

    This query indicates that there is a call to a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces.

    +

    This query indicates that there is a call to a function of the CreateProcess* family of functions, which may result in a security vulnerability if the path contains spaces.

     From 1d853691eb09ce033120222db880564ab9bf07b2 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Mon, 15 Oct 2018 13:40:40 -0700 Subject: [PATCH 4/7] Update UnsafeCreateProcessCall.qhelp --- cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp index ee3a44cd149c..550355e8026d 100644 --- a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp @@ -14,7 +14,7 @@

    In the following example, CreateProcessW is called with a NULL value for lpApplicationName, -and the value for lpCommandLine that represent the application path is not quoted and has spaces int.

    +and the value for lpCommandLine that represent the application path is not quoted and has spaces in it.

    If an attacker has access to the file system, it is possible to elevate privileges by creating a file such as "C:\Program.exe" that will be executed instead of the intended application.

    From cd5e788aa74fe5acc9a240525490880bdad5d051 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Mon, 15 Oct 2018 13:41:21 -0700 Subject: [PATCH 5/7] Update UnsafeCreateProcessCall.ql --- cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql index 7ff62a1cd868..ec355ac9c288 100644 --- a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql @@ -1,6 +1,6 @@ /** * @name NULL application name with an unquoted path in call to CreateProcess - * @description Calling a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces. + * @description Calling a function of the CreateProcess* family of functions, which may result in a security vulnerability if the path contains spaces. * @id cpp/unsafe-create-process-call * @kind problem * @problem.severity error @@ -132,4 +132,4 @@ where and quotedConfig.hasFlow(DataFlow2::exprNode(source), DataFlow2::exprNode(cmd)) and msg2 = " and with an unquoted lpCommandLine (" + cmd + ") may result in a security vulnerability if the path contains spaces." ) -select call, msg1 + " " + msg2 \ No newline at end of file +select call, msg1 + " " + msg2 From 22d54801e5b73dc3b75f2bf930dc9c753492466c Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Mon, 15 Oct 2018 15:53:02 -0700 Subject: [PATCH 6/7] Removed one false-positive scenario (no space on lpCommandLine) Improved the query to avoid multiple calls to hasGlobalName Fixed typos Simplified the test case file --- .../CWE/CWE-428/UnsafeCreateProcessCall.ql | 76 ++-- .../CWE/CWE-428/UnsafeCreateProcessCall.cpp | 379 +++++++----------- .../CWE-428/UnsafeCreateProcessCall.expected | 26 +- 3 files changed, 201 insertions(+), 280 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql index ec355ac9c288..de7685bf5063 100644 --- a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql @@ -14,55 +14,47 @@ import cpp import semmle.code.cpp.dataflow.DataFlow import semmle.code.cpp.dataflow.DataFlow2 +predicate isCreateProcessFunction(FunctionCall call, int applicationNameIndex, int commandLineIndex) { + ( + call.getTarget().hasGlobalName("CreateProcessA") + and applicationNameIndex = 0 + and commandLineIndex = 1 + ) or ( + call.getTarget().hasGlobalName("CreateProcessW") + and applicationNameIndex = 0 + and commandLineIndex = 1 + ) or ( + call.getTarget().hasGlobalName("CreateProcessWithTokenW") + and applicationNameIndex = 2 + and commandLineIndex = 3 + ) or ( + call.getTarget().hasGlobalName("CreateProcessWithLogonW") + and applicationNameIndex = 4 + and commandLineIndex = 5 + ) or ( + call.getTarget().hasGlobalName("CreateProcessAsUserA") + and applicationNameIndex = 1 + and commandLineIndex = 2 + ) or ( + call.getTarget().hasGlobalName("CreateProcessAsUserW") + and applicationNameIndex = 1 + and commandLineIndex = 2 + ) +} /** * A function call to CreateProcess (either wide-char or single byte string versions) */ class CreateProcessFunctionCall extends FunctionCall { CreateProcessFunctionCall() { - ( - this.getTarget().hasGlobalName("CreateProcessA") or - this.getTarget().hasGlobalName("CreateProcessW") or - this.getTarget().hasGlobalName("CreateProcessWithTokenW") or - this.getTarget().hasGlobalName("CreateProcessWithLogonW") or - this.getTarget().hasGlobalName("CreateProcessAsUserA") or - this.getTarget().hasGlobalName("CreateProcessAsUserW") - ) + isCreateProcessFunction( this, _, _) } int getApplicationNameArgumentId() { - if( - this.getTarget().hasGlobalName("CreateProcessA") or - this.getTarget().hasGlobalName("CreateProcessW") - ) then ( result = 0 ) - else if ( - this.getTarget().hasGlobalName("CreateProcessWithTokenW") - ) then ( result = 2 ) - else if ( - this.getTarget().hasGlobalName("CreateProcessWithLogonW") - ) then ( result = 4 ) - else if( - this.getTarget().hasGlobalName("CreateProcessAsUserA") or - this.getTarget().hasGlobalName("CreateProcessAsUserW") - ) then ( result = 1 ) - else (result = -1 ) + isCreateProcessFunction( this, result, _) } int getCommandLineArgumentId() { - if( - this.getTarget().hasGlobalName("CreateProcessA") or - this.getTarget().hasGlobalName("CreateProcessW") - ) then ( result = 1 ) - else if ( - this.getTarget().hasGlobalName("CreateProcessWithTokenW") - ) then ( result = 3 ) - else if ( - this.getTarget().hasGlobalName("CreateProcessWithLogonW") - ) then ( result = 5 ) - else if( - this.getTarget().hasGlobalName("CreateProcessAsUserA") or - this.getTarget().hasGlobalName("CreateProcessAsUserW") - ) then ( result = 2 ) - else (result = -1 ) + isCreateProcessFunction( this, _, result) } } @@ -99,7 +91,7 @@ class QuotedCommandInCreateProcessFunctionConfiguration extends DataFlow2::Confi exists( string s | s = source.asExpr().getValue().toString() and - not isQuotedApplicationNameOnCmd(s) + not isQuotedOrNoSpaceApplicationNameOnCmd(s) ) } @@ -113,8 +105,10 @@ class QuotedCommandInCreateProcessFunctionConfiguration extends DataFlow2::Confi } bindingset[s] -predicate isQuotedApplicationNameOnCmd(string s){ - s.regexpMatch("\"([^\"])*\"(\\s|.)*") +predicate isQuotedOrNoSpaceApplicationNameOnCmd(string s){ + s.regexpMatch("\"([^\"])*\"(\\s|.)*") // The first element (path) is quoted + or + s.regexpMatch("[^\\s]+") // There are no spaces in the string } from CreateProcessFunctionCall call, string msg1, string msg2 diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp index 20508ee5f1b7..6f3f76001b5f 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.cpp @@ -1,207 +1,126 @@ // semmle-extractor-options: --microsoft #define NULL 0 #define FALSE 0 -#define far #define LOGON_WITH_PROFILE 0x00000001 -typedef char CHAR; -typedef unsigned short WCHAR; -typedef int BOOL; -#define CONST const -typedef CHAR *PCHAR, *LPCH, *PCH; -typedef CONST CHAR *LPCCH, *PCCH; -typedef CHAR *NPSTR, *LPSTR, *PSTR; -typedef CONST PSTR *PCZPSTR; -typedef CONST CHAR *LPCSTR, *PCSTR; -typedef WCHAR *PWCHAR, *LPWCH, *PWCH; -typedef CONST WCHAR *LPCWCH, *PCWCH; -typedef WCHAR *NWPSTR, *LPWSTR, *PWSTR; -typedef PWSTR *PZPWSTR; -typedef CONST PWSTR *PCZPWSTR; -typedef CONST WCHAR *LPCWSTR, *PCWSTR; -typedef unsigned long DWORD; -typedef void far *LPVOID; -typedef unsigned short WORD; -typedef unsigned char BYTE; -typedef BYTE far *LPBYTE; -typedef void *HANDLE; - -typedef struct _SECURITY_ATTRIBUTES { - DWORD nLength; - LPVOID lpSecurityDescriptor; - BOOL bInheritHandle; -} SECURITY_ATTRIBUTES, *PSECURITY_ATTRIBUTES, *LPSECURITY_ATTRIBUTES; - -typedef struct _PROCESS_INFORMATION { - HANDLE hProcess; - HANDLE hThread; - DWORD dwProcessId; - DWORD dwThreadId; -} PROCESS_INFORMATION, *PPROCESS_INFORMATION, *LPPROCESS_INFORMATION; - -typedef struct _STARTUPINFOA { - DWORD cb; - LPSTR lpReserved; - LPSTR lpDesktop; - LPSTR lpTitle; - DWORD dwX; - DWORD dwY; - DWORD dwXSize; - DWORD dwYSize; - DWORD dwXCountChars; - DWORD dwYCountChars; - DWORD dwFillAttribute; - DWORD dwFlags; - WORD wShowWindow; - WORD cbReserved2; - LPBYTE lpReserved2; - HANDLE hStdInput; - HANDLE hStdOutput; - HANDLE hStdError; -} STARTUPINFOA, *LPSTARTUPINFOA; -typedef struct _STARTUPINFOW { - DWORD cb; - LPWSTR lpReserved; - LPWSTR lpDesktop; - LPWSTR lpTitle; - DWORD dwX; - DWORD dwY; - DWORD dwXSize; - DWORD dwYSize; - DWORD dwXCountChars; - DWORD dwYCountChars; - DWORD dwFillAttribute; - DWORD dwFlags; - WORD wShowWindow; - WORD cbReserved2; - LPBYTE lpReserved2; - HANDLE hStdInput; - HANDLE hStdOutput; - HANDLE hStdError; -} STARTUPINFOW, *LPSTARTUPINFOW; - -typedef STARTUPINFOW STARTUPINFO; -typedef LPSTARTUPINFOW LPSTARTUPINFO; - - -BOOL +int CreateProcessA( - LPCSTR lpApplicationName, - LPSTR lpCommandLine, - LPSECURITY_ATTRIBUTES lpProcessAttributes, - LPSECURITY_ATTRIBUTES lpThreadAttributes, - BOOL bInheritHandles, - DWORD dwCreationFlags, - LPVOID lpEnvironment, - LPCSTR lpCurrentDirectory, - LPSTARTUPINFOA lpStartupInfo, - LPPROCESS_INFORMATION lpProcessInformation + const char* lpApplicationName, + char* lpCommandLine, + void* lpProcessAttributes, + void* lpThreadAttributes, + int bInheritHandles, + unsigned long dwCreationFlags, + void* lpEnvironment, + const char* lpCurrentDirectory, + void* lpStartupInfo, + void* lpProcessInformation ); -BOOL +int CreateProcessW( - LPCWSTR lpApplicationName, - LPWSTR lpCommandLine, - LPSECURITY_ATTRIBUTES lpProcessAttributes, - LPSECURITY_ATTRIBUTES lpThreadAttributes, - BOOL bInheritHandles, - DWORD dwCreationFlags, - LPVOID lpEnvironment, - LPCWSTR lpCurrentDirectory, - LPSTARTUPINFOW lpStartupInfo, - LPPROCESS_INFORMATION lpProcessInformation + const wchar_t* lpApplicationName, + wchar_t* lpCommandLine, + void* lpProcessAttributes, + void* lpThreadAttributes, + int bInheritHandles, + unsigned long dwCreationFlags, + void* lpEnvironment, + const wchar_t* lpCurrentDirectory, + void* lpStartupInfo, + void* lpProcessInformation ); #define CreateProcess CreateProcessW -BOOL +int CreateProcessWithTokenW( - HANDLE hToken, - DWORD dwLogonFlags, - LPCWSTR lpApplicationName, - LPWSTR lpCommandLine, - DWORD dwCreationFlags, - LPVOID lpEnvironment, - LPCWSTR lpCurrentDirectory, - LPSTARTUPINFOW lpStartupInfo, - LPPROCESS_INFORMATION lpProcessInformation + void* hToken, + unsigned long dwLogonFlags, + const wchar_t* lpApplicationName, + wchar_t* lpCommandLine, + unsigned long dwCreationFlags, + void* lpEnvironment, + const wchar_t* lpCurrentDirectory, + void* lpStartupInfo, + void* lpProcessInformation ); -BOOL +int CreateProcessWithLogonW( - LPCWSTR lpUsername, - LPCWSTR lpDomain, - LPCWSTR lpPassword, - DWORD dwLogonFlags, - LPCWSTR lpApplicationName, - LPWSTR lpCommandLine, - DWORD dwCreationFlags, - LPVOID lpEnvironment, - LPCWSTR lpCurrentDirectory, - LPSTARTUPINFOW lpStartupInfo, - LPPROCESS_INFORMATION lpProcessInformation + const wchar_t* lpUsername, + const wchar_t* lpDomain, + const wchar_t* lpPassword, + unsigned long dwLogonFlags, + const wchar_t* lpApplicationName, + wchar_t* lpCommandLine, + unsigned long dwCreationFlags, + void* lpEnvironment, + const wchar_t* lpCurrentDirectory, + void* lpStartupInfo, + void* lpProcessInformation ); -BOOL +int CreateProcessAsUserA( - HANDLE hToken, - LPCSTR lpApplicationName, - LPSTR lpCommandLine, - LPSECURITY_ATTRIBUTES lpProcessAttributes, - LPSECURITY_ATTRIBUTES lpThreadAttributes, - BOOL bInheritHandles, - DWORD dwCreationFlags, - LPVOID lpEnvironment, - LPCSTR lpCurrentDirectory, - LPSTARTUPINFOA lpStartupInfo, - LPPROCESS_INFORMATION lpProcessInformation + void* hToken, + const char* lpApplicationName, + char* lpCommandLine, + void* lpProcessAttributes, + void* lpThreadAttributes, + int bInheritHandles, + unsigned long dwCreationFlags, + void* lpEnvironment, + const char* lpCurrentDirectory, + void* lpStartupInfo, + void* lpProcessInformation ); -BOOL +int CreateProcessAsUserW( - HANDLE hToken, - LPCWSTR lpApplicationName, - LPWSTR lpCommandLine, - LPSECURITY_ATTRIBUTES lpProcessAttributes, - LPSECURITY_ATTRIBUTES lpThreadAttributes, - BOOL bInheritHandles, - DWORD dwCreationFlags, - LPVOID lpEnvironment, - LPCWSTR lpCurrentDirectory, - LPSTARTUPINFOW lpStartupInfo, - LPPROCESS_INFORMATION lpProcessInformation + void* hToken, + const wchar_t* lpApplicationName, + wchar_t* lpCommandLine, + void* lpProcessAttributes, + void* lpThreadAttributes, + int bInheritHandles, + unsigned long dwCreationFlags, + void* lpEnvironment, + const wchar_t* lpCurrentDirectory, + void* lpStartupInfo, + void* lpProcessInformation ); #define CreateProcessAsUser CreateProcessAsUserW void positiveTestCases() { - LPCWSTR lpCommandLine = (LPCWSTR)L"C:\\Program Files\\MyApp"; - HANDLE h = 0; - LPWSTR lpApplicationName = NULL; + const wchar_t* lpCommandLine = (const wchar_t*)L"C:\\Program Files\\MyApp"; + void* h = 0; + wchar_t* lpApplicationName = NULL; // CreatePorcessA CreateProcessA( //BUG NULL, - (LPSTR)"C:\\Program Files\\MyApp", + (char*)"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcessW CreateProcessW( //BUG NULL, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcess CreateProcess( //BUG NULL, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // lpCommandLine as hardcoded variable CreateProcess( //BUG NULL, - (LPWSTR)lpCommandLine, + (wchar_t*)lpCommandLine, NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessWithTokenW @@ -209,48 +128,49 @@ void positiveTestCases() h, LOGON_WITH_PROFILE, NULL, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); // CreateProcessWithLogonW CreateProcessWithLogonW( //BUG - (LPCWSTR)L"UserName", - (LPCWSTR)L"CONTOSO", - (LPCWSTR)L"", + (const wchar_t*)L"UserName", + (const wchar_t*)L"CONTOSO", + (const wchar_t*)L"", LOGON_WITH_PROFILE, NULL, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); // CreateProcessAsUserA CreateProcessAsUserA( //BUG h, NULL, - (LPSTR)"C:\\Program Files\\MyApp", + (char*)"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessAsUserW CreateProcessAsUserW( //BUG h, NULL, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessAsUser CreateProcessAsUser( //BUG h, NULL, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcess with a hardcoded variable for application Name (NULL) + // Variation: tab instead of space CreateProcess( //BUG lpApplicationName, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program\tFiles\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); } -void PositiveTestCasesWithCmdLineParameter(LPWSTR lpCommandLine) +void PositiveTestCasesWithCmdLineParameter(wchar_t* lpCommandLine) { // lpCommandLine as variable CreateProcess( //BUG - Depends on the caller @@ -261,12 +181,12 @@ void PositiveTestCasesWithCmdLineParameter(LPWSTR lpCommandLine) void PositiveTestCasesWithCmdLineParameter_caller() { - PositiveTestCasesWithCmdLineParameter((LPWSTR)L"C:\\Program Files\\MyApp"); + PositiveTestCasesWithCmdLineParameter((wchar_t*)L"C:\\Program Files\\MyApp"); } // NOTE: This function will not be flagged as having a bug by this rule. // but as it is, the function can still be misused -void FalseNegativeTestCasesWithCmdLineParameter(LPWSTR lpCommandLine) +void FalseNegativeTestCasesWithCmdLineParameter(wchar_t* lpCommandLine) { // lpCommandLine as variable CreateProcess( //Depends on the caller, this time the caller will quote @@ -279,18 +199,18 @@ void FalseNegativeTestCasesWithCmdLineParameter_caller() { // No bug - escaped command line // But compare with "PositiveTestCasesWithCmdLineParameter" - FalseNegativeTestCasesWithCmdLineParameter((LPWSTR)L"\"C:\\Program Files\\MyApp\""); + FalseNegativeTestCasesWithCmdLineParameter((wchar_t*)L"\"C:\\Program Files\\MyApp\""); } -void PositiveTestCasesWithAppNameParameter(LPWSTR lpApplicationName) +void PositiveTestCasesWithAppNameParameter(wchar_t* lpApplicationName) { - HANDLE h = 0; + void* h = 0; CreateProcessWithTokenW( //BUG - Depends on the caller. In this case the caller sends NULL h, LOGON_WITH_PROFILE, lpApplicationName, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); } @@ -301,15 +221,15 @@ void PositiveTestCasesWithAppNameParameter_caller() // NOTE: This function will not be flagged as having a bug by this rule. // but as it is, the function can still be misused -void FalseNegativeTestCasesWithAppNameParameter(LPWSTR lpApplicationName) +void FalseNegativeTestCasesWithAppNameParameter(wchar_t* lpApplicationName) { - HANDLE h = 0; + void* h = 0; CreateProcessWithTokenW( // Depends on the caller. In this case the caller sends an ApplicatioName h, LOGON_WITH_PROFILE, lpApplicationName, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); } @@ -317,10 +237,10 @@ void FalseNegativeTestCasesWithAppNameParameter_caller() { // No bug - escaped command line // But compare with "PositiveTestCasesWithAppNameParameter" - FalseNegativeTestCasesWithAppNameParameter((LPWSTR)L"MyApp.exe"); + FalseNegativeTestCasesWithAppNameParameter((wchar_t*)L"MyApp.exe"); } -bool MayReturnFalse() +int MayReturnFalse() { // return ((rand() % 2) == 0); return true; @@ -328,20 +248,20 @@ bool MayReturnFalse() void TestCaseProbablyBug() { - LPCWSTR lpApplicationName = NULL; + const wchar_t* lpApplicationName = NULL; if (!MayReturnFalse()) { - lpApplicationName = (LPCWSTR)L"app.exe"; + lpApplicationName = (const wchar_t*)L"app.exe"; } CreateProcessWithLogonW( // BUG (Probably - depends on a condition that may be false) - (LPCWSTR)L"UserName", - (LPCWSTR)L"CONTOSO", - (LPCWSTR)L"", + (const wchar_t*)L"UserName", + (const wchar_t*)L"CONTOSO", + (const wchar_t*)L"", LOGON_WITH_PROFILE, - (LPWSTR)lpApplicationName, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)lpApplicationName, + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); if (lpApplicationName) @@ -352,32 +272,32 @@ void TestCaseProbablyBug() void negativeTestCases_quotedCommandLine() { - LPCWSTR lpCommandLine = (LPCWSTR)L"\"C:\\Program Files\\MyApp\" with additional params"; - HANDLE h = 0; - LPWSTR lpApplicationName = NULL; + const wchar_t* lpCommandLine = (const wchar_t*)L"\"C:\\Program Files\\MyApp\" with additional params"; + void* h = 0; + wchar_t* lpApplicationName = NULL; // CreatePorcessA CreateProcessA( NULL, - (LPSTR)"\"C:\\Program Files\\MyApp\"", + (char*)"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcessW CreateProcessW( NULL, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcess CreateProcess( NULL, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // lpCommandLine as hardcoded variable CreateProcess( NULL, - (LPWSTR)lpCommandLine, + (wchar_t*)lpCommandLine, NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessWithTokenW @@ -385,119 +305,126 @@ void negativeTestCases_quotedCommandLine() h, LOGON_WITH_PROFILE, NULL, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", 0, NULL, NULL, NULL, NULL); // CreateProcessWithLogonW CreateProcessWithLogonW( - (LPCWSTR)L"UserName", - (LPCWSTR)L"CONTOSO", - (LPCWSTR)L"", + (const wchar_t*)L"UserName", + (const wchar_t*)L"CONTOSO", + (const wchar_t*)L"", LOGON_WITH_PROFILE, NULL, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", 0, NULL, NULL, NULL, NULL); // CreateProcessAsUserA CreateProcessAsUserA( h, NULL, - (LPSTR)"\"C:\\Program Files\\MyApp\"", + (char*)"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessAsUserW CreateProcessAsUserW( h, NULL, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessAsUser CreateProcessAsUser( h, NULL, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcess with a hardcoded variable for application Name (NULL) CreateProcess( lpApplicationName, - (LPWSTR)L"\"C:\\Program Files\\MyApp\"", + (wchar_t*)L"\"C:\\Program Files\\MyApp\"", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + + // Null AppName, but lpComamndLine has no spaces/tabs + CreateProcessA( + NULL, + (char*)"C:\\MyFolder\\MyApp.exe", + NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); + } void negativeTestCases_AppNameSet() { - LPCWSTR lpCommandLine = (LPCWSTR)L"C:\\Program Files\\MyApp"; - HANDLE h = 0; - LPCWSTR lpApplicationName = (LPCWSTR)L"MyApp.exe"; + const wchar_t* lpCommandLine = (const wchar_t*)L"C:\\Program Files\\MyApp"; + void* h = 0; + const wchar_t* lpApplicationName = (const wchar_t*)L"MyApp.exe"; // CreatePorcessA CreateProcessA( - (LPSTR)"MyApp.exe", - (LPSTR)"C:\\Program Files\\MyApp", + (char*)"MyApp.exe", + (char*)"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcessW CreateProcessW( - (LPWSTR)L"MyApp.exe", - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"MyApp.exe", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcess CreateProcess( - (LPWSTR)L"MyApp.exe", - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"MyApp.exe", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // lpCommandLine as hardcoded variable CreateProcess( - (LPWSTR)L"MyApp.exe", - (LPWSTR)lpCommandLine, + (wchar_t*)L"MyApp.exe", + (wchar_t*)lpCommandLine, NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessWithTokenW CreateProcessWithTokenW( h, LOGON_WITH_PROFILE, - (LPWSTR)L"MyApp.exe", - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"MyApp.exe", + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); // CreateProcessWithLogonW CreateProcessWithLogonW( - (LPCWSTR)L"UserName", - (LPCWSTR)L"CONTOSO", - (LPCWSTR)L"", + (const wchar_t*)L"UserName", + (const wchar_t*)L"CONTOSO", + (const wchar_t*)L"", LOGON_WITH_PROFILE, - (LPWSTR)L"MyApp.exe", - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"MyApp.exe", + (wchar_t*)L"C:\\Program Files\\MyApp", 0, NULL, NULL, NULL, NULL); // CreateProcessAsUserA CreateProcessAsUserA( h, - (LPSTR)"MyApp.exe", - (LPSTR)"C:\\Program Files\\MyApp", + (char*)"MyApp.exe", + (char*)"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessAsUserW CreateProcessAsUserW( h, - (LPWSTR)L"MyApp.exe", - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"MyApp.exe", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreateProcessAsUser CreateProcessAsUser( h, - (LPWSTR)L"MyApp.exe", - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)L"MyApp.exe", + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); // CreatePorcess with a hardcoded variable for application Name (NULL) CreateProcess( - (LPWSTR)lpApplicationName, - (LPWSTR)L"C:\\Program Files\\MyApp", + (wchar_t*)lpApplicationName, + (wchar_t*)L"C:\\Program Files\\MyApp", NULL, NULL, FALSE, 0, NULL, NULL, NULL, NULL); } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected index 1f4519cabb01..652d92d19f58 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected @@ -1,13 +1,13 @@ -| UnsafeCreateProcessCall.cpp:184:5:184:18 | call to CreateProcessA | call to CreateProcessA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:190:5:190:18 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:196:5:196:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:202:5:202:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:208:5:208:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:216:5:216:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:226:5:226:24 | call to CreateProcessAsUserA | call to CreateProcessAsUserA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:233:5:233:24 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:240:5:240:23 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:247:5:247:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:256:5:256:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:289:5:289:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:338:5:338:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:103:5:103:18 | call to CreateProcessA | call to CreateProcessA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:109:5:109:18 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:115:5:115:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:121:5:121:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:127:5:127:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:135:5:135:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:145:5:145:24 | call to CreateProcessAsUserA | call to CreateProcessAsUserA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:152:5:152:24 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:159:5:159:23 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:167:5:167:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program\tFiles\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:176:5:176:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:209:5:209:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:258:5:258:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | From 7ab723ae7950e69629321058090c79bf12b79bbc Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Tue, 16 Oct 2018 10:00:51 -0700 Subject: [PATCH 7/7] Fixing typos & incorporating feedback. (MSFT feedback) Adding a new tag in the header @msrc.severity important --- .../CWE/CWE-428/UnsafeCreateProcessCall.qhelp | 6 ++--- .../CWE/CWE-428/UnsafeCreateProcessCall.ql | 5 ++-- .../CWE-428/UnsafeCreateProcessCall.expected | 26 +++++++++---------- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp index 550355e8026d..d4691284c3ca 100644 --- a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.qhelp @@ -4,7 +4,7 @@ -

    This query indicates that there is a call to a function of the CreateProcess* family of functions, which may result in a security vulnerability if the path contains spaces.

    +

    This query indicates that there is a call to a function of the CreateProcess* family of functions, which introduces a security vulnerability.

     @@ -13,9 +13,9 @@ -

    In the following example, CreateProcessW is called with a NULL value for lpApplicationName, +

    In the following example, CreateProcessW is called with a NULL value for lpApplicationName, and the value for lpCommandLine that represent the application path is not quoted and has spaces in it.

    -

    If an attacker has access to the file system, it is possible to elevate privileges by creating a file such as "C:\Program.exe" that will be executed instead of the intended application.

    +

    If an attacker has access to the file system, they can elevate privileges by creating a file such as C:\Program.exe that will be executed instead of the intended application.

    To fix this issue, specify a valid string for lpApplicationName, or quote the path for lpCommandLine. For example:

    diff --git a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql index de7685bf5063..1518c6c1f0f0 100644 --- a/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql +++ b/cpp/ql/src/Security/CWE/CWE-428/UnsafeCreateProcessCall.ql @@ -1,10 +1,11 @@ /** * @name NULL application name with an unquoted path in call to CreateProcess - * @description Calling a function of the CreateProcess* family of functions, which may result in a security vulnerability if the path contains spaces. + * @description Calling a function of the CreateProcess* family of functions, where the path contains spaces, introduces a security vulnerability. * @id cpp/unsafe-create-process-call * @kind problem * @problem.severity error * @precision medium + * @msrc.severity important * @tags security * external/cwe/cwe-428 * external/microsoft/C6277 @@ -124,6 +125,6 @@ where QuotedCommandInCreateProcessFunctionConfiguration quotedConfig | cmd = call.getArgument(call.getCommandLineArgumentId()) and quotedConfig.hasFlow(DataFlow2::exprNode(source), DataFlow2::exprNode(cmd)) - and msg2 = " and with an unquoted lpCommandLine (" + cmd + ") may result in a security vulnerability if the path contains spaces." + and msg2 = " and with an unquoted lpCommandLine (" + cmd + ") introduces a security vulnerability if the path contains spaces." ) select call, msg1 + " " + msg2 diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected index 652d92d19f58..7295f1197b85 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-428/UnsafeCreateProcessCall.expected @@ -1,13 +1,13 @@ -| UnsafeCreateProcessCall.cpp:103:5:103:18 | call to CreateProcessA | call to CreateProcessA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:109:5:109:18 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:115:5:115:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:121:5:121:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:127:5:127:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:135:5:135:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:145:5:145:24 | call to CreateProcessAsUserA | call to CreateProcessAsUserA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:152:5:152:24 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:159:5:159:23 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:167:5:167:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program\tFiles\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:176:5:176:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:209:5:209:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | -| UnsafeCreateProcessCall.cpp:258:5:258:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) may result in a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:103:5:103:18 | call to CreateProcessA | call to CreateProcessA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:109:5:109:18 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:115:5:115:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:121:5:121:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:127:5:127:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:135:5:135:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:145:5:145:24 | call to CreateProcessAsUserA | call to CreateProcessAsUserA with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:152:5:152:24 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:159:5:159:23 | call to CreateProcessAsUserW | call to CreateProcessAsUserW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:167:5:167:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program\tFiles\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:176:5:176:17 | call to CreateProcessW | call to CreateProcessW with lpApplicationName == NULL (0) and with an unquoted lpCommandLine (lpCommandLine) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:209:5:209:27 | call to CreateProcessWithTokenW | call to CreateProcessWithTokenW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. | +| UnsafeCreateProcessCall.cpp:258:5:258:27 | call to CreateProcessWithLogonW | call to CreateProcessWithLogonW with lpApplicationName == NULL (lpApplicationName) and with an unquoted lpCommandLine (C:\\Program Files\\MyApp) introduces a security vulnerability if the path contains spaces. |