Unable to detect SQL injection vulnerability in gorm framework in go language

[LeslieLai1999]

When I used the rule of CWE-89 to detect the SQL injection vulnerability in the GO repository, no known vulnerability was detected. I found that the reason was that source and sink were not connected. The specific statement is as follows

sql := g.gorm.WithContext(ctx).Where(q)
if param.Direction == "" {
sql.Order(param.OrderBy + " desc")
} else {
sql.Order(param.OrderBy + " " + param.Direction)
}
}

The result of my analysis is that ctx parameters are not input by users, so there is no data flow, but I will not optimize the rules, can you give me some help

[ginsbach]

Thank you for reporting this.

You write that "no known vulnerability was detected". Do you think this is a false negative, as in you think there is a vulnerability that CodeQL should have detected? Can you specify precisely where you would have liked to see an alert?

Can you show more of the code for context?

[LeslieLai1999]

Yes yes, the following code snippet should be able to detect the SQL injection vulnerability, but it is not detected, here is a more detailed code snippet

sql := g.gorm.WithContext(ctx).Where(q)
if param.OrderBy == "" {
sql.Order("created_at desc")
} else {
if param.Direction == "" {
sql.Order(param.OrderBy + " desc")
} else {
sql.Order(param.OrderBy + " " + param.Direction)
}
}
ret := sql.Offset(param.Offset).Limit(limit).Find(xxx)

As you can see, in the above code, the Order parameter is external input, the OrderBy and Direction variables are user controlled, and when I move the WithContext(ctx) method to the Where or Order method, it will be checked out normally. In my opinion, because the parameter ctx of WithContext is not controlled by the user, when this position is detected, it is considered that the data stream does not exist, resulting in a false negative

[ginsbach]

Do I understand correctly that you are expecting alerts in lines 6 and 8, but are not getting any from CodeQL code scanning?

 1:   sql := g.gorm.WithContext(ctx).Where(q)
 2:   if param.OrderBy == "" {
 3:     sql.Order("created_at desc")
 4:   } else {
 5:   if param.Direction == "" {
 6:     sql.Order(param.OrderBy + " desc")
 7:   } else {
 8:     sql.Order(param.OrderBy + " " + param.Direction)
 9:   }
10: }
11: ret := sql.Offset(param.Offset).Limit(limit).Find(xxx)

when I move the WithContext(ctx) method to the Where or Order method, it will be checked out normally.

When you move WithContext(ctx), then you get an alert? Can you show the modified code please.

[LeslieLai1999]

Your understanding is correct，The following is the modified code, you can detect the existence of SQL injection vulnerabilities, but I do not know how to modify the rules, do not know how to modify the rules

sql := g.gorm.Where(q)
if param.OrderBy == "" {
sql.Order("created_at desc")
} else {
if param.Direction == "" {
sql.Order(param.OrderBy + " desc")
} else {
sql.Order(param.OrderBy + " " + param.Direction)
}
}
ret := sql.WithContext(ctx).Offset(param.Offset).Limit(limit).Find(xxx)

[smowton]

I'm afraid I can't reproduce the problem:

My go.mod:

module dfsdafds

go 1.21.1

require gorm.io/gorm v1.25.7

require (
    github.com/jinzhu/inflection v1.0.0 // indirect
    github.com/jinzhu/now v1.1.5 // indirect
)

My test.go:

package dsfsdfs

import (
  "gorm.io/gorm"
  "context"
  "net/http"
)

func test(ctx *context.Context, q string, req *http.Request) {

  orderBy := req.URL.Query()["a"][0]
  direction := req.URL.Query()["b"][0]

  db := gorm.DB{}
  sql := db.WithContext(*ctx).Where(q)
  if orderBy == "" {
    sql.Order("created_at desc")
  } else {
    if direction == "" {
      sql.Order(orderBy + " desc")
    } else {
      sql.Order(orderBy + " " + direction)
    }
  }

}

Query go/sql-injection then produces alerts concerning the uses of orderBy and direction as expected.

Can you supply a complete example of the problem you are seeing?

[LeslieLai1999]

I'm sorry, the above code does detect the SQL injection vulnerability, but the following code, I can not detect the vulnerability through the CWE-89, but it is clear that the vulnerability exists in the OrderKey parameter, either through the body parameter or through the URLParam parameter

package main

import (
    "fmt"
    "net/http"

    borm "git.in.zhihu.com/go/borm"
    sq "github.com/Masterminds/squirrel"
    "github.com/go-chi/chi"
    "github.com/go-chi/chi/middleware"
    "github.com/go-chi/render"
    _ "github.com/go-sql-driver/mysql"
)

type User struct {
    ID   int    `json:"id"`
    Name string `json:"name"`
}

func main() {
    router := chi.NewRouter()
    router.Use(middleware.Logger)
    router.Use(middleware.Recoverer)
    router.Get("/users", GetUser)
    fmt.Println("Server started on :8080")
    http.ListenAndServe(":8080", router)
}

func GetUser(w http.ResponseWriter, r *http.Request) {
    var userQuery struct {
        Name     string `json:"name"`
        OrderKey string `json:"OrderKey"`
    }

    if err := render.DecodeJSON(r.Body, &userQuery); err != nil {
        render.Status(r, http.StatusBadRequest)
        render.JSON(w, r, map[string]string{"error": err.Error()})
        return
    }

    userQuery.Name = chi.URLParam(r, "name")
    OrderKey := chi.URLParam(r, "OrderKey")

    db, err := borm.Open("mysql", "user:password@tcp(localhost:3306)/database")
    if err != nil {
        render.Status(r, http.StatusInternalServerError)
        render.JSON(w, r, map[string]string{"error": err.Error()})
        return
    }
    defer db.Close()
    columns := []string{"id", "name"}
    tableName := "users"
    builder := sq.Select(columns...).From(tableName)

    //query := squirrel.Select("id", "name").
    //  From("users").
    //  Where(squirrel.Eq{"name": userQuery.Name}).OrderBy(OrderKey + " desc")

    builder.Where(sq.Eq{"name": userQuery.Name})
    OrderKey = fmt.Sprintf("%s desc", OrderKey)
    sqlStr, args, _ := builder.OrderBy(OrderKey).ToSql()

    rows, err := db.Query(sqlStr, args...)
    if err != nil {
        render.Status(r, http.StatusInternalServerError)
        render.JSON(w, r, map[string]string{"error": err.Error()})
        return
    }
    defer rows.Close()

    var users []User
    for rows.Next() {
        var user User
        err := rows.Scan(&user.ID, &user.Name)
        if err != nil {
            render.Status(r, http.StatusInternalServerError)
            render.JSON(w, r, map[string]string{"error": err.Error()})
            return
        }
        users = append(users, user)
    }

    render.JSON(w, r, users)
}

[atorralba]

Hi @LeslieLai1999. Thanks for your report.

We looked into this and noticed we were missing a few sinks in Squirrel. We've modeled them in github/codeql#15812. Once that gets merged and released, your use case should be covered.

[LeslieLai1999]

Thank you very much, I can successfully detect the SQL injection vulnerability through your modified rules!