Security questions: Markup component and others for non-admins
Amir Simantov - April 16, 2009 - 00:24
| Project: | Webform |
| Version: | 6.x-2.7 |
| Component: | Code |
| Category: | support request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
Description
In my application I am letting any user to create webforms, and I have tweaked code to let each one of them to edit his/her own webforms and see only submissions made to his/her webform.
1. Is there any security risk inovles in doing so? Is there anything extra I should to take care of when letting non-admin users to add a webform?
2. In particular, as the markup component let's webform author to use php code, and in my case the author can be any registered user, should it be disabled or is it safe? I have read that the markup component replaces the label one a few years ago.
3. Are there any other webform components I should be cautious about?
Thanks!
#1
if you are allowing standard users to add php code, your site is insecure. standard users should not be allowed to enter php code ever.
#2
Letting uses create their own webforms is fine as long as you don't grant the "use PHP for additional processing" to those users. Of course like VerMisunderstood says, you definitely should not allow access to the PHP input format either.
Also, regarding "I have tweaked code....": Please do not open a support request for Webform if you have modified the code. It's no longer Webform if it's been hacked. In hacking a module you accept responsibility for it as if it were a custom module.
#3
Thanks quicksketch and VeryMissunderstood for your fast replies.
I do not give "use PHP for additional processing" permission to users of any kind, so I understand from you answer that I am safe. Markup can contain HTML though, is it still safe (code injections, cross site scripting, etc)?
@quicksketch
I VeryUndestand what you mean about tweaked modules. I agree, and it is not reasonable to open a bug for such a tweaked thing. Of course, authors and maintainers do not responsible for tweaked code...
However, my question was of category "support request", and a principal question not regarding to some specific code in the module. Of course, one can always post in the support forums, but the best players to get good advice from for webform module are.. here!
I appreciate your help and hard work Nathan and not underestimate your time. I open an issue always after trying to search first.
Thanks again,
Amir
#4
The markup component is always run through the filtering system, so as long as you don't grant users access to the "Full HTML" input format, creating a markup component is no more dangerous than a user leaving a comment. Other fields that accept markup but do not allow a selectable input format (such as the "description" for each field), are also run through the filtering system and use the default input format for the site.
#5
Thanks once again!
#6
Speaking of filtering, formats, security... The code says:
<?php
function _webform_filter_values(...) {
...
foreach ($tokens as $token => $variable) {
if (strpos($string, $token) !== FALSE) {
foreach ($variable as $key => $value) {
$find[] = $token .'['. $key .']';
// This special case for profile module dates.
if ($token == '%profile' && is_array($value) && isset($value['year'])) {
$replace[] = format_date(strtotime($value['month'] .'/'. $value['day'] .'/'. $value['year']), 'custom', 'F j, Y', '0');
}
else {
$replace[] = (string)$value;
}
}
}
}
$string = str_replace($find, $replace, $string);
// Clean up any unused tokens.
foreach (array_keys($tokens) as $token) {
$string = preg_replace('/\\'. $token .'\[\w+\]/', '', $string);
}
return $strict ? filter_xss($string) : $string;
?>
Looking at the security of that code, I will tell you that this is wrong and does not protect you unless your webforms are all protected (i.e. no random users can create a webform.) -- as of 6.x-2.7
Why is that?
The $strict flag, if false, will leave data coming from $_POST and $_GET go through as is!!! Whether $strict is ever FALSE, I do not know. The case I looked at, it was TRUE, but the caller could set it to FALSE (Someone writing another component, for instance.)
The reason why I'm posting here is because this is a real bad XSS security issue that needs to be fixed. What I suggest is to use the
filter_xss()all the time (no $strict flag) or to run it on the parameters and not on the whole string (I would prefer!) This is what got me because I wanted to use <p> tags in my descriptions and it did not work. It looked like you were properly calling the check_markup() function with the default filter which filters out the tags I do not want, but <p> is included in my good tags that I want to keep...Fix 1. which does not allow for fully formatted descriptions as per my default format:
<?php// now make sure no XSS attack can occur!
return filter_xss($string);
?>
Fix 2. which let you have a nicely formatted description but still protects you from bad external parameters:
<?phpelse {
$replace[] = filter_xss((string)$value);
}
?>
I would think that the strtotime() will fail if you have bad code. On the other hand, it could be safer to also check the parameters of the function to some extend.
Thank you.
Alexis Wilke
P.S. again this only applies to websites where any user can create a webform. If only the admin can, you're safe (as far as I can tell.) Also, the calling functions I checked would call check_markup() on top of the filter_xss() call. In other words, the result is filtered twice and was safe anyway in that specific case. But I'd lose my nice formatting!
#7
I'm not sure what you mean by "go through" in this statement. Yes the contents of $_POST/$_GET will not be filtered if $strict = FALSE. Could you provide a scenario in which this causes a problem? Often times Webform will pass in $strict = FALSE because it will do additional filtering after replacing th tokens (such as a filter_xss, check_plain, or a check_markup). For example in the case of a Markup component, _webform_filter_values() is run with $strict = FALSE, because the result is then run through check_markup().
There are currently no known exploits within Webform, but if you do find one, you should send an issue to the security team security@drupal.org, not post it on the issue queue. What you've described here isn't really a security problem as more of a continuation on a security question. As far as I'm aware, the ability to pass in $filter = FALSE is intentional and necessary for such situations as I described for markup components (though other uses certainly also exist).
#8
Note that I posted a patch here: #243322
Now I did not say that there was an exploit per se, this does not means you should not fix the code.
Why would anyone want to call that very function and get raw data from the $_GET & $_POST?! That's dangerous.
Say I create an entry where I have a variable %get[foo] and that is to be replaced by ?foo=<something>. This means something can be entered by anyone, including a hacker.
Now, I put that %get[foo] in one of my description and for that one I can change the Input format to Full HTML. I'm a dumb end user and I trust your code will prevent any XSS attacks, obviously...
Yet, now the hacker can attack your site with an XSS.
And again, I'm not saying that it is possible in the current implementation, however, the fact that this very function allows for the data to go through untouched is dangerous.
Therefore, at this time it is not directly a security issue, but it could very well be. I don't have the time to check is possible entry to the function.
Thank you.
Alexis Wilke
#9
If a user were to use %get[foo] in a markup component, then a user were to set
example.com/foo=<exploit>, then the worst that would happen is that the user would see the HTML they typed on the page, no different than if they'd modified the HTML via Firebug. Since markup components are not ever saved to the database, there's no chance that they could possibly affect the output of other uses (thus completing an XSS attack). If the %get or %post values are entered into a default value, then there is no harm since it's no different than typing "<exploit>" into a text area or textfield manually, these values are always filtered before they're shown to other users or the admin reviewing submissions.Now, if we didn't allow some way to provide what basically is a find/replace in an unfiltered manner, then it would be impossible to do the find/replace, then provide a different filtering afterwards. If you can find an ACTUAL scenario where Webform does not filter the values, I'll be happy to fix that scenario. But the ability to do a find/replace without filtering (so as to allow filtering later) is intentional and necessary and won't be changed globally, since it required for Webform to properly do filtering besides that provided by the filter_xss() function.
#10
When I receive emails from hackers they include a URL. That URL could include the <exploit>. They could even hide it from the email by using some tinyurl.org scheme (namely a redirect from an intermediate site). That means anyone can be affected by the XSS attack.
Alexis