Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions go/ql/lib/semmle/go/frameworks/K8sIoApimachineryPkgRuntime.qll
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ module K8sIoApimachineryPkgRuntime {
}
}

private class DecoderDecode extends Method, UnmarshalingFunction::Range {
private class DecoderDecode extends UnmarshalingFunction::Range, Method {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a coding-style policy that could be written as a ql-for-ql check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. I don't know if other language libraries follow the same convention, but in the Go library we aim to always puts a Range supertype first, followed by other supertypes. I'm not sure it's worth the effort though. Maybe if people agree to this convention for all language libraries.

DecoderDecode() {
this.implements(packagePath(), "Decoder", "Decode") or
this.hasQualifiedName(packagePath(), "WithoutVersionDecoder", "Decode")
Expand Down Expand Up @@ -95,7 +95,7 @@ module K8sIoApimachineryPkgRuntime {
}
}

private class ParameterCodecDecodeParameters extends Method, UnmarshalingFunction::Range {
private class ParameterCodecDecodeParameters extends UnmarshalingFunction::Range, Method {
ParameterCodecDecodeParameters() {
this.implements(packagePath(), "ParameterCodec", "DecodeParameters")
}
Expand All @@ -110,7 +110,7 @@ module K8sIoApimachineryPkgRuntime {
}
}

private class ParameterCodecEncodeParameters extends Method, MarshalingFunction::Range {
private class ParameterCodecEncodeParameters extends MarshalingFunction::Range, Method {
ParameterCodecEncodeParameters() {
this.implements(packagePath(), "ParameterCodec", "EncodeParameters")
}
Expand All @@ -125,7 +125,7 @@ module K8sIoApimachineryPkgRuntime {
}
}

private class ProtobufMarshallerMarshalTo extends Method, MarshalingFunction::Range {
private class ProtobufMarshallerMarshalTo extends MarshalingFunction::Range, Method {
ProtobufMarshallerMarshalTo() {
this.implements(packagePath(), "ProtobufMarshaller", "MarshalTo") or
this.implements(packagePath(), "ProtobufReverseMarshaller", "MarshalToSizedBuffer")
Expand All @@ -138,7 +138,7 @@ module K8sIoApimachineryPkgRuntime {
override string getFormat() { result = "protobuf" }
}

private class RawExtensionMarshal extends Method, MarshalingFunction::Range {
private class RawExtensionMarshal extends MarshalingFunction::Range, Method {
RawExtensionMarshal() { this.hasQualifiedName(packagePath(), "RawExtension", "Marshal") }

override DataFlow::FunctionInput getAnInput() { result.isReceiver() }
Expand All @@ -148,7 +148,7 @@ module K8sIoApimachineryPkgRuntime {
override string getFormat() { result = "protobuf" }
}

private class RawExtensionUnmarshal extends Method, UnmarshalingFunction::Range {
private class RawExtensionUnmarshal extends UnmarshalingFunction::Range, Method {
RawExtensionUnmarshal() { this.hasQualifiedName(packagePath(), "RawExtension", "Unmarshal") }

override DataFlow::FunctionInput getAnInput() { result.isReceiver() }
Expand All @@ -158,7 +158,7 @@ module K8sIoApimachineryPkgRuntime {
override string getFormat() { result = "protobuf" }
}

private class UnknownMarshal extends Method, MarshalingFunction::Range {
private class UnknownMarshal extends MarshalingFunction::Range, Method {
string methodName;

UnknownMarshal() {
Expand All @@ -177,7 +177,7 @@ module K8sIoApimachineryPkgRuntime {
override string getFormat() { result = "protobuf" }
}

private class UnknownUnmarshal extends Method, UnmarshalingFunction::Range {
private class UnknownUnmarshal extends UnmarshalingFunction::Range, Method {
UnknownUnmarshal() { this.hasQualifiedName(packagePath(), "Unknown", "Unmarshal") }

override DataFlow::FunctionInput getAnInput() { result.isReceiver() }
Expand Down
2 changes: 1 addition & 1 deletion go/ql/lib/semmle/go/frameworks/Revel.qll
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ module Revel {
/**
* A render of a template.
*/
abstract class TemplateRender extends DataFlow::Node, TemplateInstantiation::Range {
abstract class TemplateRender extends TemplateInstantiation::Range {
/** Gets the name of the file that is rendered. */
abstract File getRenderedFile();

Expand Down
2 changes: 1 addition & 1 deletion go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ module NetHttp {
}
}

private class MapWrite extends Http::HeaderWrite::Range, DataFlow::Node {
private class MapWrite extends Http::HeaderWrite::Range {
DataFlow::Node index;
DataFlow::Node rhs;

Expand Down
2 changes: 1 addition & 1 deletion go/ql/lib/semmle/go/frameworks/stdlib/Regexp.qll
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ module Regexp {
}
}

private class DefaultRegexpMatchFunction extends RegexpMatchFunction::Range, Function {
private class DefaultRegexpMatchFunction extends RegexpMatchFunction::Range {
int patArg;
int strArg;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ module CleartextLogging {
*
* This is a source since `log.Print(obj)` will often show the fields of `obj`.
*/
private class StructPasswordFieldSource extends DataFlow::Node, Source {
private class StructPasswordFieldSource extends Source {
string name;

StructPasswordFieldSource() {
Expand All @@ -137,7 +137,7 @@ module CleartextLogging {
}

/** An access to a variable or property that might contain a password. */
private class ReadPasswordSource extends DataFlow::Node, Source {
private class ReadPasswordSource extends Source {
string name;

ReadPasswordSource() {
Expand All @@ -162,7 +162,7 @@ module CleartextLogging {
}

/** A call that might return a password. */
private class CallPasswordSource extends DataFlow::CallNode, Source {
private class CallPasswordSource extends Source, DataFlow::CallNode {
string name;

CallPasswordSource() {
Expand Down
10 changes: 5 additions & 5 deletions go/ql/lib/semmle/go/security/OpenUrlRedirectCustomizations.qll
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ module OpenUrlRedirect {
/**
* An HTTP redirect, considered as a sink for `Configuration`.
*/
class RedirectSink extends Sink, DataFlow::Node {
class RedirectSink extends Sink {
RedirectSink() { this = any(Http::Redirect redir).getUrl() }
}

/**
* A definition of the HTTP "Location" header, considered as a sink for
* `Configuration`.
*/
class LocationHeaderSink extends Sink, DataFlow::Node {
class LocationHeaderSink extends Sink {
LocationHeaderSink() {
exists(Http::HeaderWrite hw | hw.getHeaderName() = "location" | this = hw.getValue())
}
Expand All @@ -95,20 +95,20 @@ module OpenUrlRedirect {
* A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is
* considered a barrier guard for sanitizing untrusted URLs.
*/
class RedirectCheckBarrierGuardAsBarrierGuard extends RedirectCheckBarrier, Barrier { }
class RedirectCheckBarrierGuardAsBarrierGuard extends Barrier instanceof RedirectCheckBarrier { }

/**
* A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs.
*
* This is overapproximate: we do not attempt to reason about the correctness of the regexp.
*/
class RegexpCheckAsBarrierGuard extends RegexpCheckBarrier, Barrier { }
class RegexpCheckAsBarrierGuard extends Barrier instanceof RegexpCheckBarrier { }

/**
* A check against a constant value or the `Hostname` function,
* considered a barrier guard for url flow.
*/
class UrlCheckAsBarrierGuard extends UrlCheckBarrier, Barrier { }
class UrlCheckAsBarrierGuard extends Barrier instanceof UrlCheckBarrier { }
}

/** A sink for an open redirect, considered as a sink for safe URL flow. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,15 @@ module RequestForgery {
* A call to a function called `isLocalUrl`, `isValidRedirect`, or similar, which is
* considered a barrier guard.
*/
class RedirectCheckBarrierGuardAsBarrierGuard extends RedirectCheckBarrier, Sanitizer { }
class RedirectCheckBarrierGuardAsBarrierGuard extends Sanitizer instanceof RedirectCheckBarrier {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this make sense as a ql-for-ql check too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern we are looking for is "extend an abstract class by adding all instances of another class". I'm not totally sure how you'd search for this using ql-for-ql. It relies a lot on knowing the intention. I used the regex extends [^@,{ ]+, [^@{ ]+ and there are lots of results for go which I skipped over because I can see they don't match the pattern. One example is database types, which I've excluded by putting the @ in the regex. Another is when one of the types is a branch of a newtype. I suppose you might be able to have a query like "if type X is an extending subtype of non-abstract type Y, but X doesn't use any of the member predicates of Y in its char pred or member predicates, then it could be a non-extending subtype of Y instead". I don't know if that would be something we'd always want to apply.

}

/**
* A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs.
*
* This is overapproximate: we do not attempt to reason about the correctness of the regexp.
*/
class RegexpCheckAsBarrierGuard extends RegexpCheckBarrier, Sanitizer { }
class RegexpCheckAsBarrierGuard extends Sanitizer instanceof RegexpCheckBarrier { }

/**
* An equality check comparing a data-flow node against a constant string, considered as
Expand All @@ -114,7 +115,7 @@ module RequestForgery {
* Additionally, a check comparing `url.Hostname()` against a constant string is also
* considered a barrier guard for `url`.
*/
class UrlCheckAsBarrierGuard extends UrlCheckBarrier, Sanitizer { }
class UrlCheckAsBarrierGuard extends Sanitizer instanceof UrlCheckBarrier { }

/**
* A simple-typed node, considered a sanitizer for request forgery.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ module UnsafeUnzipSymlink {
* An argument to a call to `os.Symlink` within a loop that extracts a zip or tar archive,
* taken as a sink for unsafe unzipping of symlinks.
*/
class OsSymlink extends DataFlow::Node, SymlinkSink {
class OsSymlink extends SymlinkSink {
OsSymlink() {
exists(DataFlow::CallNode n | n.asExpr() = getASymlinkCall() |
this = n.getArgument([0, 1]) and
Expand All @@ -139,7 +139,7 @@ module UnsafeUnzipSymlink {
* An argument to `path/filepath.EvalSymlinks` or `os.Readlink`, taken as a sink for detecting target
* paths that are likely safe to extract to.
*/
class StdlibSymlinkResolvers extends DataFlow::Node, EvalSymlinksSink {
class StdlibSymlinkResolvers extends EvalSymlinksSink {
StdlibSymlinkResolvers() {
exists(DataFlow::CallNode n |
n.getTarget().hasQualifiedName("path/filepath", "EvalSymlinks")
Expand Down
6 changes: 3 additions & 3 deletions go/ql/lib/semmle/go/security/Xss.qll
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,14 @@ module SharedXss {
* A `Template` from `html/template` will HTML-escape data automatically
* and therefore acts as a sanitizer for XSS vulnerabilities.
*/
class HtmlTemplateSanitizer extends Sanitizer, DataFlow::Node {
class HtmlTemplateSanitizer extends Sanitizer {
HtmlTemplateSanitizer() {
exists(Method m, DataFlow::CallNode call | m = call.getCall().getTarget() |
m.hasQualifiedName("html/template", "Template", "ExecuteTemplate") and
call.getArgument(2) = this
this = call.getArgument(2)
or
m.hasQualifiedName("html/template", "Template", "Execute") and
call.getArgument(1) = this
this = call.getArgument(1)
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion go/ql/lib/semmle/go/security/ZipSlipCustomizations.qll
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ module ZipSlip {
/**
* A tar file header, as a source for zip slip.
*/
class TarHeaderSource extends Source, DataFlow::Node {
class TarHeaderSource extends Source {
TarHeaderSource() {
this =
any(DataFlow::MethodCallNode mcn |
Expand Down
2 changes: 1 addition & 1 deletion go/ql/src/Security/CWE-209/StackTraceExposure.ql
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ module StackTraceExposureConfig implements DataFlow::ConfigSig {
predicate isSink(DataFlow::Node sink) { sink instanceof Http::ResponseBody }

predicate isBarrier(DataFlow::Node node) {
// Sanitise everything controlled by an is-debug-mode check.
// Sanitize everything controlled by an is-debug-mode check.
// Imprecision: I don't try to guess which arm of a branch is intended
// to mean debug mode, and which is production mode.
exists(ControlFlow::ConditionGuardNode cgn |
Expand Down
4 changes: 1 addition & 3 deletions go/ql/src/Security/CWE-352/ConstantOauth2State.ql
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,7 @@ predicate privateUrlFlowsToAuthCodeUrlCall(DataFlow::CallNode call) {

module FlowToPrintConfig implements DataFlow::ConfigSig {
additional predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
exists(LoggerCall logCall | call = logCall |
sink = logCall.getAValueFormattedMessageComponent()
)
sink = call.(LoggerCall).getAValueFormattedMessageComponent()
}

predicate isSource(DataFlow::Node source) { source = any(AuthCodeUrl m).getACall().getResult() }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ module ImproperLdapAuth {
*
* This is overapproximate: we do not attempt to reason about the correctness of the regexp.
*/
class RegexpCheckAsBarrierGuard extends RegexpCheckBarrier, LdapSanitizer { }
class RegexpCheckAsBarrierGuard extends LdapSanitizer instanceof RegexpCheckBarrier { }

/**
* An empty string.
Expand Down
4 changes: 2 additions & 2 deletions go/ql/src/experimental/CWE-918/SSRF.qll
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ module ServerSideRequestForgery {
*
* This is overapproximate: we do not attempt to reason about the correctness of the regexp.
*/
class RegexpCheckAsBarrierGuard extends RegexpCheckBarrier, Sanitizer { }
class RegexpCheckAsBarrierGuard extends Sanitizer instanceof RegexpCheckBarrier { }

private predicate equalityAsSanitizerGuard(DataFlow::Node g, Expr e, boolean outcome) {
exists(DataFlow::Node url, DataFlow::EqualityTestNode eq |
Expand Down Expand Up @@ -156,5 +156,5 @@ module ServerSideRequestForgery {
* The method Var of package validator is a sanitizer guard only if the check
* of the error binding exists, and the tag to check is one of "alpha", "alphanum", "alphaunicode", "alphanumunicode", "number", "numeric".
*/
class ValidatorAsSanitizer extends Sanitizer, ValidatorVarCheckBarrier { }
class ValidatorAsSanitizer extends Sanitizer instanceof ValidatorVarCheckBarrier { }
}
Loading