Skip to content

Commit

Permalink
Ruby: Restrict GraphQL remote flow sources
Browse files Browse the repository at this point in the history
Previously we considered any splat parameter in a graphql resolver to be
a remote flow source. Now we limit that to reads of the parameter which
yield scalar types (e.g. String), as defined by the GraphQL schema.

This should reduce GraphQL false positives.
  • Loading branch information
hmac committed Sep 14, 2023
1 parent b291ee3 commit 20f1a74
Show file tree
Hide file tree
Showing 5 changed files with 116 additions and 10 deletions.
91 changes: 81 additions & 10 deletions ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ class GraphqlFieldDefinitionMethodCall extends GraphqlSchemaObjectClassMethodCal

/** Gets the name of this GraphQL field. */
string getFieldName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }

GraphqlType getFieldType() { result = this.getArgument(1) }

Check warning on line 257 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlFieldDefinitionMethodCall::getFieldType/0

GraphqlFieldArgumentDefinitionMethodCall getArgumentCall() {

Check warning on line 259 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlFieldDefinitionMethodCall::getArgumentCall/0
result.getEnclosingCallable() = this.getBlock()
}
}

/**
Expand Down Expand Up @@ -289,6 +295,38 @@ private class GraphqlFieldArgumentDefinitionMethodCall extends GraphqlSchemaObje

/** Gets the name of the argument (i.e. the first argument to this `argument` method call) */
string getArgumentName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }

/** Gets the type of this argument */
GraphqlType getArgumentType() { result = this.getArgument(1) }
}

private DataFlow::LocalSourceNode graphQlEnum() {
result =
API::getTopLevelMember("GraphQL")
.getMember("Schema")
.getMember("Enum")
.getADescendentModule()
.getAnImmediateReference()
}

private class GraphqlType extends ConstantAccess {
Module getModule() { result.getAnImmediateReference() = this }

Check warning on line 313 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlType::getModule/0

GraphqlType getAField() { result = this.getField(_) }

Check warning on line 315 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlType::getAField/0

GraphqlType getField(string name) {

Check warning on line 317 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlType::getField/1
result =
any(GraphqlFieldDefinitionMethodCall field |
field.getFieldName() = name and
this.getModule().getADeclaration() = field.getReceiverClass()
).getFieldType()
}

predicate isEnum() { graphQlEnum().asExpr().getExpr() = this }

Check warning on line 325 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlType::isEnum/0

predicate isUserControlled() { this.getName() = ["String", "ID", "JSON"] }

Check warning on line 327 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlType::isUserControlled/0

predicate isScalar() { not exists(this.getAField()) and not this.isEnum() }

Check warning on line 329 in ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate GraphQL::GraphqlType::isScalar/0
}

/**
Expand Down Expand Up @@ -363,16 +401,9 @@ class GraphqlFieldResolutionMethod extends Method, Http::Server::RequestHandler:
override Parameter getARoutedParameter() {
result = this.getAParameter() and
exists(GraphqlFieldArgumentDefinitionMethodCall argDefn |
argDefn.getEnclosingCallable() = this.getDefinition().getBlock() and
(
result.(KeywordParameter).hasName(argDefn.getArgumentName())
or
// TODO this will cause false positives because now *anything* in the **args
// param will be flagged as RoutedParameter/RemoteFlowSource, but really
// only the hash keys corresponding to the defined arguments are user input
// others could be things defined in the `:extras` keyword argument to the `argument`
result instanceof HashSplatParameter // often you see `def field(**args)`
)
argDefn = this.getDefinition().getArgumentCall()
|
result.(KeywordParameter).hasName(argDefn.getArgumentName())
)
}

Expand All @@ -383,3 +414,43 @@ class GraphqlFieldResolutionMethod extends Method, Http::Server::RequestHandler:
/** Gets the class containing this method. */
GraphqlSchemaObjectClass getGraphqlClass() { result = schemaObjectClass }
}

private DataFlow::CallNode hashAccess(DataFlow::Node recv, string key) {
result.asExpr() instanceof ExprNodes::ElementReferenceCfgNode and
result.getArgument(0).getConstantValue().isStringlikeValue(key) and
result.getReceiver() = recv
}

private DataFlow::CallNode parameterAccess(
GraphqlFieldResolutionMethod method, GraphqlFieldArgumentDefinitionMethodCall def,
HashSplatParameter param, string key, GraphqlType type
) {
param = method.getARoutedParameter() and
def = method.getDefinition().getArgumentCall() and
(
// Direct access to the params hash
def.getArgumentType() = type and
def.getArgumentName() = key and
exists(DataFlow::Node paramRead |
paramRead.asExpr().getExpr() = param.getVariable().getAnAccess().(VariableReadAccess) and
result = hashAccess(paramRead, key)
)
or
// Nested access
exists(GraphqlType type2 |
parameterAccess(method, _, param, _, type2)
.(DataFlow::LocalSourceNode)
.flowsTo(result.getReceiver()) and
result = hashAccess(_, key) and
type2.getField(key) = type
)
)
}

private class GraphqlParameterAccess extends RemoteFlowSource::Range {
GraphqlParameterAccess() {
exists(GraphqlType type | this = parameterAccess(_, _, _, _, type) and type.isScalar())
}

override string getSourceType() { result = "GraphQL" }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class Types::BaseEnum < GraphQL::Schema::Enum
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module Types
class MediaCategory < Types::BaseEnum
value "AUDIO", "An audio file, such as music or spoken word"
value "IMAGE", "A still image, such as a photo or graphic"
value "TEXT", "Written words"
value "VIDEO", "Motion picture, may have audio"
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class Types::Post < GraphQL::Schema::Object
field :title, String
field :body, String, null: false
field :media_category, Types::MediaCategory, null: false
end
end

Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,23 @@ def with_splat_and_named_arg(arg1:, **rest)
def foo(arg)
system("echo #{arg}")
end

field :with_enum, String, null: false, description: "A field with an enum argument" do
argument :enum, Types::MediaCategory, "An enum", required: true
argument :arg2, String, "Another arg", required: true
end
def with_enum(**args)
system("echo #{args[:enum]}")
system("echo #{args[:arg2]}")
end

field :with_nested_enum, String, null: false, description: "A field with a nested enum argument" do
argument :inner, Types::Post, "Post", required: true
end
def with_nested_enum(**args)
system("echo #{args[:inner]}")
system("echo #{args[:inner][:title]}")
system("echo #{args[:inner][:media_category]}")
end
end
end

0 comments on commit 20f1a74

Please sign in to comment.