Open Bug 1324496 Opened 7 years ago Updated 6 years ago

MySQL full text pattern language injection in /jsonrpc.cgi summary string results in SQL error in JSON response

Categories

(bugzilla.mozilla.org :: API, defect)

Production
defect
Not set
normal

Tracking

()

People

(Reporter: claudijd, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: sec-low, wsec-errorhandling, wsec-input)

When making the following request to /jsonrpc.cgi on bugzilla.allizom.org...

POST /jsonrpc.cgi HTTP/1.1
Host: bugzilla.allizom.org
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:52.0) Gecko/20100101 Firefox/52.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: https://bugzilla.allizom.org/enter_bug.cgi
X-Requested-With: XMLHttpRequest
Content-Type: application/json; charset=UTF-8
Content-Length: 305
Cookie: Bugzilla_login=568998; Bugzilla_logincookie=QStoJL7LDiR7psoFaFimkO
Connection: close

{"version":"1.1","method":"Bug.possible_duplicates","id":1,"params":{"product":["Enterprise Information Security"],"summary":"'%20or%201%20in%20(@@version)--","limit":12,"include_fields":["id","summary","status","resolution","update_token","cc","component"],"Bugzilla_api_token":"HbK6rrfx92AKs55E3tzlj5"}}

I get the following SQL syntax error...

HTTP/1.1 200 OK
Server: Apache
X-Backend-Server: web4.stage.bugs.scl3.mozilla.com
Vary: Accept-Encoding
Content-Type: application/json; charset=UTF-8
Strict-transport-security: max-age=31536000; includeSubDomains
Date: Mon, 19 Dec 2016 17:49:19 GMT
Keep-Alive: timeout=5, max=1000
X-xss-protection: 1; mode=block
X-content-type-options: nosniff
Connection: close
X-frame-options: SAMEORIGIN

{"version":"1.1","error":{"name":"JSONRPCError","message":"DBD::mysql::db selectall_arrayref failed: syntax error, unexpected '@' [for Statement \"SELECT bugs.bug_id AS bug_id, bugs.resolution AS resolution,\n                (MATCH(bugs_fulltext.short_desc) AGAINST('\\\"20or%201%20in%20\\\"(@@version' IN BOOLEAN MODE)) AS relevance\n           FROM bugs\n                INNER JOIN bugs_fulltext ON bugs.bug_id = bugs_fulltext.bug_id\n          WHERE (MATCH(bugs_fulltext.short_desc) AGAINST('\\\"20or%201%20in%20\\\"(@@version' IN BOOLEAN MODE)) AND product_id IN (138)\n       ORDER BY relevance DESC, bug_id DESC LIMIT 17\"] at /data/www/bugzilla.allizom.org/Bugzilla/Bug.pm line 613.\n","code":100500},"id":1}

I believe this bug is similar in nature to bug 1321009.  I don't have any reason to believe it has direct security implications, but figured I'd report the issue anyways as I discovered it during security testing and I suspect it's something we could handle better.
Summary: SQL injection in /jsonrpc.cgi summary string → SQL injection in /jsonrpc.cgi summary string results in SQL error in JSON response
:dkl - could you have a peek at this one since you did the triage for bug 1321009?
Flags: needinfo?(dkl)
Here's a direct link to the code referenced in the trace:

https://github.com/bugzilla/bugzilla/blob/master/Bugzilla/Bug.pm#L613
Summary: SQL injection in /jsonrpc.cgi summary string results in SQL error in JSON response → MySQL full text pattern language injection in /jsonrpc.cgi summary string results in SQL error in JSON response
(In reply to Jonathan Claudius [:claudijd] (use NEEDINFO) from comment #1)
> :dkl - could you have a peek at this one since you did the triage for bug
> 1321009?

Yeah it is similar but slightly different in that possible_duplicates() is splitting the string on word boundaries and the resulting search is words are ['20or%201%20in%20(@@version'] and that is all. Then sql_fulltext_search() treats the left over '('  as a boolean character and adds BOOLEAN MODE. If you remove the ( it worksfine but of course may not be the search results you are looking for.

Will need to take a look at revising how we do this in general to cover these edge cases once I get caught up with other stuff.

dkl
Flags: needinfo?(dkl)
Also, I wanted to add a little more context here around recommendation considerations...

1.) Revisit the SQL query syntax/code so it's more resilient to SQL injection bugs like this triggering a SQL error condition (I believe we have the spirit of this captured in comment 3, so consider this part redundant)

2.) Consider disabling detailed SQL error reporting on BMO.  I say this not because the SQL syntax is top secret stuff (Bugzilla is obviously opensource), but the active error reporting feedback can actually help an attacker enumerate record content from the database when the injection vector is bad enough.  It tends to take a while in practice (multiple HTTP requests), but the end result (especially because the table names/columns are already known and don't need to be enumerated) is the enumeration of the data.
:dkl/:dylan - thoughts on 1 and 2 from comment 4?
Flags: needinfo?(dylan)
Flags: needinfo?(dkl)
(In reply to Jonathan Claudius [:claudijd] (use NEEDINFO) from comment #4)
> Also, I wanted to add a little more context here around recommendation
> considerations...
> 
> 1.) Revisit the SQL query syntax/code so it's more resilient to SQL
> injection bugs like this triggering a SQL error condition (I believe we have
> the spirit of this captured in comment 3, so consider this part redundant)

We're as resilient as we can be without changing to a better underlying SQL ORM, such as DBIx::Class.
Already it is impossible for unsanitized strings to be sent to mysql -- unless we blindly detaint them.
Put another way, the only way of avoiding the invalid search query language problem would be to parse the full mysql fulltext query language in perl. This is something that would be a lot of fun but not provide any tangible benefit.

Outside of the Search code, we don't generate SQL without place holders, so the scope of the attack surface is Bugzilla/Search.pm
(for the most part, anyway). 

> 2.) Consider disabling detailed SQL error reporting on BMO.  I say this not
> because the SQL syntax is top secret stuff (Bugzilla is obviously
> opensource), but the active error reporting feedback can actually help an
> attacker enumerate record content from the database when the injection
> vector is bad enough.  It tends to take a while in practice (multiple HTTP
> requests), but the end result (especially because the table names/columns
> are already known and don't need to be enumerated) is the enumeration of the
> data.

Pretty much this will be taken care of by fixing our error handling code, which is something I'm actively working on.
The problem is that we *don't* catch exceptions generated by non-bugzilla code in a reliable way. I'll be working on this in bug 1262676.
Flags: needinfo?(dylan)
Flags: needinfo?(dkl)
:dylan - thanks so much for that dose of clarity! Much appreciated!
You need to log in before you can comment on or make changes to this bug.