When expading array argument in query, current code uses the array key values themselves to generate the query placeholders.

This poses 3 problems:

[1] Low degree of "repetitivity" between queries, that difficults implementation of advanced query caching. In SQL Server driver we are using regex all the time to manipulate queries.

[2] User can easily crash the query if it includes no placeholder-valid characters (alphanumeric + underscore) in the keys passed in the argument. So, this will break the query:

$params[':nids'] = array(
'uid1' => 5,
'what a bad placeholder name why should we care ?¿?' => 6,
);

db_query('SELECT UID FROM USERS WHERE USERS.UID IN (:nids)', $params);

[3] Posible door open for SQL injection?

I've tried for a while with someting like this:

$params[':nids'] = array(
'ok' => 5,
'ok2) OR (1=1) OR 5 IN (5' => 6,
'ok2' => 7
);

db_query('SELECT UID FROM USERS WHERE USERS.UID IN (:nids)', $params);

But I am running SQL Server and the only why I can think of exploiting this would be using duplicate placeholder, wich MySQL swallows but SQL Server complains about. Maybe someone with MySQL can give a try and see if it can make it work.

Anyways, I don't understand why it is using the array keys passed by the user to generate the placeholders, makes no sense to me.

I've moved this into MAJOR due to points [2] and [3], feel free to downgrade.

Files: 
CommentFileSizeAuthor
#1 2146839-array-argument-placeholder-naming.patch1 KBdavid_garcia
PASSED: [[SimpleTest]]: [MySQL] 40,741 pass(es). View

Comments

david_garcia’s picture

Priority: Normal » Major
Issue summary: View changes
Status: Active » Needs review
Issue tags: +#security
FileSize
1 KB
PASSED: [[SimpleTest]]: [MySQL] 40,741 pass(es). View
david_garcia’s picture

Title: Database ExpandArguments function repetitivity » Database ExpandArguments placeholder naming issues when using array
pwolanin’s picture

Category: Feature request » Bug report
Priority: Major » Critical
Status: Needs review » Fixed

Fixed in 7.32 today.

Please report anything security-related via: https://security.drupal.org/node/add/project-issue/drupal

They should not be in the public issue queue.

David_Rothstein’s picture

Since this issue has been linked to all over the Internet today, it might be good to expand a bit on the last comment.

There are two reasons why it's important to report potential or suspected security issues privately to the security team rather than in the public issue queue:

  1. So that the problem can be investigated in private and (if warranted) a fix can be developed and released without tipping off potential attackers before the fix is available.
  2. So that the issue is guaranteed to be noticed by people who investigate and fix security issues. With thousands of issues filed against Drupal core, there is no guarantee that a particular issue which mentions possible security issues as an aside will be noticed.

There are a couple possible improvements to drupal.org that have been suggested today as a result of this:

  1. #2358373: Improve clarity of the reporting security issues warning - to make it less likely that potential security issues get reported in the public issue queue in the first place.
  2. #2358243: Create automated monitoring for new issues in the public queue - to make it more likely that if a security issue is ever reported in public, someone is able to notice it amidst the thousands of other issues and take appropriate action (unpublish it and bring it to the attention of the security team).

In practice, I'm not sure it mattered much that aspects of this particular issue were public beforehand; we have no reason to think that the vulnerability was ever actually exploited before the fix was made publicly available. The main difference is that we would have all been scrambling around a year ago to deal with this, rather than now... But obviously, we can't know that for sure, and in the general case it could be a huge problem, which is why it's important to prevent that in the future.

crystaldawn’s picture

It looks to me as though this WAS reported to the security team unless I'm not understanding what the "tag" system is suppose to be used for. 7 months ago it was tagged with "Security". Does no one on the sec team monitor the tag system? BTW, props to david, nice find. Rather than ridicule saying "should have done xyz.........." Probably better to notice he did make an attempt.

pwolanin’s picture

@crystaldawn - issues can be free-tagged with any words. Tags are not a reporting system. So, yes, it's good he tried to flag it for attention, but there is not (yet) and automatic way we would notice it.

crystaldawn’s picture

Seems like this would be a prime example of why adding a security category would be wise. I believe this has been mentioned in the past in other threads but obviously no action was ever taken to that end. Kinda strange how the biggest security breach to Drupal in quite some time would be a good candidate for such argument. What is the prognosis of the Security category suggestions after this week? :)

jcisio’s picture

It should not. When it publicly filed in an "official" security tag, hackers could monitor the issue queue and cause even more harm.

Or are you suggesting that whenever an issue is filed in the "security" category, it should be automatically immediately deleted? [/end jk]

crystaldawn’s picture

In my view, the "security" category would be viewable by the reporter as well as anyone who has the role that can view the security category. It would not be a public thing, I dont believe that it was ever suggested that the security category be a public category. They could possibly then become public once marked "closed" perhaps since that would only happen AFTER the release of the patch/fix/whatever. I know personally as a module maintainer, I would love to be able to use the issue queue to discuss security related things with my modules in private without having to use any thing different from the issue queue system. So it would make sense to me that module owners could also see the category, but that would require custom code probably. The proposed security category has been a built in feature that is possible but not utilized without need for custom development from what I've read.

jcisio’s picture

Ah ok ideally it should be like that, and I'd love it, to avoid two separate issue queues. Also about public discussion, yes I think a lot of people wanted to discuss about issues like why image style security token was introduced but not a simpler solution.

If only we didn't have performance issue (add another access control layer for less than 5% of nodes) :(

David_Rothstein’s picture

There's already a private issue queue on https://security.drupal.org which is used for this purpose and which module maintainers are invited to when there is an issue with their module.

Having that all happen directly on drupal.org instead would probably be a better experience, but a lot of work to implement (and yes, there would be performance concerns, etc).

Is there an issue somewhere where the "security category" idea is discussed? It would be better to have a discussion there than here.

jackbravo’s picture

The thing is not everyone knows about the security.drupal.org issue queue. I think ti is worth the effort to implement something on drupal.org, this is bound to happen again.

David_Rothstein’s picture

They don't need to know about security.drupal.org beforehand since they're directed there via a message when they are creating an issue on drupal.org.

But the way in which they're warned about that and directed there could certainly be improved (without having to move the whole thing to drupal.org) - see #2358373: Improve clarity of the reporting security issues warning which I linked to earlier.

webchick’s picture

Actually, the general essence of #8 is not a horrible idea at all. Filed a suggestion at #2358373-18: Improve clarity of the reporting security issues warning.

webchick’s picture

One other note: if you click on that #security tag, you'll see that this one is the only issue in the list, probably because if you type "security" into the issue tags box you get silly nonsense. I believe #2362065: Sort autocomplete of issue tags descending by usage count *in the issue metadata form* is another change that could've gotten this issue to more eyeballs quicker.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.