dingoblog / dingo

Blog engine written in Go
MIT License
284 stars 37 forks source link

Potential SQL injection vulnerability #51

Closed bentranter closed 8 years ago

bentranter commented 8 years ago

In post.go we have

func (posts *Posts) GetAllPostList(isPage bool, onlyPublished bool, orderBy string) error {
    var where string
    if isPage {
        where = `page = 1`
    } else {
        where = `page = 0`
    }
    if onlyPublished {
        where = where + `AND published`
    }
    err := meddler.QueryAll(db, posts, fmt.Sprintf(stmtGetAllPostList, where, orderBy))
    return err
}

The issue is that, since this function accepts orderBy as any string, that means a SQL statement could be passed as an argument, and it will be evaluated in the query! This would open Dingo up to SQL injection attacks. I checked through the code briefly and there are not any instances where we pass an argument to orderBy from any public-facing code (we only hard-code the value in two places), but I think for safety's sake it'd be good to change this. My suggestion is that we use a map of "safe" values for orderBy, like:

var safe = map[string]string{
    "createdAt": "created_at",
    "publishedAt": "published_at",
}

// then use it like
func isSafe(orderBy string) string {
    if val, ok := safe[orderBy]; ok {
        return val
    }
    return "some_acceptable_default_value"
}

anyway that's just a quick suggestion, I'll look into it some more to ensure that's truly a safe solution and then open a PR, I know you're busy right now @dinever