Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 2146839-array-argument-placeholder-naming.patch | 1 KB | david_garcia |
Comments
Comment #1
david_garcia CreditAttribution: david_garcia commentedComment #2
david_garcia CreditAttribution: david_garcia commentedComment #3
pwolanin CreditAttribution: pwolanin commentedFixed 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.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedSince 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:
There are a couple possible improvements to drupal.org that have been suggested today as a result of this:
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.
Comment #5
crystaldawn CreditAttribution: crystaldawn commentedIt 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.
Comment #6
pwolanin CreditAttribution: pwolanin commented@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.
Comment #7
crystaldawn CreditAttribution: crystaldawn commentedSeems 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? :)
Comment #8
jcisio CreditAttribution: jcisio commentedIt 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]
Comment #9
crystaldawn CreditAttribution: crystaldawn commentedIn 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.
Comment #10
jcisio CreditAttribution: jcisio commentedAh 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) :(
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedThere'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.
Comment #12
jackbravo CreditAttribution: jackbravo commentedThe 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.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedThey 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.
Comment #14
webchickActually, 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.
Comment #15
webchickOne 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.