Rb: more taint-steps for shell-command-construction #11478

erik-krogh · 2022-11-29T13:27:27Z

Adds some taint steps for CVE-2022-33127

Evaluations (rails-projects, nightly) seems OK, there are a few new results that look acceptable to me.
The rails result is adding an extra source to a sink that was already flagged.
The database result looks good. It's possible to configure which executeable is used, and that is interpreted as a shell-string.

…edicate

hvitved

Couldn't we instead simply define a taint-summary for join?

input = "Argument[self].Element[any]" and
output = "ReturnValue" and
preservesValue = false

Such a summary should also work when we consider an array it self tainted (as opposed to an element of that array).

erik-krogh · 2023-02-15T11:35:33Z

Couldn't we instead simply define a taint-summary for join?

Yes, that works for .join() 👍

ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll

@@ -1186,6 +1186,16 @@
    }
  }

+  private class JoinSummary extends SimpleSummarizedCallable {
+    JoinSummary() { this = ["join"] }


hvitved · 2023-02-15T11:55:22Z

ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll

+   * Holds if there an array element `pred` might taint the array defined by `succ`.
+   * This is used for queries where we consider an entire array to be tainted if any of its elements are tainted.
+   */
+  predicate taintedArrayObjectSteps(DataFlow::Node pred, DataFlow::Node succ) {


I think the better way to do this is to allow implicit array reads at sinks. We could either do this for the two relevant queries, using Configuration::allowImplicitRead, or for all taint tracking configurations, using defaultImplicitTaintRead, like e.g. in Java.

That does give me some of the desired behavior, but not all.

This test case is flagged with my "taint-step"-approach, but not with your "implicit-read"-approach.

def join_indirect(x, y) arr = Array("foo = ", x) eval(arr.join(" ")) # NOT OK arr2 = [Array("foo = ", y).join(" ")] eval(arr2.join("\n")) # NOT OK end

I'll do the allowImplicitRead approach for now as it gives me most of the desired behavior (I'm just adding it on the two relevant queries), and then I'll return to the CVE later.

Isn't this because the kernel method Array does not work like you intend in your examples? But I also don't think we currently have flow summaries for that method; I guess it should be something like

// already an array input = "Argument[0].WithElement[0..]" and output = "ReturnValue" or // not already an array input = "Argument[0].WithoutElement[0..]" and output = "ReturnValue.Element[0]"

You're completely right. I was looking at how it was used in the CVE, and probably just assumed that it was similar to the JS Array() call.
I've added a summary for Array() based on your suggestion.

hvitved

Thanks for addressing my comments. This looks good now, except for two redundant imports.

Also, we should have a final DCA run.

hvitved · 2023-03-01T12:22:37Z

ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll

@@ -10,6 +10,7 @@ import codeql.ruby.DataFlow
 import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction
 private import codeql.ruby.TaintTracking
 private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.frameworks.core.Array


Can be removed now.

hvitved · 2023-03-01T12:22:50Z

ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll

@@ -11,6 +11,7 @@ import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruct
 private import codeql.ruby.TaintTracking
 private import CommandInjectionCustomizations::CommandInjection as CommandInjection
 private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.frameworks.core.Array


erik-krogh · 2023-03-01T20:24:39Z

Also, we should have a final DCA run.

It blew up.
I'll investigate, but I don't think I'll have time for that this week.
I'm putting this in draft, and will undraft again when I've figured out what's happening.

…fix performance

hvitved · 2023-03-14T07:57:05Z

ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll

+      result.getMethodName() = "Array" and
+      // I have to have a simplified "KernelMethodCall" implementation inlined here, because relying on `UnknownMethodCall` results in non-monotonic recursion (even if using `getACall`).
+      (
+        result = API::getTopLevelMember("Kernel").getAMethodCall(_).asExpr().getExpr()


Consider using something like getAStaticArrayCall here instead, in order to be able to go back to getASimpleCall (which means that the summary will also be used during type tracking).

erik-krogh · 2023-03-16T12:48:17Z

New evaluations (nightly, rails-projects) show OK performance and some new results.

Triaging the new results they are mostly from new steps through .join("..").
And for most of the results we already flagged the sink from another source, but the extra completeness is nice.

github-actions bot added the Ruby label Nov 29, 2022

erik-krogh added 3 commits January 30, 2023 16:46

more custom array steps from unsafe-code-construction to a utility pr…

a4c42aa

…edicate

add array-taint-steps to unsafe-shell-command-construction

962465f

add more array taint steps that taint the entire array

c2e8206

erik-krogh force-pushed the more-shell-taint branch from 6e6a327 to c2e8206 Compare January 30, 2023 20:18

erik-krogh marked this pull request as ready for review February 2, 2023 08:39

erik-krogh requested a review from a team as a code owner February 2, 2023 08:39

erik-krogh added the no-change-note-required This PR does not need a change note label Feb 2, 2023

calumgrant requested a review from hvitved February 6, 2023 09:30

hvitved reviewed Feb 8, 2023

View reviewed changes

erik-krogh added 2 commits February 15, 2023 11:35

Merge branch 'main' into more-shell-taint

d2bd70d

rewrite the taint-step for join() to a flowsummary

17f7ba2

github-advanced-security bot found potential problems Feb 15, 2023

View reviewed changes

ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll

@@ -1186,6 +1186,16 @@

}

}

private class JoinSummary extends SimpleSummarizedCallable {

JoinSummary() { this = ["join"] }

Check warning

Code scanning / CodeQL

Singleton set literal

Singleton set literal can be replaced by its member.

hvitved reviewed Feb 15, 2023

View reviewed changes

erik-krogh added 2 commits February 27, 2023 18:27

Merge branch 'main' into more-shell-taint

b0797a2

use allowImplicitRead instead of a taint-step from elements to the array

36b3376

erik-krogh requested a review from a team as a code owner February 28, 2023 15:10

erik-krogh requested a review from hvitved February 28, 2023 15:10

add summary for the Array method on Kernel

31336b0

hvitved requested changes Mar 1, 2023

View reviewed changes

remove redundant imports

6cd4cd3

erik-krogh marked this pull request as draft March 1, 2023 20:24

erik-krogh added 3 commits March 13, 2023 17:33

Merge branch 'main' into HEAD

25a6d49

fix bad join by removing bad recursion

8b99e8a

specialize allowImplicitRead in unsafe-shell-command-construction to …

4307889

…fix performance

hvitved reviewed Mar 14, 2023

View reviewed changes

erik-krogh added 2 commits March 14, 2023 09:20

don't depend on the callgraph in KernelArraySummary

984a589

Merge branch 'main' into more-shell-taint

2133d1a

also restrict allowImplicitRead in unsafe-code-construction

cc3efcd

erik-krogh marked this pull request as ready for review March 16, 2023 12:48

hvitved approved these changes Mar 17, 2023

View reviewed changes

erik-krogh merged commit af98ceb into github:main Mar 20, 2023

hvitved mentioned this pull request Mar 20, 2023

Ruby: Introduce ContentSet::isElementOfType[OrUnknown]/1 #12582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rb: more taint-steps for shell-command-construction #11478

Rb: more taint-steps for shell-command-construction #11478

Uh oh!

erik-krogh commented Nov 29, 2022 •

edited

Loading

Uh oh!

hvitved left a comment

Uh oh!

erik-krogh commented Feb 15, 2023

Uh oh!

Check warning

hvitved Feb 15, 2023

Uh oh!

erik-krogh Feb 28, 2023 •

edited

Loading

Uh oh!

hvitved Mar 1, 2023

Uh oh!

erik-krogh Mar 1, 2023

Uh oh!

hvitved left a comment

Uh oh!

hvitved Mar 1, 2023

Uh oh!

hvitved Mar 1, 2023

Uh oh!

erik-krogh commented Mar 1, 2023

Uh oh!

hvitved Mar 14, 2023

Uh oh!

erik-krogh commented Mar 16, 2023

Uh oh!

Uh oh!

Rb: more taint-steps for shell-command-construction #11478

Rb: more taint-steps for shell-command-construction #11478

Uh oh!

Conversation

erik-krogh commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Feb 15, 2023

Uh oh!

Check warning

hvitved Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

erik-krogh Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

erik-krogh Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

hvitved Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Mar 1, 2023

Uh oh!

hvitved Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Mar 16, 2023

Uh oh!

Uh oh!

erik-krogh commented Nov 29, 2022 •

edited

Loading

erik-krogh Feb 28, 2023 •

edited

Loading