Gorm: SQL injection in Gorm With using first and find.

[wahyuhadi]

What version of Go are you using (go version)?

latest

Which database and its version are you using?

postgress latest and gorm latest

Blind Sql injection localhost:8000/user?id=id=1)) or 1=1--

func GetUser(c *gin.Context) {
    var user []models.User
    dbms := db.GetDatabaseConnection() /*  Open connectins */
    defer dbms.Close()
    id := c.Query("id")

    err := dbms.First(&user, id) // Sql Injection in this line /user?id=id=1)) or 1=1--
    if err == nil {
        c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "message": "success", "result": user})

    } else {
        c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "message": "something error", "result": err})
    }
    return

}

Need to runnable with GORM's docker compose config or please provides your config.

package main

import (
    "fmt"
    "net/http"

    "./db"
    "./models"
    "github.com/gin-gonic/gin"
)

// DBMigrate for Auto Migrate
func DBMigrate() { /* Auto Migrations */
    fmt.Println("[::] Migration Databases .....")
    dbms := db.GetDatabaseConnection() /* Get connction to database */
    defer dbms.Close()
    dbms.AutoMigrate(&models.User{})
    // db.AutoMigrate(&models.Profile{}) /* Migration Models */
    fmt.Println("[::] Migration Databases Done")
}

func main() {
    DBMigrate()
    router := gin.Default()
    router.POST("/user", CreateUser)
    router.GET("/user", GetUser)
    router.Run(":8080")
}

// CreateUser function
func CreateUser(c *gin.Context) {
    var (
        user models.User
    )

    c.BindJSON(&user)
    dbms := db.GetDatabaseConnection() /*  Open connectins */
    defer dbms.Close()
    dbms.Create(&user)
    c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "message": "success", "result": user})
    return
}

// GetUser function
func GetUser(c *gin.Context) {
    var user []models.User
    dbms := db.GetDatabaseConnection() /*  Open connectins */
    defer dbms.Close()
    id := c.Query("id")

    err := dbms.First(&user, id) // Sql Injection in this line /user?id=id=1)) or 1=1--
    if err == nil {
        c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "message": "success", "result": user})

    } else {
        c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "message": "something error", "result": err})
    }
    return

}

[wahyuhadi]

Your issue may already be reported! Please search on the issue track before creating one.

What version of Go are you using (go version)?

latest

Which database and its version are you using?

postgress latest and gorm latest

Please provide a complete runnable program to reproduce your issue. IMPORTANT

func GetUser(c *gin.Context) {
  var user []models.User
  dbms := db.GetDatabaseConnection() /*  Open connectins */
  defer dbms.Close()
  id := c.Query("id")

  err := dbms.First(&user, id) // Sql Injection in this line /user?id=id=1)) or 1=1--
  if err == nil {
      c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "message": "success", "result": user})

  } else {
      c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "message": "something error", "result": err})
  }
  return

}

Need to runnable with GORM's docker compose config or please provides your config.

package main

import (
  "fmt"
  "net/http"

  "./db"
  "./models"
  "github.com/gin-gonic/gin"
)

// DBMigrate for Auto Migrate
func DBMigrate() { /* Auto Migrations */
  fmt.Println("[::] Migration Databases .....")
  dbms := db.GetDatabaseConnection() /* Get connction to database */
  defer dbms.Close()
  dbms.AutoMigrate(&models.User{})
  // db.AutoMigrate(&models.Profile{}) /* Migration Models */
  fmt.Println("[::] Migration Databases Done")
}

func main() {
  DBMigrate()
  router := gin.Default()
  router.POST("/user", CreateUser)
  router.GET("/user", GetUser)
  router.Run(":8080")
}

// CreateUser function
func CreateUser(c *gin.Context) {
  var (
      user models.User
  )

  c.BindJSON(&user)
  dbms := db.GetDatabaseConnection() /*  Open connectins */
  defer dbms.Close()
  dbms.Create(&user)
  c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "message": "success", "result": user})
  return
}

// GetUser function
func GetUser(c *gin.Context) {
  var user []models.User
  dbms := db.GetDatabaseConnection() /*  Open connectins */
  defer dbms.Close()
  id := c.Query("id")

  err := dbms.First(&user, id) // Sql Injection in this line /user?id=id=1)) or 1=1--
  if err == nil {
      c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "message": "success", "result": user})

  } else {
      c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "message": "something error", "result": err})
  }
  return

}

[nevernet]

@wahyuhadi @emirb please review #2561

[deanshapira]

Can you please explain the revert that you did? There is no fix for this vulnerability issue?

[ags799]

Should this be reopened? I see there's an open pull request for resolving this (#2878).

I ask because I am getting alerts that this package has security vulnerabilities.

[wahyuhadi]

HI @ags799
this issue still exists,

[baderkha]

@wahyuhadi does this affect where , find , raw , exec or is it limited to that use case ?

I just want to know is this package escaping strings manually . Does it not use prepare statements / interpolating queries .

[baderkha]

i see the problem now .
@wahyuhadi
if you do db.First(&result,"id=?",id) you should be safe. tbh this should not be escaped manually it should just be passed to the driver as a prepare stmt

[xpunch]

@baderkha I've tested where is ok, when using ("arg = ?",arg), gorm will use prepared statement, and the sql injection will not work.

[xpunch]

If using ("arg = " + arg), there will a risk for sql injection.
Example:
db.Where("email = " + email)
email: 'test') or 1=1 -- ')

[orishoshan]

This isn't actually a vulnerability. GORM lets you pass the WHERE as a string, in which case it places the string verbatim into the query. You might say this is bad API design as it invites the user to create SQL injection vulnerabilities if they don't sanitize the input.

Keep in mind if you use the built-in parameterization (e.g. use "id = ?" and pass the ID as a parameter), there's no vulnerability. There's also no vulnerability if you pass the model struct as a parameter instead of a string. In short, this isn't a vulnerability in GORM.

Here are some examples:

SQL injection as in this issue - but the problem is in the calling code, not GORM

The only valid case to pass a string like this to GORM is when the entire WHERE condition string comes from your code, e.g. you're writing a complex WHERE condition that takes no user input.

idUserInput := "1"
var u models.User
db.Find(&u, fmt.Sprintf("id = %s", idUserInput))

Use parameters - no SQL injection as proper escaping is done by database/sql

idUserInput := "1"
var u models.User
db.Find(&u, "id = ?", idUserInput)

Use model - no SQL injection as the WHERE condition is generated by GORM, and Id is an int

idUserInput := "1"
// Another gotcha here: if Id is 0, gorm will consider it empty and there will be no WHERE condition at all.
var u models.User
idAsInt := convertToInt(idUserInput)
db.Find(&u, models.User{Id: idAsInt})

[aiacobelli2]

Not agree @orishoshan!! GORM must bring default safe to the user, and if the user needs to control the WHERE clause it should have a method like React with dangerouslysetinnerhtml!

[orishoshan]

Absolutely, this is bad API design, but it's not a vulnerability.

[baderkha]

i beleive it just needs to be communicated in the documentation to avoid doing something like that. I don't see this as a vulnerability nor bad api design per say.

There should be documentation on how to avoid sql injection / bad practices...

[julimen5ml]

Absolutely, this is bad API design, but it's not a vulnerability.

How is this not a vulnerability? I see a SQLi there.

[jinzhu]

http://v2.gorm.io/docs/security.html

[baderkha]

@jinzhu this is awesome . Thank you for your effort on providing documentation !

[wahyuhadi]

Thanks @jinzhu

[alokmenghrajani]

https://twitter.com/empijei/status/1295843334855032835 explains how the API could be made safer by requiring a constantString in places where it can potentially lead to a SQLi.