Insecure Bean Validation query

[pwntester]

No description provided.

[ghost]

All committers have signed the CLA.

[felicitymay]

I should have mentioned earlier, I was added as a reviewer automatically by the CODEOWNERS file. However this PR doesn't need a review from the docs team because it's changing the experimental directory. We've since updated the CODEOWNERS file to reflect this.

[pwntester]

@github/codeql-java what is the status if this issue? I think @adityasharad wrote a more complete and precise query for this one. We should probably add a query for insecure bean validation asap since we have blog posts, workshops and talks about it. Im pretty sure some of our customers have these issues in their codebases

[aschackmull]

Looks like status is "fallen through the cracks". Where is this "more complete and precise query"? I could look into reviewing and merging this PR, but just want to know whether there's a different query I ought to be looking at instead?

[pwntester]

I found this on the security_lab slack channel while reviewing it after coming back from the leave:

/**
 * @name Insecure Bean validation
 * @description User-controlled data may be evaluated as Java EL expressions, leading to arbitrary code execution.
 * @kind path-problem
 * @problem.severity error
 * @precision high
 * @id java/ctf/insecure-bean-validation
 * @tags security
 *       external/cwe/cwe-094
 */

import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.FlowSources
import DataFlow::PathGraph

/** Models the JSR 380 constraint validation code in the package `javax.validation`. Collapse in the IDE for readability. */
module ConstraintValidation {
  /** The generic interface `javax.validation.ConstraintValidator`. */
  class TypeConstraintValidator extends Interface {
    TypeConstraintValidator() { this.hasQualifiedName("javax.validation", "ConstraintValidator") }

    /** Gets a method named `isValid` declared in this interface. */
    Method getIsValidMethod() {
      result.getDeclaringType() = this and
      result.hasName("isValid")
    }
  }

  /**
   * An implementation of the method named `isValid` from the
   * interface `javax.validation.ConstraintValidator`.
   */
  class ConstraintValidatorIsValidMethod extends Method {
    ConstraintValidatorIsValidMethod() {
      this.overridesOrInstantiates*(any(TypeConstraintValidator t).getIsValidMethod())
    }

    Parameter getValidatedParameter() {
      // The first parameter of this `isValid` method
      result = this.getParameter(0) and
      // which must be present in the source code
      this.fromSource()
    }
  }

  /** The type `javax.validation.ConstraintValidatorContext`. */
  class TypeConstraintValidatorContext extends RefType {
    TypeConstraintValidatorContext() {
      this.hasQualifiedName("javax.validation", "ConstraintValidatorContext")
    }
  }

  /**
   * A method named `buildConstraintViolationWithTemplate` declared on a subtype
   * of `javax.validation.ConstraintValidatorContext`.
   */
  class BuildConstraintViolationWithTemplateMethod extends Method {
    BuildConstraintViolationWithTemplateMethod() {
      this.getDeclaringType().getASupertype*() instanceof TypeConstraintValidatorContext and
      this.hasName("buildConstraintViolationWithTemplate")
    }
  }

  /** The interface `javax.validation.Constraint`. */
  class TypeConstraint extends Interface {
    TypeConstraint() { this.hasQualifiedName("javax.validation", "Constraint") }
  }

  /** A `@Constraint` annotation. */
  class ConstraintAnnotation extends Annotation {
    ConstraintAnnotation() { this.getType() instanceof TypeConstraint }

    /** Holds if this constraint is validated by the class `validatorType`. */
    predicate isValidatedBy(RefType validatorType) {
      validatorType =
        this.getValue("validatedBy").(ArrayInit).getAnInit().(TypeLiteral).getTypeName().getType()
    }
  }

  /**
   * The first parameter of a method that implements `ConstraintValidator.isValid`.
   */
  class BeanValidationSource extends DataFlow::Node {
    BeanValidationSource() {
      exists(ConstraintValidatorIsValidMethod isValidMethod |
        this.asParameter() = isValidMethod.getValidatedParameter()
      )
    }
  }

  /** Holds if `ma` gets the field `f` of the object in `qualifier`. */
  predicate getsField(MethodAccess ma, Expr qualifier, Field f) {
    ma.getMethod().(GetterMethod).getField() = f and
    qualifier = ma.getQualifier()
  }

  /**
   * Holds if `isValidMethod` is declared on the constraint validator `validatorType`
   * and is used to validate the field/property `validatedField`,
   * whose value may be obtained from the user-controlled `remoteInput`.
   */
  predicate validationFlowStep(DataFlow::Node source, DataFlow::Node target) {
    exists(
      Field validatedField, RefType validatorType, ConstraintValidatorIsValidMethod isValidMethod,
      BeanValidationSource validatedParameter
    |
      // The source is an assignment to a field
      source.asExpr() = validatedField.getAnAssignedValue() and
      // The target is a use of the field within an `isValid` method
      target.getEnclosingCallable() = isValidMethod and
      validatorType = isValidMethod.getDeclaringType() and
      validatedParameter.asParameter() = isValidMethod.getValidatedParameter() and
      (
        // that validates the field directly
        validatedConstraint(validatedField, _, _, validatorType) and
        target = validatedParameter
        or
        // or that validates the class
        validatedConstraint(validatedField.getDeclaringType(), _, _, validatorType) and
        exists(DataFlow::ExprNode qualifier |
          getsField(target.asExpr(), qualifier.asExpr(), validatedField) and
          DataFlow::localFlow(validatedParameter, qualifier)
        )
      )
    )
  }

  /**
   * Holds if `validatedElement` is annotated with a validation constraint defined by `constraintType`,
   * which in turn is annotated with `constraintAnnotation` and validated by `validatorType`.
   */
  predicate validatedConstraint(
    Annotatable validatedElement, RefType constraintType, ConstraintAnnotation constraintAnnotation,
    RefType validatorType
  ) {
    validatedElement.getAnAnnotation().getType() = constraintType and
    constraintType.getAnAnnotation() = constraintAnnotation and
    constraintAnnotation.isValidatedBy(validatorType)
  }

  /**
   * Taint tracking step from a field write within a constructor to a read of the same field
   * within a validator method that checks its constraints.
   * 
   * When an object of the class is constructed, or the field is initialised,
   * the validator method will run on the same object or field value, so this is a global taint step.
   */
  class ValidationTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) { validationFlowStep(n1, n2) }
  }
}

/** Additional taint steps needed to detect the entire flow path. Collapse this in the IDE for readability. */
module AdditionalTaintSteps {
  /** Tracks the flow of taint through getter calls: `x -> x.get*(...)`. */
  class GetterTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(MethodAccess ma |
        ma.getMethod() instanceof GetterMethod and
        n1.asExpr() = ma.getQualifier() and
        n2.asExpr() = ma
      )
    }
  }

  /** A call to the method `keySet()` declared on a subtype of `java.util.Map`. */
  class MapKeySetCall extends MethodAccess {
    MapKeySetCall() { this.getMethod().(MapMethod).getName() = "keySet" }
  }

  /** Tracks the flow of taint from `x` to `x.keySet()`. */
  class KeySetTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(MapKeySetCall call |
        n1.asExpr() = call.getQualifier() and
        n2.asExpr() = call
      )
    }
  }

  /** A call to the constructor `java.util.HashSet<>(...)`. */
  class HashSetConstructorCall extends Call {
    HashSetConstructorCall() {
      this
          .(ConstructorCall)
          .getConstructedType()
          .getSourceDeclaration()
          .hasQualifiedName("java.util", "HashSet")
    }
  }

  /** Tracks the flow of taint from `x` to `new HashSet(x)`. */
  class HashSetTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(HashSetConstructorCall call |
        n1.asExpr() = call.getAnArgument() and
        n2.asExpr() = call
      )
    }
  }

  /** A call to the method `stream` declared in a collection type. */
  class CollectionStreamCall extends MethodAccess {
    CollectionStreamCall() { this.getMethod().(CollectionMethod).getName() = "stream" }
  }

  /** Track taint from `x` to `x.stream()` where `x` is a collection. */
  class CollectionStreamTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(CollectionStreamCall call |
        n1.asExpr() = call.getQualifier() and
        n2.asExpr() = call
      )
    }
  }

  /** The interface `java.util.stream.Stream`. */
  class TypeStream extends Interface {
    TypeStream() { this.hasQualifiedName("java.util.stream", "Stream") }
  }

  /** A method declared in a stream type, that is, a subtype of `java.util.stream.Stream`. */
  class StreamMethod extends Method {
    StreamMethod() { this.getDeclaringType().getASourceSupertype+() instanceof TypeStream }
  }

  /** A call to the method `map` or `collect` declared in a stream type. */
  class StreamMapCall extends MethodAccess {
    StreamMapCall() { this.getMethod().(StreamMethod).getName() = ["map", "collect"] }
  }

  /** Track taint from `stream` to `stream.map(lambda)` or `stream.collect(lambda)`. */
  class StreamMapTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(StreamMapCall call |
        n1.asExpr() = call.getQualifier() and
        n2.asExpr() = call
      )
    }
  }

  /**
   * Tracks the flow of data from a protobuf message to a corresponding Java object,
   * created using a method called `toCore<Name>`.
   * This additional step is used to track from remote input parameters
   * to bean validation of their values.
   */
  class ProtoToCoreTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(MethodAccess ma |
        ma.getMethod().getName().matches("toCore%") and
        n2.asExpr() = ma and
        n1.asExpr() = ma.getArgument(0)
      )
    }
  }

  /**
   * Tracks the flow of data through utility methods in the class `CollectionsExt`.
   * This additional step is used to track from remote input parameters
   * to bean validation of their values.
   */
  class CollectionsExtTaintStep extends TaintTracking::AdditionalTaintStep {
    override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
      exists(MethodAccess ma |
        ma.getMethod().getName() = ["nullableImmutableCopyOf", "nonNull"] and
        ma.getMethod().getDeclaringType().getName() = "CollectionsExt" and
        n2.asExpr() = ma and
        n1.asExpr() = ma.getArgument(0)
      )
    }
  }
}

/**
 * Taint tracking BeanValidationConfiguration describing the flow of data from user input
 * to the argument of a method that builds constraint error messages.
 */
class BeanValidationConfig extends TaintTracking::Configuration {
  BeanValidationConfig() { this = "BeanValidationConfig" }

  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }

  override predicate isSink(DataFlow::Node sink) {
    exists(MethodAccess ma |
      ma.getMethod() instanceof ConstraintValidation::BuildConstraintViolationWithTemplateMethod and
      sink.asExpr() = ma.getArgument(0)
    )
  }
}


from BeanValidationConfig c, DataFlow::PathNode source, DataFlow::PathNode sink
where c.hasFlowPath(source, sink)
select sink, source, sink, "Error message contains unsanitized user data"

I assumed its more complete and accurate because its from @adityasharad :D but havent really look into it yet. Lets wait for @adityasharad feedback and move from there

[adityasharad]

Yes: the version of the query Alvaro just shared traces the entire flow from the actual remote flow source, through classes/fields and their corresponding validators, and then to the sink within the validation method.

It seems to work in its current form (at least for the target codebase), but I expect there are two potential areas of future cleanup:

• the javax.validation modelling may be better placed in the standard library
• the validation flow step currently goes from the RHS of a field write to a read of the field within the validation method (either as a parameter or via a call to a getter method). This was necessary because one cannot currently write an additional flow step that is aware of fields or access paths, but could be more precise when that becomes possible.

[pwntester]

Even though getting the full trace from the real remote source to the sink is great, we may want to keep the source at isValid(0) since in some cases having the complete flow will not be possible (eg: in Nexus some of the REST endpoints where implemented in Groovy) and isValid should be a good approximation. Is it possible to filter out subflows (eg: remove a flow (eg: isValid->sink) if its contained within other, larger flow (eg: REST endpoint -> sink))

Also, the AdditionalTaintSteps module contains many nodes that are unrelated to this query (although needed to get the issue reported in the netflix project) that should probably be taken out to a different PR. eg:

• java.util.Map.keySet()
• java.util.HashSet.init^()
• java.util.Collection.stream()
• java.util.stream.Stream.collect()
• java.util.stream.Stream.map()
• Protobuf toCore.*
• CollectionsExt

[aschackmull]

Is this a remote flow source that ought to apply to all security queries or only this one? Conversely, does this query intend to use all remote flow sources as sources or just the BeanValidationSource?

[pwntester]

In theory, the validated bean properties could reach a different sink (eg: a SQLi one) so this source need to be accounted for all security queries. Ideally the source should not be necessary but that would require taint steps like the ones in @adityasharad version of the query. That version of the query missed the new issues so I would stick to this source and treat it as a generic remote source.

@adityasharad are you ok with this? otherwise we can try to debug your query and see why its not working for the new cases.

[aschackmull]

This taint step seems too ad-hoc, please delete.

[pwntester]

ok, will do, but that taintstep was required for the Netflix findings. In some cases, the way to validate a bean property is parsing it and consider it valid if it does not through an exception. Otherwise include the exception message in the bean validation error message. In that case, if the user input is reflected in the exception message, then it will lead to a real issue.

[aschackmull]

Taint flow through exceptions definitely is one of our blind spots, so I can understand the need for something like this. I would just like it to be a little more rigorous and then applied to all queries.

[aschackmull]

Could you insert some line breaks? Might as well make the qhelp a bit easier to read here in the diff view.

[aschackmull]

This could use a qltest.

[aschackmull]

Could you perhaps apply a maximum line width of 80-120 or something like that for this entire file?

[pwntester]

should I include the MD links on that limit considering they shouldnt be visible in VSCode?

[aschackmull]

Missing pieces:

• Move this out of experimental.
• Move the source to FlowSources.qll to make it apply to all queries (assuming that we'll want this - otherwise it shouldn't extend RemoteFlowSource).
• Add qltest.
• Add change note.
• Documentation review.

[sj]

@jf205 and/or @shati-patel: could you take a look at the documentation for this query please? We're considering expediting its promotion from experimental into the regular query set.

[shati-patel]

I've added a few editorial suggestions 📝 😃

[pwntester]

Thanks for the feedback @shati-patel

[shati-patel]

Two more tiny things I missed previously 🙈