Skip to content

RB: add a step directly from a store of an instance field to a read #11158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
erik-krogh wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

RB: add a step directly from a store of an instance field to a read #11158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb8ec04
add failing test
erik-krogh Nov 8, 2022
b628f1b
add a step directly from a store of an instance field to a read
erik-krogh Nov 8, 2022
bbc7741
more flow
erik-krogh Nov 8, 2022
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead.
*/

import codeql.ruby.AST as Ast
import codeql.ruby.DataFlow
import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
private import codeql.ruby.TaintTracking
Expand All @@ -32,4 +33,27 @@ class Configuration extends TaintTracking::Configuration {
override DataFlow::FlowFeature getAFeature() {
result instanceof DataFlow::FeatureHasSourceCallContext
}

// A direct step from a write to an instance field in a constructor to a read of that same field.
// Corresponds to `DataFlow::localFieldStep` in the JS analysis.
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
exists(DataFlow::ModuleNode clz, string field, Ast::Method constructor |
clz.getAnOwnInstanceVariableWriteValue(field) = pred and
constructor = pred.asExpr().getExpr().getEnclosingCallable() and
constructor.getName() = "initialize" and
not succ.asExpr().getExpr().getEnclosingCallable() = constructor and
(
clz.getAnOwnInstanceVariableRead(field) = succ
or
// TODO: have getAnOwnInstanceVariableRead return this when `attr_reader` is used
exists(DataFlow::MethodNode instMeth | instMeth = clz.getAnInstanceMethod() |
// the parent thing is to recognize reads in blocks. It's ugly, so do something else. TODO:
// TODO: Also add a test for reads inside blocks.
succ.asExpr().getExpr().getParent*().(Ast::Expr).getEnclosingCallable() =
instMeth.asExpr().getExpr() and
succ.(DataFlow::CallNode).getMethodName() = field.suffix(1)
)
)
)
}
}
13 changes: 13 additions & 0 deletions ...s/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ edges
| impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} |
| impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:52:14:52:14 | x |
| impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:54:29:54:29 | x |
| impl/unsafeShell.rb:59:18:59:23 | source : | impl/unsafeShell.rb:60:12:60:17 | source : |
| impl/unsafeShell.rb:60:12:60:17 | source : | impl/unsafeShell.rb:64:21:64:24 | @foo : |
| impl/unsafeShell.rb:60:12:60:17 | source : | impl/unsafeShell.rb:70:21:70:23 | call to foo : |
| impl/unsafeShell.rb:64:21:64:24 | @foo : | impl/unsafeShell.rb:64:19:64:25 | #{...} |
| impl/unsafeShell.rb:70:21:70:23 | call to foo : | impl/unsafeShell.rb:70:19:70:24 | #{...} |
nodes
| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : |
| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
Expand All @@ -35,6 +40,12 @@ nodes
| impl/unsafeShell.rb:51:17:51:17 | x : | semmle.label | x : |
| impl/unsafeShell.rb:52:14:52:14 | x | semmle.label | x |
| impl/unsafeShell.rb:54:29:54:29 | x | semmle.label | x |
| impl/unsafeShell.rb:59:18:59:23 | source : | semmle.label | source : |
| impl/unsafeShell.rb:60:12:60:17 | source : | semmle.label | source : |
| impl/unsafeShell.rb:64:19:64:25 | #{...} | semmle.label | #{...} |
| impl/unsafeShell.rb:64:21:64:24 | @foo : | semmle.label | @foo : |
| impl/unsafeShell.rb:70:19:70:24 | #{...} | semmle.label | #{...} |
| impl/unsafeShell.rb:70:21:70:23 | call to foo : | semmle.label | call to foo : |
subpaths
#select
| impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
Expand All @@ -49,3 +60,5 @@ subpaths
| impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command |
| impl/unsafeShell.rb:52:14:52:24 | call to join | impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:52:14:52:14 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:52:5:52:30 | call to popen | shell command |
| impl/unsafeShell.rb:54:14:54:40 | call to join | impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:54:29:54:29 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:54:5:54:46 | call to popen | shell command |
| impl/unsafeShell.rb:64:14:64:26 | "cat #{...}" | impl/unsafeShell.rb:59:18:59:23 | source : | impl/unsafeShell.rb:64:19:64:25 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:59:18:59:23 | source | library input | impl/unsafeShell.rb:64:5:64:32 | call to popen | shell command |
| impl/unsafeShell.rb:70:14:70:25 | "cat #{...}" | impl/unsafeShell.rb:59:18:59:23 | source : | impl/unsafeShell.rb:70:19:70:24 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:59:18:59:23 | source | library input | impl/unsafeShell.rb:70:5:70:31 | call to popen | shell command |
16 changes: 16 additions & 0 deletions ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,19 @@ def arrayJoin(x)
IO.popen(["foo", "bar", x].join(' '), "w") # NOT OK
end
end

class Fields
def initialize(source)
@foo = source
end

def foo1()
IO.popen("cat #{@foo}", "w") # NOT OK
end

attr_reader :foo

def foo2()
IO.popen("cat #{foo}", "w") # NOT OK
end
end