orderby() should verify direction [DONE] and escape fields

What about expressions, which one can also order by? (ORDER BY RAND() for example, which is not exactly a good idea, but it works..) Introduce orderByExpression?

Also, what exactly do you mean with "escape fields / aliases"? Use our escapeField() method which doesn't actually escape but simply sanitize user data or use actual identifier escaping? (` on mysql). Because the latter often generates more problems than it solves IMHO.

And, like I already said in the escape table names/fields issue, even with fully automated escaping/sanitizing, the developer *needs* to properly check user input (for example with a whitelist of allowed values). While maybe not so problematic for an ORDER BY, in a select query that touches {user} and allowing the user to somehow select the column to read you could read out the password column instead of the username.

At the very least we should be sanitizing/whitelisting the direction, as there are exactly two values that are ever legal ("ASC" and "DESC"). That part is easy and we should do so.

Sanitizing field names for order by would be nice, I agree, but as Berdir notes is harder than it sounds. :-( Is there some minimal sanitization we could do, at least?

We could check Order By fields against those fields listed in the Select part of the query, but I'm not sure if that's being more strict than SQL itself is. We'd have to check. I don't remember the rules for when you can use aliased fields vs. not off hand, unfortunately.

I think it is perfectly fine to sort by a field that is not selected but I'm not 100% sure.

I still think the easiest solution is to limit orderBy() to fields (aka: use escapeField()) and introduce orderByExpression(), this is consistent with fields (addField()/addExpression()), conditions (condition()/where()) and having (having()/havingCondition()). That list actually shows that it is not very consistent but the pattern is the same for all of them.

I guess we could actually do what Crell suggested in #4 for groupBy() as that would also help in enforcing PostgreSQL compatibility.

Ok...

- Enforced ASC/DESC for the direction

- Escaped the field.

- The last sentence in #5 doesn't make sense since it's actually the other way round.

- Pgsql does have an special SelectQuery_pgsql::orderByRandom() implementation, but it is actually the same as the default. So that class is unecessary. Note that removing this will generate fatal errors on postgresql unless the registry uses require_once instead of require (there is an issue for that)

- Actually, according to that implementation, it looks like PostgreSQL needs (?) a field to order on, not a direct expression. This means that we can imho safely escape the field and if someone wants to use an expression to order on, they can add an expression and order on the alias and we can escape that. And on the way, we help enforcing PostgreSQL compatibility.

- I was also able to simplify some code in TableSort, since SelectQuery enforces $direction now.

Let's see if we have any orderBy() implementation (and tests to verify them ;)) that break now...

Fixed a pretty dumb typo... and also converted two comment.module queries.

#8 results in "if you order DESC you get DESC, any other string at all becomes ASC". Do we want that auto-folding, or do we want to just ignore or error on anything other than those 2?

The Postgres implementation of orderRandom() is not identical. The base implementation uses a function RAND(). The Postgres one has a function RANDOM(). We cannot remove that.

The current implementation gives everything a field to order by when doing orderRandom(). I don't recall exactly why we did that other than "because Views does it". :-) We'd need to check if it's faster/slower/more portable to make "only order by a real field" a requirement, and if so, document that in the code.

Yes, that is the same as TableSort did and ASC is the default value, so I thought that makes sense.

Uhm, yes, of course... ;)

Btw, why has the patch passed with 0 passes?

#8: orderby_escape2.patch queued for re-testing.

Ok, I was too tired when doing these comment.module changes, let's try again with this. Also re-added the pgsql specifc select class.

#13 looks good. It doesn't cover every escaping-needed eventuality, but it at least covers the orderBy(). However, if we're now mandating that you can only orderBy an already-existing (or soon to exist) field then we should document that in the method's docblock, since that's not a standard-SQL restriction.

Let's add that docblock chunk so people don't get confused and then we can get this in.

Updated the docs. Can someone confirm that the structure is ok? I tried to use the new api.module to parse the files but it takes forever....

Comments look great to me. Thanks, Berdir!

+++ includes/database/select.inc
@@ -1236,6 +1246,8 @@ class SelectQuery extends Query implements SelectQueryInterface {
+    $direction = strtoupper($direction) == 'DESC' ? 'DESC' : 'ASC';

Instead of converting unknowns to 'ASC', what about throwing an exception or something? Wouldn't that provide a better DX?

Not sure. We would at least need to re-add the check in TableSort, because if not, every user could easily produce an exception just by passing in a different GET param.

Since Tablesort previously did this logic, I'm fine with leaving it. We still need to get this in for D7 either way, as it's arguably a (small) security hole otherwise.

I'd categorize it more as an "additional security protection" or something like that :). Right now, tablesort still contains the ASC/DESC check and static queries or custom build dynamic queries in D6 were never protected anyway.

On the other side, the current comment ("The direction to sort. Legal values are "ASC" and "DESC".") could give you the false impression that the provided value is checked.

Should we perhaps just document "legal values are ASC and DESC. Anything else will be converted to ASC"?

I just want to make it explicit that the RTBC'd patch is an API change and will break code that currently uses orderBy('expression'). Question is, do we want to do that at this point to be a bit more secure (there is no security hole that *needs* to be fixed)?

I'm not so sure. :)

Note that we can still commit the ASC/DESC part of this part, that's not really an API change because every other value results in an SQL error :)

Sounds like we're still discussing this a bit. I'll let Crell decide and set this back to RTBC.

Heine pinged me about this again today. We're now a year and a bit after release so we need to tread more cautiously than in 2010.

The direction changes look fine, and I think at minimum we need docs here to explain to module developers their requirements.

The escaping field stuff though really needs a decision by a DBTNG maintainer (e.g. Crell) as I don't feel like I have enough knowledge to make a call on the impact on contrib. Worst-case, I think we should get the easy stuff (docs + direction escaping) in, then maybe punt field escaping to a separate issue.

Bumping to D8 cos it has to be fixed there first. And I'm sure it needs a re-roll by now.

Tagging.

Yes, let's separate them. Docs/direction absolutely needs to be done, and is the same in D7/8, and there's no API breakage unless you're already breaking the API.

Updating tags per http://drupal.org/node/1517250

Kind of related:
#1861670: Dynamic arguments for SelectQueryInterface::orderBy().

#15 needs a reroll, but I'm still +1 on doing this on both D7 and D8.

I'm pissed because I trusted EFQ and DBTNG to handle user provided data for me in the standard methods, but unfortunately I was wrong. Got bitten by this in RESTWS 2.x: https://drupal.org/node/2050389

Since I'm the one who implemented the whole querying stuff in restws and therefore used EFQ without looking out for possible security holes, I thought should pick up this issue.
For now I basically rerolled the patch from #829464-15: orderby() should verify direction [DONE] and escape fields. I think the new docs in this patch clarify enough how devs should use expressions and how the orderBy function is limited.
Anything else we should get into this patch?

klausi opened a new pull request for this issue.

+++ b/core/lib/Drupal/Core/Database/Query/TableSortExtender.php
@@ -47,9 +47,7 @@ public function orderByHeader(array $header) {
 
       // Sort order can only be ASC or DESC.
-      $sort = drupal_strtoupper($ts['sort']);
-      $sort = in_array($sort, array('ASC', 'DESC')) ? $sort : '';
-      $this->orderBy($field, $sort);
+      $this->orderBy($field, $ts['sort']);

Does the comment still make sense now? Maybe change it to something like "orderBy() will ensure that only ASC/DESC are accepted"? ?

Some commits were pushed to the pull request.

Thank you, GitHub Sync! :-P Your cousin Testbot can object if he wants.

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -541,6 +541,8 @@ public function addJoin($type, $table, $alias = NULL, $condition = NULL, $argume
+    $direction = strtoupper($direction) == 'DESC' ? 'DESC' : 'ASC';

Do we really want to swallow invalid input like this? What about validating then throwing an exception instead of converting?

I think swallowing/ignoring invalid input is common in the database layer, we don't throw exceptions when you try to pass a '-- DELETE FROM node for an input field of a condition value either...

I agree that swallowing the error is ok in this case. I can image quite a few use cases where the sort direction is directly supplied from user provided input (URL query parameter for example) and it would be tedious for the developer to catch exceptions for such cases. The sort direction does not shape the SQL query in a major way like what fields are queried or what are filter criterions. As there are only two valid possibilities ASC and DESC it is easy to auto-correct any invalid values without bothering the developer and without breaking the desired query outcome in a major way.

Hmm given we already do that elsewhere I guess it's fine. Feels like if not exceptions we could at least trigger error etc. - thinking about a situation where someone is doing an automated SQL injection attempt, I'd like to know about that via the logs if we can make that information available. Opened #2184361: Log when invalid input is swallowed by database layer.

Committed/pushed to 8.x, thanks! (and this might inadvertently be the first commit mention for github sync, whoops).

klausi opened a new pull request for this issue.

Looks the same to me, which is good.

42: 829464.patch queued for re-testing.

Still looks the same to me...

Sorry, I don't think I can pull the trigger on the change to $field without more discussion on the effect it will have on existing contrib/custom code. The current documentation of that parameter is pretty unclear, the syntax ->orderBy('SUBSTRING(thread, 1, (LENGTH(thread) - 1))') looks very very natural, and Drupal core itself is using it (which people often copy)...

The change to $direction is definitely fine though.

For $field, this might be going down a bad road... but is there some escaping we could do on it that is more lenient than what escapeField() does (i.e. that still lets most useful expressions through)? We don't actually care that it's a field, right... but we do care that it's not completely random SQL (i.e. allowing a semi-colon would be bad).

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

OK, so let's do baby steps and fix the obvious thing first, which we all agree on: the sort direction.

Seems simple enough.

I did test two things explicitly since I had no idea what would really happen combining RAND() with ASC when sorting, and I didn't see this in the MySQL docs.

$query = db_select('node', 'n')
  ->fields('n', array('nid'))
  ->orderBy('RAND()');

produces

SELECT n.nid AS nid
FROM 
{node} n
ORDER BY RAND() ASC

and

$query = db_select('node', 'n')
  ->fields('n', array('nid'))
  ->orderRandom();

produces

SELECT n.nid AS nid, RAND() AS random_field
FROM 
{node} n
ORDER BY random_field ASC

And both of those methods do produce random results.

Does this need to be checked on PostgreSQL?

Random test fail, back to RTBC.

@mpdonadio: this patch does not affect any Postgres compatibility since it merely sanitizes the sort direction.

Given that such DB layer problems can lead to catastrophic vulnerabilities such as SA-CORE-2014-005 I think we can raise this to critical against D7.

Committed to 7.x - thanks!

So... back to needs work to figure out what to do about escaping fields. What do we think - is there an intermediate solution, such as rejecting "fields" that have a semi-colon in them? (That doesn't stop all possible SQL injection, but should at least greatly reduce the severity of any issue that slips through, since orderBy() is only used for select queries... right?)

Here's a manual reroll of the field related code that was removed from the 7.x patch.

I agree this could break some sites, so if it's committed it should be committed early in the dev cycle of a bugfix/feature release so that people can learn about it.

The patch in #59 contained some code from #2348931: Use native MySQL statement preparation via PDO. This patch should have significantly fewer test fails.

OK, tests pass. IMO this is worth committing.

There are so many places where knowledgable people make claims that lead people to the belief "just use dbtng and 100% of the problems are gone" - if we don't provide that then we're creating unnecessary risk.

+++ b/modules/simpletest/tests/database_test.test
@@ -1986,12 +1986,17 @@ class DatabaseSelectOrderedTestCase extends DatabaseTestCase {
   function testOrderByEscaping() {
     $query = db_select('test')->orderBy('name', 'invalid direction');
     $order_bys = $query->getOrderBy();
     $this->assertEqual($order_bys['name'], 'ASC', 'Invalid order by direction is converted to ASC.');
+
+    $query = db_select('test')->orderBy('x; DROP TABLE node; --');
+    $sql = $query->__toString();
+    $lines = explode("\n", $sql);
+    $this->assertEqual(array_pop($lines), 'ORDER BY xDROPTABLEnode ASC', 'Order by field is escaped correctly.');
   }
 }

Should this also check that proper `->orderby()`, both with table alias and without, don't get munged? That is implicitly tested in other tests, but having it explicitly tested here would pinpoint a regression point, should one creep in.

I'm not sure, I just re-rolled what Klausi had done, and I think it's a backport. So I defer to him. My sense is that if we should fix that we should fix it in D8 first.

+1 for addOrderByExpression(). This is an issue when trying to alter order in a hook_views_query_alter and characters and spaces get stripped out. For example:

http://drupal.stackexchange.com/questions/189303

Or trying to use this trick:

http://stackoverflow.com/a/8174026/1483861

WHERE and HAVING get add*Expression functions, no reason to leave ORDER BY out. Also, developer experience suffers.

I was confused due to Views using differing language (and that this is a not-small change from D7 behavior) - all I think is needed here is some beefed up documentation.

I'd like to go ahead on this issue.

As David_Rothstein pointed in comment #57, we should seek an intermediate solution because many sites may already use SQL functions in orderBy(). IMHO, it's good enough to check ; and -- here.

And I agree with mpdonadio on #63. It's better to add more tests to confirm that we don't bring regression.

I made a patch. In this patch,

• orderBy() is changed instead of __toString().
• escapeField() is not used because it'll bring regression in many sites.
• An exception is thrown when a string that contains ; or -- is injected in orderBy().
• comment_new_page_count() is not changed.
• Tests for valid orderBy() cases are added.

The auto test experienced failure once and passed after requeueing. The full patch passed and test-only patch failed as expected!

I'd like someone to review the approach and code.

(If this approach is OK, I'd like to change the issue title because we're not escaping the field but throwing an exception.)

FWIW, fixing the remaining bits here would have prevented https://www.drupal.org/node/2890353

Adding this as a target for the next release.

Bumping to 7.61. This didn't make it into 7.60.

Is it fair to say this has been RTBC?

Some minor things I saw:

1. +++ b/includes/database/select.inc
   @@ -375,12 +375,15 @@ interface SelectQueryInterface extends QueryConditionInterface, QueryAlterableIn
   +   *
   +   * @throws InvalidOrderByValueException
   

   Please add a comment like "Thrown if $field contains invalid '--' or ';' characters.

2. +++ b/modules/simpletest/tests/database_test.test
   @@ -1986,12 +1986,47 @@ class DatabaseSelectOrderedTestCase extends DatabaseTestCase {
   +    $result = db_query('SELECT 1 FROM node LIMIT 1;')->execute();
   +    $this->assertTrue($result, 'Injected SQL into orderBy() were not run.');
   

   I'm confused by this, why do we test db_query() here? The assertion message also does not make sense, there was no injection into oderby().

Hi @stephencamilo can you clarify why you believe this should be marked won't fix?

I believe it should still be "Needs work".