Harden FAPI against $form array keys containing XSS

subscribe.

Doing this is not trivial because if you change form indexes everything changes. I am more inclined to add this to write secure code page. This clearly wont happen in D7 however.

subscribe

To make this issue a bit broader. FAPI sanitizes almost all data supplied via the elements. It either uses:
- check_plain() on places where no html at all is allowed, e.g. values and attributes. (and that includes name as long as name gets rendered via drupal_attributes())
- filter_xss_admin() on places where some html is to be allowed, e.g. #title (which gets rendered as the content of a 'label' tag).

However I found (at least) 3 inconsistencies:
- Legend title (#title entry for #type = ‘fieldset’ elements): not filtered (whereas label text does go through xss_filter_admin()). According to the xhtml specification they accept both the same data between tag and closing tag: text and inline elements. Conceptually both are labels and thus should be handled the same.
- Optgroup text (multi level array #options entry for #type = ‘select’ elements): Select options can be grouped using the optgroup tag. The text for the optgroup does not get filtered, whereas if the #options array is not multi-level (not grouped), it does go through check_plain().
- #description does not get filtered. Description may contain html markup, but still, passing it through filter_xss_admin seems reasonable.

Functions concerned (all in form.inc):
- theme_fieldset() (versus theme_form_element_label())
- form_select_options()
- theme_fieldset(), theme_form_element()

Security implications are small, that's why the security team advised to publicly post these findings (as follow up to this post). IMO developers may assume consistent behaviour from FAPI and that behaviour being filtering/encoding all supplied data. Especially the keys in the select options being encoded when used as value attribute (flat array) but not encoded when used as optgroup (multi level array) is very inconsistent. Therefore I change the category to bug and the version back to 7.

Notes:
- D6 has the same problems and thus the same (minor) security implications. So I think it is better to solve it there as well.
- #field_prefix and #field_suffix are not filtered either. But I'm not sure that this should be done. I don't think so. These are helper fields that allow to render just about anything you (as developer) may want, that's their strength. I can't imagine a situation where this would come from user input.

I attached a patch (against D7 official release, so line count may be a bit off) for the above described situations.

I'm setting this to CNW just because the testbots seem to be testing over and over, and I'd like them to catch up. It's OK to queue this for testing later if it doesn't come back green. (It is in fact testing right now.)

subscribe

subscribe

Marked #932144: fieldset <legend /> uses $element['#title'] without filter_xss_admin() as a duplicate (but only covering part of this patch).

Changing to D8 as that's where it should be committed first, hopefully before the new html5 form element initiative is started.

And tagging...

Marked #319483: FAPI checkboxes and radios need strengthening for XSS as candidate for merging into this issue.

I agree with @chx in #4, and I don't think that #319483: FAPI checkboxes and radios need strengthening for XSS is related.

This issue is about form array keys that are dynamically defined via user input, not property values. Therefore, the patch in #6 is bogus.

I see. I overlooked that difference. I will broaden one of the other issues to "Harden FAPI against $form array values containing XSS" (#319483: FAPI checkboxes and radios need strengthening for XSS or #932144: fieldset <legend /> uses $element['#title'] without filter_xss_admin()) and combine my patch with the suggested changes in those issues.

Regarding this issue: I don't see a use case where either a translated string or user input would be used to construct a form array key (and expecting FAPI to check for safe and/or construct valid name, id, and class attributes). But then, don't underestimate the creativity of developers to use unchecked user input in the most unexpected places. But should/can we protect against that in this specific case?

I assume twig auto escape effectively mitigates any vectors now, but will confirm before closing

Confirming this doesn't happen in D8+ because of twig auto-escaping

This test form doesn't yield any XSS alerts, everything is nicely stripped out.

$xss = function ($count) {
      return '<script type="text/javascript">alert("boo ' . $count . '👻");</script>';
    };

    $form[$xss(1)] = [
      '#title' => 'radios',
      '#type' => 'radios',
      '#options' => [
        $xss(2) => $xss(3),
        $xss(4) => $xss(5),
      ],
      '#default_value' => $xss(2),
    ];
    $form[$xss(6)] = [
      '#title' => 'checkboxes',
      '#type' => 'checkboxes',
      '#options' => [
        $xss(7) => $xss(8),
        $xss(9) => $xss(10),
      ],
      '#default_value' => $xss(7),
    ];
    $form[$xss(11)] = [
      '#type' => 'fieldset',
      '#title' => $xss(12),
      '#description' => $xss(13),
      $xss(14) => [
        '#title' => 'foo',
        '#description' => $xss(15),
        '#type' => 'textfield',
      ],
    ];