Insecure database queries

[joepie91]

The Gogs codebase seems to be very inconsistent in how it generates queries. There are entire chunks of code where parameterized queries are used alongside string-concatenated queries, and this has led to SQL injection vulnerabilities in the past.

Is there any particular reason why Gogs isn't using parameterized queries everywhere? Given that all major databases now support them, it's kind of shocking to me that this isn't already the case, and makes me rather concerned about deploying Gogs on my system, security-wise.

[andreynering]

You are right.

I'd say: PRs are welcome if you are willing to help.

[joepie91]

Unfortunately, I don't speak Go, otherwise I definitely would've sent in PRs.

It does seem like something that really needs to be prioritized, though - it's relatively little effort (for an experienced Go developer) to rule out a very large attack surface, as it'd be just a change of syntax for the most part.

[tboerger]

Do you have any insecure example?

[elithrar]

The link in the OP to issue.go#L805 provides a good example:

    queryStr := "SELECT COUNT(*) FROM `issue` "
    if opts.LabelID > 0 {
        queryStr += "INNER JOIN `issue_label` ON `issue`.id=`issue_label`.issue_id AND `issue_label`.label_id=" + com.ToStr(opts.LabelID)
    }

    baseCond := " WHERE issue.repo_id=" + com.ToStr(opts.RepoID) + " AND issue.is_closed=?"
    if opts.MilestoneID > 0 {
        baseCond += " AND issue.milestone_id=" + com.ToStr(opts.MilestoneID)
    }
    if opts.AssigneeID > 0 {
        baseCond += " AND assignee_id=" + com.ToStr(opts.AssigneeID)
    }
    baseCond += " AND issue.is_pull=?"

com.ToStr naively converts any type to a string, and the code here is just concatenating strings into an SQL query - e.g.

baseCond := " WHERE issue.repo_id=" + com.ToStr(opts.RepoID) + " AND issue.is_closed=?"

This (and any query like it) needs to be parameterized.

[tboerger]

We don't need to discuss if that is good code from the perspective of cleanliness or style. But securitywise it's a pretty bad/naive example as it's forced to be an integer value, casted into a string (required for string concat), so it's simply not doable to inject anything here.

[joepie91]

@tboerger The fact that there's string-concatenation at all is the problem here. Parameterized queries exist for a very good reason - they remove the need to think about the entire code path that a value travels through to assess whether it's "injectable", by treating values as being separate from the query entirely. This is especially important for two reasons:

1. Humans make mistakes. If you let a developer do the same thing over and over again, then no matter their competency, they will screw it up at some point. Case in point: the various SQLi CVEs for Gogs in the past.
2. Assumptions about what a variable contains or where it comes from, do not necessarily hold true over time. Requirements change, code changes, and at some point, somewhere, some of these assumptions are going to become invalid. And that's how security issues are introduced in previously safe code.

The whole idea of string-concatenating database queries is a historical artifact; a result of the fact that public-facing applications grew faster than databases did, and applications had to somehow use user input in queries. This has been rectified with the introduction of parameterized queries, and today, there is absolutely zero reason to still be string-concatenating queries, with all their associated security risks.

[elithrar]

@tboerger In this instance, that's correct, but you are wholly relying on the object you pass to com.ToStr to be of a type that cannot be crafted into an SQLi vector. That is the problem: the code here is brittle. If this was correctly parameterized, you could throw any type through com.ToStr and be OK.

Edit: @joepie91 sums this up better than I have.

[tboerger]

@tboerger In this instance, that's correct, but you are wholly relying on the object you pass to com.ToStr to be of a type that cannot be crafted into an SQLi vector. That is the problem: the code here is brittle. If this was correctly parameterized, you could throw any type through com.ToStr and be OK.

As i said, the style or current code is everything else than beautiful, but this specific case is not yet a security issue.

[joepie91]

This is a security issue, not a style issue. As I've explained in my previous comment.

[xinity]

@joepie91 any poc maybe to illustrate your thoughts? I'd be very interested for education propose only, as i am evaluating gogs by now

[tboerger]

This is a security issue, not a style issue. As I've explained in my previous comment.

This specific example is not an security issue as you simply can not inject anything. It's just really bad practice and not clean code.

[unknwon]

Thanks for the comments!

If anyone could explain how to make int values injectable, I would be very happy to mark this as a security issue.

Update: I do try to use parameterized queries as much as possible, but failed in this case. If someone could PR a better implemented with parameterized queries, that is the best thing I can think of. Instead of talking philosophy. 😅

[elithrar]

The point is not that the int values themselves are injectable, but that the moment anything upstream of this database code changes, you risk an SQLi.

e.g.

- queryStr += "INNER JOIN `issue_label` ON `issue`.id=`issue_label`.issue_id AND `issue_label`.label_id=" + com.ToStr(opts.LabelID)
+ queryStr += "INNER JOIN `issue_label` ON `issue`.id=`issue_label`.issue_id AND `issue_label`.label_id=" + com.ToStr(opts.LabelName) // An injection issue

or

type IssueStatsOptions struct {
    RepoID      int64
    UserID      int64
-   LabelID     int64
+   LabelID     []byte // Now a UUID type; you could pass a string ID as a query param and it could potentially traverse its way into your query
    MilestoneID int64

As @joepie91 points out:

Assumptions about what a variable contains or where it comes from, do not necessarily hold true over time.

The code here (and elsewhere, where the same assumptions are applied) is effectively safe by accident. That it requires such lengths to explain/address this is concerning unto itself.

[unknwon]

I think I may need to provide a bit of background of Gogs database design.

Everything ends with ID is absolutely int64 in Go, and is the primary id field in database tables.

UUIDs are called UUID, that's the convention.

[elithrar]

@unknwon I get that. The problem is that you are relying on never passing anything but an int64 here. If this code ever changes there is the risk of an SQLi. You are hoping that anyone committing (submitting|reviewing) code against this repo is aware of that.

[unknwon]

@unknwon I get that. The problem is that you are relying on never passing anything but an int64 here. If this code ever changes there is the risk of an SQLi. You are hoping that anyone committing (submitting|reviewing) code against this repo is aware of that.

This leads to completely different topic of this thread, you're talking code reviewing now.

I would like to clear my statement again:

If someone could PR a better implemented with parameterized queries, that is the best thing I can think of. Instead of talking philosophy.

[joepie91]

For those who'd like to fix this issue and submit a PR, I think the following command will uncover all the instances of string-concatenation in queries:

grep -rE "\+.*ToStr\(" .

(... as well as a few false positives.)

EDIT: Plus two other cases, but these may be intentional/unavoidable (although they should still be treated with care and suspicion):

./models/migrations/migrations.go:          baseSQL := "UPDATE `" + table.name + "` SET "
./models/migrations/migrations.go:                  fieldSQL += col + "_unix = "

[polyfloyd]

@joepie91 asked me te have a quick look at the code.

Go's database API has a perfectly usable way of using prepared statements and placeholders. But this seems to be thrown out of the window.

Why? Go's query placeholders are positional.

The way I see it the query concatenation seems to be a solution to building complex parameterized queries. Something that is a lot harder to do using positional placeholders. Concatting the query will move those placeholders around making it hard to keep track of the order of the parameters, so the data is added in the concatenation instead.

To solve the concatenation issue, I propose one of these solutions:

1
Stick the conditions in the query but make them toggleable by a parameter.
For example:

if opts.AssigneeID > 0 {
    baseCond += " AND assignee_id=" + com.ToStr(opts.AssigneeID)
}

Could be rewritten to:

condAssigneeId := opts.AssigneeID > 0

db.Query(`
    SELECT * FROM stuff
    WHERE
    ...
    AND (? OR assignee_id=?)
    ...
`, ..., !condAssigneeId, opts.AssigneeID)

This way, the condition is sort of ignored if condAssigneeId is set to false. Ofcourse, the query might be a little slower since it has to check more conditions, but a smart database will probably optimize that out.

2
Use a query builder. With something like GORM it
is possible to have complex queries that are build using a series of conditions
and have the benefits of placeholders.

[tboerger]

@polyfloyd it can't be written like that within Gogs. Don't think like database/sql, Gogs is using XORM.

[andreynering]

In GORM, we would write something like this:

query := dm.Where("foo =  ?", 1)
if cond == 1 {
  query = query.Where("bar = ?", 2)
}
if cond == 2 {
  query = query.Where("baz = ?", 3)
}
// ...

Gogs uses Xorm instead of Gorm. I think it would be possible, the issue may be the JOINS. I don't know how well Gorm and Xorm handle JOINS.

[tboerger]

I think XORM needs proper handling of conditions for Join. Currently there is code like this:

labelIDs := base.StringsToInt64s(strings.Split(opts.Labels, ","))
if len(labelIDs) > 0 {
  validJoin := false
  queryStr := "issue.id=issue_label.issue_id"
  for _, id := range labelIDs {
    if id == 0 {
      continue
    }
    validJoin = true
    queryStr += " AND issue_label.label_id=" + com.ToStr(id)
  }
  if validJoin {
    sess.Join("INNER", "issue_label", queryStr)
  }
}

As Join just accepts a string as a join condition it's not really possible to change that. Instead of relying on a string they should accept the conditions, than you should write something like this:

labelIDs := base.StringsToInt64s(strings.Split(opts.Labels, ","))

if len(labelIDs) > 0 {
  validJoin := false
  cond := x.Where("issue.id=issue_label.issue_id")

  for _, id := range labelIDs {
    if id == 0 {
      continue
    }

    validJoin = true
    cond.Where("issue_label.label_id = ?", id)
  }

  if validJoin {
    sess.Join("INNER", "issue_label", cond)
  }
}

[andreynering]

Gorm would accept something like this:

query := db.Where("something_id IN (?)", []int{1, 2, 3, 4, 5})

[tboerger]

Gorm would accept something like this:

query := db.Where("something_id IN (?)", []int{1, 2, 3, 4, 5})

And XORM got In, but not for join conditions.

[tboerger]

I have created #2893 as a start to improve the parameter handling. Please take a look. If you would like to see more in it feel free to contribute.

[barracks510]

The problem with using prepared statements in this program is that we are no longer database agnostic. For example, in MySQL this is a prepared statement:

INSERT INTO this(param) VALUES(?)

While, in PgSQL this is a prepared statement:

INSERT INTO this(param) VALUES($1)

There is no easy way to accommodate for both of these databases without maintaining two versions of the codebase. I think the current approach is completely acceptable (UNTIL XORM ups it's JOIN game) as SQL injections cannot be done in the current codebase, and we are targeting more than just MySQL.

[JesseWeinstein]

MySQL style prepared statements were already used in the code before this issue was brought up (e.g. https://github.com/gogits/gogs/blame/master/models/issue.go#L817 added back in Nov last year). That ship has sailed, sorry.

(I was also pointed at this thread by @joepie91 .)

(edit: As @tboerger kindly pointed out, I was "simply wrong" about this. While the style of prepared statements used by XORM does resemble that used by MySQL, it is merely a resemblance, and the example above is translated by XORM into whatever actual syntax is used by the underlying database (see postgres_dialect.go and filter.go for details on how this is done for Postgresql.))

[joepie91]

For the sake of clarity; due to the request for PRs and me not speaking Go myself, I've asked around a bit to try and find people who can help implement a fix for this. Hence the mentions above :)

@barracks510 I'd imagine it should be relatively easy to abstract away this difference. I understand that XORM might not be feature-complete, but regardless, it's never acceptable to string-concatenate queries, regardless of the scenario. It's simply the wrong API to be using.

If the difference is like you described, then surely it would just be a matter of replacing every ? with an incrementally numbered $n right before the query is executed?

[tboerger]

@JesseWeinstein you are simply wrong. We are not using MySQL prepared statements, we are just using XORM which makes it database agnostic.

[tboerger]

As far as i can see with my pull requests there are just GetRepoIssueStats and GetUserIssueStats remaining for a refactoring to get rid of the string concat issues. A first fixing PR is already merged, the next one if open so just one or two need to follow.

[lunny]

Hi, I have added join condition parameter supported on go-xorm/xorm@7d2967c

[tboerger]

@lunny really cool

[tboerger]

So #2895 should resolve all remaining issues, please review that.

[linquize]

I thought gogs fully used ORM. But here indicates NO. There are inline SQL statements.

[tboerger]

@linquize with my pull request is pure orm again

[tboerger]

All queries are parameterized now, so we can close this issue now

[joepie91]

Looks good - thanks all for the work from everybody, even upstream :)

[TreyBastian]

What's the status on this? I can fork and work through this if not completed. It seems like it is though just not closed?

[tboerger]

What's the status on this? I can fork and work through this if not completed. It seems like it is though just not closed?

#2892 (comment)
I have already fixed the issues. I just can't close this issue ;)

[bkcsoft]

Closing as fixed :)