From 648a70945d58922bba3203031bf112d5c49f5b2a Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 4 Nov 2021 15:17:45 +0000 Subject: [PATCH] Copyedit docs and improve naming --- ql/src/experimental/CWE-918/SSRF.qll | 2 +- ql/src/experimental/CWE-918/validator.qll | 95 +++++++++++++---------- 2 files changed, 53 insertions(+), 44 deletions(-) diff --git a/ql/src/experimental/CWE-918/SSRF.qll b/ql/src/experimental/CWE-918/SSRF.qll index a43561892b7..c2d7156c489 100644 --- a/ql/src/experimental/CWE-918/SSRF.qll +++ b/ql/src/experimental/CWE-918/SSRF.qll @@ -153,7 +153,7 @@ module ServerSideRequestForgery { * no error, then we consider these fields safe for SSRF. */ class BodySanitizer extends Sanitizer { - BodySanitizer() { this instanceof BodyTagSanitizer } + BodySanitizer() { this instanceof CheckedAlphanumericStructFieldRead } } /** diff --git a/ql/src/experimental/CWE-918/validator.qll b/ql/src/experimental/CWE-918/validator.qll index 3805c926480..de58d5f4123 100644 --- a/ql/src/experimental/CWE-918/validator.qll +++ b/ql/src/experimental/CWE-918/validator.qll @@ -2,22 +2,25 @@ import go import semmle.go.dataflow.Properties /** - * Only these tags are safe to build an URL + * Holds if `validationKind` is a validation kind that restricts to alphanumeric characters, + * which we consider safe for use in a URL. */ -private predicate isValidTag(string tag) { - tag in ["alpha", "alphanum", "alphaunicode", "alphanumunicode", "number", "numeric", "uuid"] +private predicate isAlphanumericValidationKind(string validationKind) { + validationKind in [ + "alpha", "alphanum", "alphaunicode", "alphanumunicode", "number", "numeric", "uuid" + ] } /** - * Struct's field with json tags like `key:"value1,value2"` + * A struct field with json tags like `key:"value1,value2"`. */ class FieldWithTags extends FieldDecl { FieldWithTags() { this.getTag().toString().regexpMatch("`([a-zA-Z0-9]+:\"[a-zA-Z0-9,]+\" *)+`") } /** - * It retrieves the values of a key - * For example: `json:"word" binding:"required,alpha"` key: "json" value:"word" - * key: "binding" values: "required","alpha". + * Holds if this field's tag maps `key` to `value`. + * For example: the tag `json:"word" binding:"required,alpha"` yields `key: "json", value: "word"` + * and `key: "binding" values: "required","alpha"`. */ predicate getTagByKeyValue(string key, string value) { exists(string tag, string key_value, string values | @@ -35,18 +38,18 @@ class FieldWithTags extends FieldDecl { } /** - * If the tainted variable comes from a valid tagged field (for example: binding:"alpha") - * it is safe + * A node that reads from a field with a tag indicating it + * must be alphanumeric (for example, having the tag `binding:"alpha"`). */ -class TaggedNode extends DataFlow::Node { +class AlphanumericStructFieldRead extends DataFlow::Node { string key; - TaggedNode() { + AlphanumericStructFieldRead() { exists(FieldWithTags decl, Field field, string tag | this = field.getARead() and field.getDeclaration() = decl.getNameExpr(0) and decl.getTagByKeyValue(key, tag) and - isValidTag(tag) + isAlphanumericValidationKind(tag) ) } @@ -54,35 +57,36 @@ class TaggedNode extends DataFlow::Node { } /** - * When we receive a body from a request, we can use certain tags on our struct's fields to hint - * the binding function to run some validations for that field. If this binding functions returns - * no error, then we consider these fields safe for SSRF. + * A node that is considered safe because it (a) reads a field with a tag indicating it should be + * alphanumeric, and (b) is guarded by a call to a validation function checking that it really + * is alphanumeric. * - * See TaggedNode and isValidTag for supported tags. See BindErrorCheck for supported binding - * functions. + * See `AlphanumericStructFieldRead` and `isAlphanumericValidationKind` for supported tags. + * See `StructValidationFunction` for supported binding functions. */ -class BodyTagSanitizer extends TaggedNode { - BodyTagSanitizer() { - exists(BindErrorCheck guard, SelectorExpr selector | +class CheckedAlphanumericStructFieldRead extends AlphanumericStructFieldRead { + CheckedAlphanumericStructFieldRead() { + exists(StructValidationFunction guard, SelectorExpr selector | guard.getAGuardedNode().asExpr() = selector.getBase() and selector = this.asExpr() and - this.getKey() = guard.getSafeKey() + this.getKey() = guard.getValidationKindKey() ) } } /** - * An error check for a bind function considered as a barrier guard + * A function that validates a struct, checking that fields conform to restrictions given as a tag. + * + * The Gin `Context.Bind` family of functions apply checks according to a `binding:` tag, and the + * Go-Playground Validator checks fields that have a `validate:` tag. */ -private class BindErrorCheck extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode { +private class StructValidationFunction extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode { Expr checked; - boolean outcome; - string safeKey; + boolean safeOutcome; + string validationKindKey; - BindErrorCheck() { - exists( - Function bindFunction, DataFlow::CallNode bindCall, DataFlow::Node resultErr, Property prop - | + StructValidationFunction() { + exists(Function bindFunction, DataFlow::CallNode bindCall, DataFlow::Node resultErr | ( // Gin call bindFunction @@ -92,29 +96,34 @@ private class BindErrorCheck extends DataFlow::BarrierGuard, DataFlow::EqualityT "BindJSON", "MustBindWith", "BindWith", "Bind", "ShouldBind", "ShouldBindBodyWith", "ShouldBindJSON", "ShouldBindWith" ]) and - safeKey = "binding" + validationKindKey = "binding" or - //Validator Struct + // Validator Struct bindFunction .(Method) .hasQualifiedName("github.com/go-playground/validator", "Validate", "Struct") and - safeKey = "validate" + validationKindKey = "validate" ) and bindCall = bindFunction.getACall() and checked = dereference(bindCall.getAnArgument()) and resultErr = bindCall.getResult().getASuccessor*() and - prop.checkOn(this, outcome, resultErr) and - prop.isNil() + nilProperty().checkOn(this, safeOutcome, resultErr) ) } - override predicate checks(Expr e, boolean branch) { e = checked and branch = outcome } + override predicate checks(Expr e, boolean branch) { e = checked and branch = safeOutcome } - string getSafeKey() { result = safeKey } + /** + * Returns the struct tag key from which this validation function draws its validation kind. + * + * For example, if this returns `xyz` then this function looks for a struct tag like + * `` mustBeNumeric string `xyz:"numeric"` `` + */ + string getValidationKindKey() { result = validationKindKey } } /** - * If nd is of type &a, returns nodes for &a and a. Else, return nd as is. + * If `nd` is an address-of expression `&a`, returns expressions `&a` and `a`. Otherwise, returns `nd` as-is. */ private Expr dereference(DataFlow::Node nd) { nd.asExpr().(AddressExpr).getOperand() = result @@ -123,21 +132,21 @@ private Expr dereference(DataFlow::Node nd) { } /** - * A validation performed by package validator's method Var is a sanitizer guard - * only when using valid tags (see isValidTag for more information) + * A validation performed by package `validator`'s method `Var` to check that an expression is + * alphanumeric (see `isAlphanumericValidationKind` for more information) sanitizes guarded uses + * of the same variable. */ class ValidatorVarCheck extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode { DataFlow::CallNode callToValidator; boolean outcome; ValidatorVarCheck() { - exists(Method validatorMethod, Property prop, DataFlow::Node resultErr | + exists(Method validatorMethod, DataFlow::Node resultErr | validatorMethod.hasQualifiedName("github.com/go-playground/validator", "Validate", "Var") and callToValidator = validatorMethod.getACall() and - isValidTag(callToValidator.getArgument(1).getStringValue()) and + isAlphanumericValidationKind(callToValidator.getArgument(1).getStringValue()) and resultErr = callToValidator.getResult().getASuccessor*() and - prop.checkOn(this, outcome, resultErr) and - prop.isNil() + nilProperty().checkOn(this, outcome, resultErr) ) }