unsanitized text (optionwidgets, number, text) [#271577]

There are possibilities for the admin to enter text for allowed values when configuring a content field that is then emitted unfiltered. Since only trusted admins can enter text for these values, this bug has been determined not to require a security release.

For the text (and number?) field type an admin can enter any code, including scripts (I confirmed via adding <script>alert('boo');</scritp> as an "allowed value" for a radio button). This presents a potential for privilege escalation or other such attacks by someone with the 'administer content types' permission. In this case, the rendering of the text is not under control of the text.module but rather falls to optionwidgets.module. This flow is confusing and obviously this user input falls though the gap.

A fix should probably be backported as well.

In general, the flow for sanitizing user input is rather confusing. Seems to be different for a node view as opposed to when the item is being viewed individually (such as part of a view). The non-sanitized version could be grabbed in a theme function like theme_text_formatter_default($element).

Comment	File	Size	Author
#20	cleanup-allowed-271577-20.patch	793 bytes	pwolanin
#19	cleanup-allowed-271577-19.patch	899 bytes	pwolanin
#17	unfiltered-allowed-271577-17.patch	13.53 KB	yched
#14	unfiltered-allowed-271577-14.patch	9.46 KB	pwolanin
#12	unfiltered-allowed-271577-12.patch	6.05 KB	pwolanin
#11	unfiltered-allowed-271577-11.patch	5.05 KB	pwolanin
#10	unfiltered-allowed-271577-10.patch	5.54 KB	pwolanin
#1	unfiltered-allowed-271577-1.patch	3.96 KB	pwolanin

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pwolanin CreditAttribution: pwolanin commented 17 June 2008 at 18:30

Status:

Active

» Needs review

File	Size
unfiltered-allowed-271577-1.patch	3.96 KB

here's a initial patch for review.

Comment #2

KarenS CreditAttribution: KarenS commented 18 June 2008 at 02:45

Status:

Needs review

» Needs work

Why wouldn't we just do this in one place -- content_allowed_values(), which is where the allowed values array is created -- instead of in several places?

Comment #3

KarenS CreditAttribution: KarenS commented 18 June 2008 at 11:02

Another question -- should we worry about unsanitized text in the widget description, too? And what about the field label? It seems like any place the user can type a value in that is displayed on the form or the view could be an issue. Other modules are adding other elements to that form where users can type in anything they want, so we need to also provide guidance on what they should do about it and exactly which elements need to be sanitized.

And should we just clean it up before storing it instead of leaving it up to the field modules to handle the output correctly? Or maybe this sanitization belongs in the theme preprocessing layer, which is where we do it for the displayed values.

Comment #4

pwolanin CreditAttribution: pwolanin commented 18 June 2008 at 12:28

Hi Karen - all good questions. per Drupal conventions, I assume text should only be filtered on output.

I'm not sure if the widget description, etc, are unfiltered. If so, that needs to be sanitized too.

Comment #5

pwolanin CreditAttribution: pwolanin commented 23 June 2008 at 13:11

Any suggestions on how to proceed?

Comment #6

KarenS CreditAttribution: KarenS commented 23 June 2008 at 15:03

I figured you (or someone) would roll a new patch :)

Was there something else I need to do?

Comment #7

pwolanin CreditAttribution: pwolanin commented 23 June 2008 at 15:31

@KarenS - I am ready to work on a revised patch, but from your comments above it sounded as though you wanted a different approach. I just wanted to see if you had more specific suggestions.

Comment #8

pwolanin CreditAttribution: pwolanin commented 23 June 2008 at 16:09

and yes, one can pass though script via the help text.

also, via the number module's prefix and suffix fields.

Comment #9

pwolanin CreditAttribution: pwolanin commented 23 June 2008 at 16:14

It's not clear to me that there is a way to catch all of these without dealing with them in the theme function (for example prefix and suffix).

Comment #10

pwolanin CreditAttribution: pwolanin commented 23 June 2008 at 18:11

Title:	unsanitized text via optionwidgets module allowed values.	» unsanitized text (optionwidgets, number, text)
Status:	Needs work	» Needs review

File	Size
unfiltered-allowed-271577-10.patch	5.54 KB

ok, I tried to sanitize as far upstream as possible in this patch. For the widget label, it seems that it needs to be done a few different places (otherwise you'll have double-escaping).

It might also make sens to add form validation to try to prevent the user from saving any tags into the label?

Comment #11

pwolanin CreditAttribution: pwolanin commented 26 June 2008 at 12:58

File	Size
unfiltered-allowed-271577-11.patch	5.05 KB

re-roll to resolve conflict.

Comment #12

pwolanin CreditAttribution: pwolanin commented 26 June 2008 at 21:19

File	Size
unfiltered-allowed-271577-12.patch	6.05 KB

somehow missed when the label is displayed during Preview/View

Comment #13

pwolanin CreditAttribution: pwolanin commented 27 June 2008 at 19:55

Status:

Needs review

» Needs work

fieldgroups also has similar problems

Comment #14

pwolanin CreditAttribution: pwolanin commented 27 June 2008 at 21:07

File	Size
unfiltered-allowed-271577-14.patch	9.46 KB

ok, here is an updated patch which also tries to clean up the group label, description, and help text.

Also, in a number of places, the user input is passed through t(). is that useful/desired? There seems to be some difference in this regard between content module and fieldgroup

compare:

'#title' => $field['widget']['label'],

with:

'#title' => $label ? t($group['label']) : '',

Comment #15

yched CreditAttribution: yched commented 27 June 2008 at 22:58

The general practice is to let field labels, help texts, etc be translated (this was often requested by users of i18n / localizer modules). To be honest, I don't know how D6 changes things in this area (I guess it doesn't).
The occurrences you spotted are probably oversights. That means we should probably have t($field['widget']['label']) take care of running check_plain().

Other than that, I'm reviewing the patch.

Comment #16

pwolanin CreditAttribution: pwolanin commented 28 June 2008 at 12:38

Ok, well - t() itself won't run check_plain unless you're passing in the label as a placeholder (in which case it can't be translated). So, I think what I did (putting the check_plain around it) might be the best option. The alternative is to put the check_plain() inside t(). That might protect the translator from XSS, but would then allow translations to include dangerous code.

Comment #17

yched CreditAttribution: yched commented 28 June 2008 at 15:40

Status:

Needs work

» Fixed

File	Size
unfiltered-allowed-271577-17.patch	13.53 KB

Doh - sure...

Committed with a few edits :
- added the missing t() calls I could spot. IIRC, we chose to have them translated on user-facing pages (node display, node forms), but not on admin pages.
- added the list of allowed tags in the help texts for 'description' and 'allowed values'.
- added to content_multiple_value_form() (form for multiple-valued fields) the check_plain() and content_filter_xss() calls that were added to content_field_form() (form for single-valued fields)
- that in turn makes that part double-encode $element['#title'], so I removed it.

@@ -466,7 +466,7 @@ function theme_content_multiple_values($
 
     $header = array(
       array(
-        'data' => t('!title: !required', array('!title' => filter_xss_admin($element['#title']), '!required' => $required)),
+        'data' => t('!title: !required', array('!title' => check_plain($element['#title']), '!required' => $required)),
         'colspan' => 2
       ),
       t('Order'),

Attached is the patch that I committed.

Thanks !

Comment #18

pwolanin CreditAttribution: pwolanin commented 28 June 2008 at 17:20

thanks!

looking at this issue (http://drupal.org/node/97640#comment-468171) and this one I just opened (http://drupal.org/node/276111)

I'm thinking maybe the sanitizing calls should be moved inside the t()?

Comment #19

pwolanin CreditAttribution: pwolanin commented 29 June 2008 at 00:58

Status:

Fixed

» Needs review

File	Size
cleanup-allowed-271577-19.patch	899 bytes

minor cleanup - it would be better to omit 'img' tags unless they are really needed, but lists might be a common need. Sorry for not thinking about this before.

Probably worth giving the tag list a second look.