C++ : NULL application name with an unquoted path in call to CreateProcess by raulgarciamsft · Pull Request #319 · github/codeql

raulgarciamsft · 2018-10-12T22:59:21Z

Calling a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces.

…ocess Calling a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces.

geoffw0

Thanks for this contribution @raulgarciams. QL, tests and help all look excellent! I haven't found any results in the wild, but I imagine this query will make a valuable regression check.

I've made a few comments - mostly not much more than typos.

geoffw0 · 2018-10-15T08:13:21Z

+<qhelp>
+
+<overview>
+  <p>This query indicates that there is a call to a function of the <code>CreatePorcess*</code> family of functions, which may result in a security vulnerability if the path contains spaces.</p>


Typo CreatePorcess -> CreateProcess.

geoffw0 · 2018-10-15T08:14:21Z

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


int -> in it?

geoffw0 · 2018-10-15T08:38:39Z

+{
+    HANDLE h = 0;
+
+    CreateProcessWithTokenW(                    // Depends on the caller. In this case the caller sends an ApplicatioName


ApplicatioName -> ApplicationName.

geoffw0 · 2018-10-15T19:42:40Z

+      this.getTarget().hasGlobalName("CreateProcessAsUserA") or
+      this.getTarget().hasGlobalName("CreateProcessAsUserW") 
+    ) then ( result = 1 )
+    else (result = -1 )


The -1 cases in this predicate and the one below could be omitted. It's perfectly fine for a predicate to have no result, and either way a use of FunctionCall.getArgument(thisPredicate()) would have no result.

Got it. I will fix it & submit the change

When I remove the "else" I get the following compilation error: Unexpected input ‘}’ expecting one of: ‘and’, ‘implies’, ‘in’, instanceof’, ‘or’, …

The best thing to do is to rewrite those predicates using and and or:

( this.getTarget().hasGlobalName("CreateProcessA") or this.getTarget().hasGlobalName("CreateProcessW") ) and result = 0 or ...

Never mind, with Dave's recommendation below, I don't need the IF anymore. Thanks

dave-bartolomeo

Overall looks good. Most of my feedback is just pointing out a few typos, plus one significant simplification to the actual query.

dave-bartolomeo · 2018-10-15T19:44:43Z

 # Visual studio temporaries, except a file used by QL4VS
 .vs/*
 !.vs/VSWorkspaceSettings.json
+


Unnecessary change?

dave-bartolomeo · 2018-10-15T19:45:38Z

+<qhelp>
+
+<overview>
+  <p>This query indicates that there is a call to a function of the <code>CreatePorcess*</code> family of functions, which may result in a security vulnerability if the path contains spaces.</p>


Typo - s/CreatePorcess/CreateProcess/

dave-bartolomeo · 2018-10-15T19:46:59Z

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


Typo - s/int/in it/

Typo - s/represent/represents/

dave-bartolomeo · 2018-10-15T19:48:34Z

@@ -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.


Typo - s/CreatePorcess/CreateProcess/

dave-bartolomeo · 2018-10-15T20:06:24Z

+#define far
+#define LOGON_WITH_PROFILE 0x00000001
+
+typedef char CHAR;


Would it be possible to remove most of these typedefs, and leave the various struct definitions with empty bodies (e.g. struct _STARTUPINFOW { };? This shouldn't affect the query, and all of these cut-and-paste definitions from the Windows headers kind of clutter up the real test cases.

No problem. I have been trying to make the test function calls as close to the real ones, but I will change it.

raulgarciamsft · 2018-10-15T20:55:15Z

BTW. After talking with Robert & Dave offline. I will try to make some major changes. Thanks

Improved the query to avoid multiple calls to hasGlobalName Fixed typos Simplified the test case file

geoffw0 · 2018-10-16T08:26:50Z

Changes LGTM. I wasn't involved in the offline conversation - are the changes finished or is there more to come?

semmledocs-ac

Technical writer editorial review.
Documentation looks great. Just a few comments/suggestions.

semmledocs-ac · 2018-10-16T10:03:45Z

+    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."


As per comment above, consider changing
may result in a security vulnerability
to
introduces a security vulnerability

raulgarciamsft · 2018-10-16T16:34:08Z

I should have pasted the offline conversation summary here, sorry about that:

We were trying to discuss if there was a way to detect this scenario when a developer uses a writable buffer (as the API intends) instead of doing an unsafe cast from a static read-only string to a writable string, but we cannot easily check if the unquoted string is copied ( i.e. lpComamndLine = new wchar_t[256]; wcscpy_s(lpComamndLine, 256, L"c:\program files\foo"); )

This version of the Semmle rule (with the changes recommended on the PR) can detect most of the cases with a static string, improving over the original PreFAST rule that misses anything that is not hardcoded directly on the CreateProcess* function call, and that adding the check for the lpCommandLine argument to have a space/tab helps to avoid a false positive.

Historically the check for C6277 made sense for a time when the majority of the Windows apps were not using Unicode, and the CreateProcess*A versions of the API would work with a string literal (circa Windows 9x), so we are not expecting a lot of hits on this one.

(MSFT feedback) Adding a new tag in the header @msrc.severity important

semmledocs-ac · 2018-10-17T06:46:38Z

@raulgarciamsft - thanks for making those changes

geoffw0 · 2018-10-17T12:52:24Z

Great. Support for string buffers sounds like a desirable improvement but I'm very happy with this as a solid initial version of the query. I'll merge this in a few hours unless anybody speaks up (@dave-bartolomeo ?).

dave-bartolomeo

Fixes look great. LGTM

Add some more vulnerable ActiveRecord methods

raulgarciamsft and others added 2 commits October 12, 2018 15:57

C++ : NULL application name with an unquoted path in call to CreatePr…

85283d6

…ocess Calling a function of the CreatePorcess* family of functions, which may result in a security vulnerability if the path contains spaces.

Merge branch 'master' into users/raulga/c6277

242d403

geoffw0 reviewed Oct 15, 2018

View reviewed changes

dave-bartolomeo suggested changes Oct 15, 2018

View reviewed changes

raulgarciamsft added 4 commits October 15, 2018 13:38

Update .gitignore

bc39873

Update UnsafeCreateProcessCall.qhelp

b8f8c99

Update UnsafeCreateProcessCall.qhelp

1d85369

Update UnsafeCreateProcessCall.ql

cd5e788

Removed one false-positive scenario (no space on lpCommandLine)

22d5480

Improved the query to avoid multiple calls to hasGlobalName Fixed typos Simplified the test case file

semmledocs-ac reviewed Oct 16, 2018

View reviewed changes

Fixing typos & incorporating feedback.

7ab723a

(MSFT feedback) Adding a new tag in the header @msrc.severity important

dave-bartolomeo approved these changes Oct 17, 2018

View reviewed changes

geoffw0 merged commit 6e10f39 into github:master Oct 17, 2018

geoffw0 mentioned this pull request Nov 20, 2018

CPP: Change notes #504

Merged

raulgarciamsft deleted the users/raulga/c6277 branch October 4, 2019 16:58

aibaars pushed a commit that referenced this pull request Oct 14, 2021

Merge pull request #319 from github/hmac-activerecord-updates

7f103b9

Add some more vulnerable ActiveRecord methods

Conversation

raulgarciamsft commented Oct 12, 2018

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!

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!

dave-bartolomeo 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulgarciamsft commented Oct 15, 2018

Uh oh!

geoffw0 commented Oct 16, 2018

Uh oh!

semmledocs-ac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulgarciamsft commented Oct 16, 2018

Uh oh!

semmledocs-ac commented Oct 17, 2018

Uh oh!

geoffw0 commented Oct 17, 2018

Uh oh!

dave-bartolomeo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants