From 286c894f3407365e38025676c334a6df95f1dd48 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Tue, 16 Nov 2021 15:39:47 +0000 Subject: [PATCH 1/2] ruby: add DataFlow::MethodCallNode class --- .../ruby/dataflow/internal/DataFlowPublic.qll | 10 ++++++++++ .../codeql/ruby/frameworks/ActiveStorage.qll | 5 +++-- ruby/ql/lib/codeql/ruby/frameworks/Files.qll | 20 +++++++++---------- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 3636573c2cd3..867bded0f152 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -60,6 +60,16 @@ class CallNode extends LocalSourceNode { Node getKeywordArgument(string name) { result.asExpr() = node.getKeywordArgument(name) } } +/** A data-flow node corresponding to a method call in the control-flow graph. */ +class MethodCallNode extends CallNode { + private CfgNodes::ExprNodes::MethodCallCfgNode node; + + MethodCallNode() { node = this.asExpr() } + + /** Gets the name of the the method called by the method call corresponding to this data-flow node */ + string getMethodName() { result = node.getExpr().getMethodName() } +} + /** * An expression, viewed as a node in a data flow graph. * diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll index f25613aa4dbb..4013bab047b2 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll @@ -5,11 +5,12 @@ private import codeql.ruby.DataFlow private import codeql.ruby.dataflow.FlowSummary /** Defines calls to `ActiveStorage::Filename#sanitized` as path sanitizers. */ -class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode { +class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, + DataFlow::MethodCallNode { ActiveStorageFilenameSanitizedCall() { this.getReceiver() = API::getTopLevelMember("ActiveStorage").getMember("Filename").getAnInstantiation() and - this.asExpr().getExpr().(MethodCall).getMethodName() = "sanitized" + this.getMethodName() = "sanitized" } } diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Files.qll b/ruby/ql/lib/codeql/ruby/frameworks/Files.qll index 440a6e6201da..e1a83463f2a9 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Files.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/Files.qll @@ -99,7 +99,7 @@ module IO { } /** - * A `DataFlow::CallNode` that reads data using the `IO` class. For example, + * A `DataFlow::MethodCallNode` that reads data using the `IO` class. For example, * the `IO.read call in: * * ```rb @@ -112,7 +112,7 @@ module IO { * filesystem. For working with filesystem accesses specifically, see * `IOFileReader` or the `FileSystemReadAccess` concept. */ - class IOReader extends DataFlow::CallNode { + class IOReader extends DataFlow::MethodCallNode { private boolean classMethodCall; private string api; @@ -127,8 +127,7 @@ module IO { api = "IO" and exists(IOInstanceStrict ii | this.getReceiver() = ii and - this.asExpr().getExpr().(MethodCall).getMethodName() = - ioFileReaderMethodName(classMethodCall) + this.getMethodName() = ioFileReaderMethodName(classMethodCall) ) or // File instance methods @@ -136,8 +135,7 @@ module IO { api = "File" and exists(File::FileInstance fi | this.getReceiver() = fi and - this.asExpr().getExpr().(MethodCall).getMethodName() = - ioFileReaderMethodName(classMethodCall) + this.getMethodName() = ioFileReaderMethodName(classMethodCall) ) // TODO: enumeration style methods such as `each`, `foreach`, etc. } @@ -151,7 +149,7 @@ module IO { } /** - * A `DataFlow::CallNode` that reads data from the filesystem using the `IO` + * A `DataFlow::MethodCallNode` that reads data from the filesystem using the `IO` * class. For example, the `IO.read call in: * * ```rb @@ -219,7 +217,7 @@ module File { /** * A call to a `File` method that may return one or more filenames. */ - class FileModuleFilenameSource extends FileNameSource, DataFlow::CallNode { + class FileModuleFilenameSource extends FileNameSource, DataFlow::MethodCallNode { FileModuleFilenameSource() { // Class methods this = @@ -232,13 +230,13 @@ module File { // Instance methods exists(FileInstance fi | this.getReceiver() = fi and - this.asExpr().getExpr().(MethodCall).getMethodName() = ["path", "to_path"] + this.getMethodName() = ["path", "to_path"] ) } } private class FileModulePermissionModification extends FileSystemPermissionModification::Range, - DataFlow::CallNode { + DataFlow::MethodCallNode { private DataFlow::Node permissionArg; FileModulePermissionModification() { @@ -321,7 +319,7 @@ module FileUtils { } private class FileUtilsPermissionModification extends FileSystemPermissionModification::Range, - DataFlow::CallNode { + DataFlow::MethodCallNode { private DataFlow::Node permissionArg; FileUtilsPermissionModification() { From c8cdbfa352f3e1b9197d0762544404c9a5fa8ab4 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Tue, 16 Nov 2021 17:11:26 +0000 Subject: [PATCH 2/2] ruby: push getMethodName into DataFlow::CallNode --- .../codeql/ruby/dataflow/internal/DataFlowPublic.qll | 11 ++--------- ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll | 3 +-- ruby/ql/lib/codeql/ruby/frameworks/Files.qll | 12 ++++++------ .../lib/codeql/ruby/frameworks/StandardLibrary.qll | 2 -- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 867bded0f152..fd8ec51e8692 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -58,16 +58,9 @@ class CallNode extends LocalSourceNode { /** Gets the data-flow node corresponding to the named argument of the call corresponding to this data-flow node */ Node getKeywordArgument(string name) { result.asExpr() = node.getKeywordArgument(name) } -} - -/** A data-flow node corresponding to a method call in the control-flow graph. */ -class MethodCallNode extends CallNode { - private CfgNodes::ExprNodes::MethodCallCfgNode node; - - MethodCallNode() { node = this.asExpr() } - /** Gets the name of the the method called by the method call corresponding to this data-flow node */ - string getMethodName() { result = node.getExpr().getMethodName() } + /** Gets the name of the the method called by the method call (if any) corresponding to this data-flow node */ + string getMethodName() { result = node.getExpr().(MethodCall).getMethodName() } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll index 4013bab047b2..93ede4d99254 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll @@ -5,8 +5,7 @@ private import codeql.ruby.DataFlow private import codeql.ruby.dataflow.FlowSummary /** Defines calls to `ActiveStorage::Filename#sanitized` as path sanitizers. */ -class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, - DataFlow::MethodCallNode { +class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode { ActiveStorageFilenameSanitizedCall() { this.getReceiver() = API::getTopLevelMember("ActiveStorage").getMember("Filename").getAnInstantiation() and diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Files.qll b/ruby/ql/lib/codeql/ruby/frameworks/Files.qll index e1a83463f2a9..0ee0c00b63a7 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Files.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/Files.qll @@ -99,7 +99,7 @@ module IO { } /** - * A `DataFlow::MethodCallNode` that reads data using the `IO` class. For example, + * A `DataFlow::CallNode` that reads data using the `IO` class. For example, * the `IO.read call in: * * ```rb @@ -112,7 +112,7 @@ module IO { * filesystem. For working with filesystem accesses specifically, see * `IOFileReader` or the `FileSystemReadAccess` concept. */ - class IOReader extends DataFlow::MethodCallNode { + class IOReader extends DataFlow::CallNode { private boolean classMethodCall; private string api; @@ -149,7 +149,7 @@ module IO { } /** - * A `DataFlow::MethodCallNode` that reads data from the filesystem using the `IO` + * A `DataFlow::CallNode` that reads data from the filesystem using the `IO` * class. For example, the `IO.read call in: * * ```rb @@ -217,7 +217,7 @@ module File { /** * A call to a `File` method that may return one or more filenames. */ - class FileModuleFilenameSource extends FileNameSource, DataFlow::MethodCallNode { + class FileModuleFilenameSource extends FileNameSource, DataFlow::CallNode { FileModuleFilenameSource() { // Class methods this = @@ -236,7 +236,7 @@ module File { } private class FileModulePermissionModification extends FileSystemPermissionModification::Range, - DataFlow::MethodCallNode { + DataFlow::CallNode { private DataFlow::Node permissionArg; FileModulePermissionModification() { @@ -319,7 +319,7 @@ module FileUtils { } private class FileUtilsPermissionModification extends FileSystemPermissionModification::Range, - DataFlow::MethodCallNode { + DataFlow::CallNode { private DataFlow::Node permissionArg; FileUtilsPermissionModification() { diff --git a/ruby/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll b/ruby/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll index ed2222ff484f..7858ed030c01 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll @@ -26,8 +26,6 @@ class KernelMethodCall extends DataFlow::CallNode { ) } - string getMethodName() { result = methodCall.getMethodName() } - int getNumberOfArguments() { result = methodCall.getNumberOfArguments() } }