2

I have a simple Golang Gin API that uses MongoDB as the backend database. My team is using GitHub CodeQL, so we want to be sure we are following the best standards. However, we continue to get this error for all of our query endpoints:

Database query built from user-controlled sources

Here is a simplified sample endpoint:

func (l LogHandler) GetLogByFQDN(ctx *gin.Context) {
    // Bind
    var request dtos.GetLogRequest
    if err := ctx.BindJSON(&data); err != nil {
        ctx.AbortWithStatusJSON(http.StatusBadRequest, validator.DecryptErrors(err))
        return
    }

    // Prepare MongoDB query
    col := getCollection("logs")
    filter := bson.M{"config.fqdn": bson.M{"$eq": request.fqdn}}

    // Execute MongoDB query
    var data Log
    err := col.FindOne(context.TODO(), filter).Decode(&data)

    if err != nil {
        ...
    }

    ctx.JSON(
        http.StatusOK,
        data,
    )
}

The following resource advised to use $eq, so the code has been updated above, but still the issue persists.

https://codeql.github.com/codeql-query-help/javascript/js-sql-injection/

I have also tried adding my own sanitation function after bind, which would remove bad characters such as:

$,{,},,

However, this has also not resolved the issue.

I can just ignore these errors and move forward, but it would be great to clear this up for our build pipelines.

Kyle Barnes
  • 729
  • 3
  • 13
  • 22

1 Answers1

0

You are creating a MongoDB filter using the request.fqdn value that comes... straight from user input:

filter := bson.M{"config.fqdn": bson.M{"$eq": request.fqdn}}

If an attacker has control over the request.fqdn, they might be able to construct a value that modifies your query in unexpected ways, leading to a potential NoSQL injection attack.

The warning from CodeQL is trying to alert you to this potential vulnerability.

You can try and use a library like go-playground/validator/v10 to validate the fqdn from the request. If it is not a valid FQDN, you would need to return a 400 error to the user.

import (
    "github.com/go-playground/validator/v10"
)

var validate *validator.Validate

func init() {
    validate = validator.New()
}

func (l LogHandler) GetLogByFQDN(ctx *gin.Context) {
    // Bind
    var request dtos.GetLogRequest
    if err := ctx.BindJSON(&data); err != nil {
        ctx.AbortWithStatusJSON(http.StatusBadRequest, validator.DecryptErrors(err))
        return
    }

    // Validate FQDN first
    err := validate.Var(request.fqdn, "fqdn")
    if err != nil {
        ctx.AbortWithStatusJSON(http.StatusBadRequest, "Invalid FQDN")
        return
    }

    // Then prepare MongoDB query
    col := getCollection("logs")
    filter := bson.M{"config.fqdn": bson.M{"$eq": request.fqdn}}

    // Execute MongoDB query
    var data Log
    err := col.FindOne(context.TODO(), filter).Decode(&data)

    if err != nil {
        // ...
    }

    ctx.JSON(
        http.StatusOK,
        data,
    )
}

Make sure you have a fqdn tag in your GetLogRequest struct like:

type GetLogRequest struct {
    fqdn string `validate:"fqdn"`
    // other fields
}

If the error persists, consider CodeQL uses static analysis to determine the flow of potentially tainted (i.e., user-controlled) data through your code. Even if your code is safe in practice, if CodeQL detects a flow of tainted data into a sensitive operation, it can still flag it.

One way to approach this is to split the logic into separate functions:

  • one to validate and sanitize user input, and
  • another to execute the MongoDB query.

You can try and, first, separate the validation and sanitation:

func validateAndSanitizeFQDN(fqdn string) (string, error) {
    err := validate.Var(fqdn, "fqdn")
    if err != nil {
        return "", err
    }
    
    // Potentially, any other sanitization logic here
    sanitizedFQDN := fqdn // in this case, we trust the validation but you can apply more sanitation if needed

    return sanitizedFQDN, nil
}

Then modify your handler to use the validateAndSanitizeFQDN() function:

func (l LogHandler) GetLogByFQDN(ctx *gin.Context) {
    // Bind
    var request dtos.GetLogRequest
    if err := ctx.BindJSON(&data); err != nil {
        ctx.AbortWithStatusJSON(http.StatusBadRequest, validator.DecryptErrors(err))
        return
    }

    sanitizedFQDN, err := validateAndSanitizeFQDN(request.fqdn)
    if err != nil {
        ctx.AbortWithStatusJSON(http.StatusBadRequest, "Invalid FQDN")
        return
    }

    // Prepare MongoDB query using sanitized FQDN
    col := getCollection("logs")
    filter := bson.M{"config.fqdn": bson.M{"$eq": sanitizedFQDN}}

    // Execute MongoDB query
    var data Log
    err = col.FindOne(context.TODO(), filter).Decode(&data)
    if err != nil {
        // handle error
    }

    ctx.JSON(http.StatusOK, data)
}

By making this separation clearer, you are giving static analysis tools like CodeQL a better chance to understand that user input is being validated and sanitized before it affects the MongoDB query.


The OP Kyle Barnes adds in the comments:

I added this as well, but the issue still persists.
At this point I think we'll add the additional sanitation you advised, but then mark the error as a false positive.

VonC
  • 1,262,500
  • 529
  • 4,410
  • 5,250
  • Thanks for pointing out this package. I just tried it and retriggered my build, but CodeQL still throws the same error. I was hopeful that aborting as soon as an error occurs would count as sanitation, but this does not seem to be the case. – Kyle Barnes Aug 06 '23 at 06:01
  • @KyleBarnes OK. I have edited the answer to address your comment. – VonC Aug 06 '23 at 10:11
  • I added this as well, but the issue still persists. At this point I think we'll add the additional sanitation you advised, but then mark the error as a false positive. Thanks so much for the help. – Kyle Barnes Aug 06 '23 at 22:26
  • 1
    @KyleBarnes OK. I have edited the answer to include your comment for more visibility. – VonC Aug 07 '23 at 05:40