FAPI checkboxes and radios need strengthening for XSS

I agree this would prevent a lot of bugs. Are there any other FAPI elements that are hazardous too, maybe '#title'? Anyway, it seems this could be straightforward, just an extra key like '#html_labels' => FALSE. We might want to so some benchmarks though to make sure all the extra check_plain() calls don't have an impact.

Like this?

That's the general idea, yes, but don't we then need to remove all the check_plains throughout core when checkbox #options are built up? Otherwise things will be double-escaped.

Also, do you think there's a legit use for overriding this, and adding markup in checkbox labels? I think there could be, I could easily see wanting to use em, strong, or img. There used to be #DANGEROUS_SKIP_CHECK, but I think that's been removed.

Yeah, why not use filter_xss() or filter_xss_admin()? And I think we need to be doing the fitering on '#title' => $choice, not $key.

~

I guess I misunderstood this issue to be only about '#type' => 'checkboxes'. I'll make a new patch.

That's the general idea, yes, but don't we then need to remove all the check_plains throughout core when checkbox #options are built up?

There are none AFAICT.

Also, do you think there's a legit use for overriding this, and adding markup in checkbox labels?

I guess using e.g. <em> may be legit. Or e.g. '#title' => 'Calculate CO<sub>2</sub> emissions'.

If we do allow HTML in checkbox titles, we probably also want to allow it in #options in '#type' => 'checkboxes'. But we cannot allow it in '#type' => 'select', so #options will be plain-text for select fields, but HTML for checkboxes and radios. We should document this in the FAPI reference on http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....

I suggest we always call check_plain() on #value. No <input> type allows HTML in the value attribute, so there is no reason that the called should make this check_plain() call.

http://api.drupal.org/api/function/aggregator_categorize_items/7

Here's one example of a core module that runs check_plain() on it's checkbox #options. That's the sort of thing we'd want to search for and remove. Though it looks such occurrences are less common than I guessed.

Some modules do not perform this check, but should (for example user.module generates checkboxes for roles, without checking). While we wouldn't technically consider this a vulnerability (because you'd need 'administer permissions' to exploit it), it sets a poor example for contrib.

#558928: Form element labeling is inconsistent, inflexible and bad for accessibility was just committed. This always passes #title through filter_admin_xss(). As long as this is filter_admin_xss() and not check_plain(), callers should still call check_plain() on the values in #options if the strings are plain text.

This patch adds filter_admin_xss() to #title in fieldsets as well as #description. check_plain() is used for the value attribute in checkbox and radio fields (this value is never displayed to the user). How does that sound?

We need to cast to string because '' == 0 is TRUE.

The key => value pair in #options are not handled consistently for element type select, checkboxes and radios. I didn’t know about this until I accidentally stumble across it as my application generates form element dynamically from user’s input. The inconsistency might cause developers to make wrong assumptions when creating custom modules for their own sites and thus introduce security issues unintentionally during coding.

• select
• check_plain(key): YES
• check_plain(value): YES
• checkboxes
• check_plain(key): NO
• check_plain(value): NO
• radios
• check_plain(key): YES and NO (See note)
• check_plain(value): NO
• Note: Key in "for" and "id" attributes are not sanitized even though "value" attribute is. The "id" attribute in "form-item" div wrapper is also not sanitized (not shown in test output).

I can understand why value (as in key => value) is not sanitized in checkboxes and radios as developers might want to highlight certain checkbox or radio with HTML (I hope this will be documented in forms_api_reference.html so developers are aware of this). However, I feel key must be sanitized as it could posed potential security issues for developers as not everyone will stumble across this like I did accidentally.

I hope this issue will be considered for Drupal 6.x as well, not only 7.x-dev.

Test Data
'#options' => array('"' => '<b>Injected</b>');

Output
select:
<option value="&quot;">&lt;b&gt;Injected&lt;/b&gt;</option>

checkboxes:
<label class="option" for="edit-test-""><input type="checkbox" name="test["]" id="edit-test-"" value=""" class="form-checkbox" /> <b>Injected</b></label>

radios:
<label class="option" for="edit-test-""><input type="radio" id="edit-test-"" name="test" value="&quot;" class="form-radio" /> <b>Injected</b></label>

Please don't change the version, issues are fixed in the current development branch first and then backported if necessary.

You can help though by reviewing the patch in #10 and posting feedback.

My apologies on changing the version.

Anyway, I started Drupal development not too long ago so I don't have the full understanding to give feedback on the patch. However, I'm just wondering what repercussion will it have if we just do the highlighted right before we use them.

foreach ($element['#options'] as $key => $choice) {
$key = check_plain($key);
$choice = filter_xss($choice);

My personal opinion is if developer or hacker decided to use funny key such as '"', I will rather inconvenience the developer or hacker by breaking something (not security related) rather than compromise on security. Anyway, just my two cents worth. I might be a bit simplistic in my suggestion.

The point of calling check_plain() on $key is not so much to prevent a developer from introducing XSS on purpose, but rather 1) allow the developer to use any value as key without having to do workarounds (similar to the workaround described #654796: Identify fix bugs with checkbox element that is/was necessary because 0 is not a valid value), and 2) allow the key to be based on user-supplied input.

Thanks for explaining the rational behind it.

My situation happens to fall in case 2 actually. I have made some safe assumption in how #options are handled until I decided to try something funny in my test cases out of the blue. Right now, I decided to workaround it by restricting input on the key as Drupal 6.15 does not catch " (double quote) yet and it have an effect on several attributes in label and input tags.

Radio element type is a bit tricky as some is sanitized and some is not in the generated output. So I implemented workaround similar to checkboxes.

subscribing

Postponing on #654796: Identify fix bugs with checkbox element

wow, just stumbled exactly over what turboflash outlined in #11:

Changed #type 'select' to 'radios' in a contributed module, and boom! Got ya site wrecked. :)

I don't care for which way we choose, but those three #types absolutely have to be consistent regarding their security handling of #options.

I'm actually not sure why I postponed on that other #return_value issue. AFAIK, the #default_value/#return_value handling is sufficiently detached from the #options expansion.

Also note that, IIRC, #284917: Allow FAPI select, radios, and checkboxes to specify some options as disabled wants or would need to change the behavior for #type select.

+++ includes/form.inc	3 Dec 2009 21:06:42 -0000
@@ -1834,20 +1834,22 @@ function theme_fieldset($variables) {
+  if ((string)$element['#value'] == (string)$element['#return_value']) {

Maybe this is nit-picky ... but shouldn't this be:

if ($element['#value'] === $element['#return_value'])

instead of type casting?

40 critical left. Go review some!

#19 is the topic of #654796: Identify fix bugs with checkbox element and needs to be properly changed and fixed there. Not sure why this patch alters that, but it should not.

so patches here need to be ported to D7?

Previosu discussion indicated that we might not fix this in Drupal 6 if it was going to be an API change.

I'm not sure what version the last patch (#10) was for but it seems that the points in #11 haven't been addressed. I agree 100% on being consistent and providing safe defaults.

We should be allowing HTML in the values and there are arguments for escaping the keys, so the consistent method would be check_plain()ing keys and filter_xss()ing values?

I'm going to demote this to Major. There is no new bug here. If we don't change anything, we will simply continue with D6 behavior. I'm not happy with that, but we could certainly ship this way.

In other issues, we already fixed theme functions to output attributes using drupal_attributes(), which calls check_plain() on attribute values. So, we don't have to worry about XSS from #value, #return_value, and #name. I'm not sure if we need to worry about XSS in element keys (as in $element[$key]); I couldn't find a vulnerability via simple grepping of D7 core, since mostly, all the key does is show up in #name. We can't replace with $element[check_plain($key)], since that will get check_plain()'d again when outputting #name. We can maybe replace with $element[filter_xss_admin($key)] or even a stricter $element[filter_xss($key)], but I'm not sure if we want to. Thoughts?

I think we want to at the very least filter_xss_admin() the #title, as is done in this patch, because of code like this within _form_validate() and potentially other functions:

form_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));

Unlike #10, I see no reason to filter_xss_admin() #description, or the #title of fieldsets. Maybe we want that, but seems unrelated to this issue, and I'd like to understand the use-case of doing that.

I'm not sure if this patch needs tests or inline code comments, but if we want to do this, seems like we would want at least one of those.

Separately, it is unfortunate that form_process_radios() calls check_plain() before setting #return_value, and form_process_checkboxes() does not. But if that's going to be dealt with, let's do it in #654796: Identify fix bugs with checkbox element. Chx has recently resurrected that issue with some nice tests. In any case, given what I said earlier about drupal_attributes(), it's not an XSS security issue, so no need to deal with it here.

#24: fapi-escaping-24.patch queued for re-testing.

Note: see #886152-89: All fields are hidden after the administrator newly configures a view mode to not use 'default' display

I've done some more thinking on this, and have thoughts I want to share about it, but first I want to poke our tests, and find out what fails if we do this... This patch is for bot only, not for committing.

Attached is what I think makes the most sense. Summary:

• #title and #description are assumed to hold HTML strings. Therefore, code that sets #title or #description needs to be responsible for sanitizing it. Code that prints #title or #description should assume that it is already sanitized. theme_form_element_label() is one function that doesn't follow this pattern. The patch adds a @todo for Drupal 8 about that.
• Element keys are text. They should never be treated as HTML. Our rules for text is to keep it as text as long as possible, and check_plain() at the time of printing, or assigning to a variable that is from there on out treated as HTML. Therefore, wherever element keys are output, that's where they need to be check_plain()'d. As per #24, D7 already does this correctly for #name, but this patch fixes a couple stray places in _form_validate() where element keys are incorrectly output without a check_plain().
• We don't have established rules for how to treat #options. The initial goal of this issue was to add a little more built-in protection for it. So, this patch does that in a way that doesn't contradict the rules above.
• As per #24, this patch is pretty small, because D7 already handles attributes properly, which was not the case when this issue started, and remains not the case in D6. If we later want to backport this to D6, we will need to deal with attributes too. But the OD indicates we don't necessarily want this backported to D6.

I agree with the purpose of this patch and think it would make a great addition to 7. It's a DX problem that leads to a security problem.

Re #7: I don't think aggregator_categorize_items() is an incorrect example, either before or after something like #29 lands. #options of checkboxes and radios is defined as text for keys and html for values. If you have something you know to be text (like $category->title), and you're making it the value of a #options, then converting from text to html by calling check_plain() is appropriate. On the other hand, if aggregator module wanted its category titles to support markup so that it could have formatting, then it would not need to call check_plain(). It is in this latter case, that #29 is helpful, by protecting against XSS, though as per comments in #29, such a module might still want to call a stricter filter function, depending on where the input is coming from.

#29: fapi-escaping-29.patch queued for re-testing.

How is this a bug when nothing is broken...? Security hardening is good.

I think we better merge this into #331879: Harden FAPI against $form array keys containing XSS. I'll try to go through the patch here and see if it contains something overlooked by the other patch.

@effulgentsia: i think that FAPI should be consistent towards developers: trust us, all output is filtered where possible, escaped (check_plain) where needed. Thus all output, how unlikely it is to come from user input, it may also come from translators, and where we can't protect visitors against malicious coders, we can protect against malicious translators. The use case for filtering #description is thus that it may come from translators as well. Moreover, there's also no use case against filtering #description (except perhaps performance). If a developer needs to add javascript or style, he shouldn't do so using #description. For people diving into the FAPI code itself, it gets more predictable as well.

If you agree, close this one as duplicate and let's get the other one RTBC'd before the html5 form element initiative takes off.

#331879: Harden FAPI against $form array keys containing XSS is not really related from what I can tell.

@effulgentsia's definition/standardization in #29 makes perfect sense to me:

• YOU have to sanitize array/property values for HTML.
• Array keys are sanitized by Form API, at the latest point in time, in case they are rendered.

Array values include all kind of array values; including, #title, #description, #options, etc.

So let's work on a D8-only patch for this. We can check later whether this could be backported in some way or not (but probably not).

Considering the major-ness of the other majors, I'm bumping this down.

still seems worth getting into 8?

This could use an issue summary update regarding exactly what XSS hardening is left to do here.

As pointed out above, the array values of #options already are sanitized via filter_xss_admin() in theme_form_element_label() before using them as a checkbox or radio button label. This is a documented security feature of Drupal 7 (https://www.drupal.org/node/28984), although it wasn't present in Drupal 6 and may possibly have gone missing in Drupal 8 (but there's a separate issue for that at #2560863: #options for radios and checkboxes uses SafeMarkup::checkPlain() to escape - use Html::escape() instead).

I just tested this in 7.x and noticed that #title and both keys and elements of #options are sanitized. I think that this changed at some point since this issue was created and...that's great.

I tried #type values of checkbox, checkboxes, radio, radios and none of them executed the alert nor printed the plaintext tag to render the rest of the page in plain text.

  $form['319483'] = array(
    '#type' => 'checkboxes',
    '#title' => t('<script>alert(1)</script>"><plaintext>'),
    '#options' => drupal_map_assoc(['<script>alert(1)</script>"><plaintext>', 'FOO']),
    '#default_value' => array(),
  );

I'm not sure about Drupal 8 - but it does seem appropriate to handle that in #2560863: #options for radios and checkboxes uses SafeMarkup::checkPlain() to escape - use Html::escape() instead.

I think this could be closed as a duplicate of...wherever it was fixed :)

#options for radios and checkboxes is definitely XSS admin filtered and select #options are escaped in D8. #2560863: #options for radios and checkboxes uses SafeMarkup::checkPlain() to escape - use Html::escape() instead adds complete test coverage around this behaviour. I agree that this issue is a duplicate of something :).