Transferring this issue from security.drupal.org

The Security Team considered including this in SA-2014-06, but after debate about unexpected side effects decide to have it as a public issue and commit the prepared patch soon after the SA.

catch agreed that this could be committed to 7.x and 6.x in advance of an 8.x patch, as if it had been part of the SA.


Commit credit: klausi, pwolanin, tsphethean, sun

Problem/Motivation

posting an array as value of a required and maxlength limited textfield will cause the field to pass both the #required => TRUE and #maxlength validation.

Proposed resolution

Enforce the expected data type in the Form API

Remaining tasks

commit this patch for 7 and 6, make follow-up issues for other form elements

User interface changes

API changes

FAPI change to enforce correct data type of certain fields

Comments

pwolanin’s picture

Issue summary: View changes
FileSize
5.45 KB
greggles’s picture

Issue summary: View changes

I should not get commit credit - I commented, but didn't work on the patch specifically.

pwolanin’s picture

Issue summary: View changes
FileSize
3.37 KB
tstoeckler’s picture

This makes a lot of sense. I had thought about how to fix this problem in the light of the SA but did not come up with a solution. A #value_callback is a very elegant solution and is exactly the right spot. Nice!

Leaving for some others to review, but to me this is fine.

If contrib or custom modules have set their own #value_callback's this would not conflict (all though they of course would not get the added security) so the only impact I see with this would be contrib modules relying on arrays to be able to be input into textfields and textareas. Which I doubt anyone realized was possible before the SA and doubt even more anyone is actively expecting.

What is seemingly not supported in the patch is passing e.g. integers into a textarea (not a textfield) programmatically. I don't think this is a very common use-case either, so from my point of view this would be good to go.

pwolanin’s picture

@tstockler - yeah, I don't think putting an int into a textarea was much of a valid use case, compared to it being more likely for a textfield.

This patch was reviewed and rtbc by the Security team, so it shouldn't need further change before commit unless there is a bug.

tstoeckler’s picture

ahh yes, I missed that this was already marked RTBC. awesome

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.16 KB
6.48 KB
5.41 KB

I think the point about integers being possible in a programmatically-submitted textarea is a good one, actually. (Also, what about password confirm elements, where it might be more likely?) Let's not break any such code, if it exists. Here's a reroll that allows scalars in those cases, plus adds tests for that. I also fixed a few other minor things.

---

A little more background on the security team discussion:

  1. For the fundamental issue here (arrays being allowed where string are expected) we could not think of any direct security implications.
  2. The #required constraint is not actually being bypassed here (if you manage to POST an empty array it would be rejected). You can submit array('') and get it through, but that's not actually empty, so it's no worse than the point above.
  3. For the #maxlength constraint, again we couldn't think of any security implications (basically you can submit an arbitrarily large array, but the fact that it's an array is the biggest problem anyway).

We decided we can go ahead and fix this for the core text-like fields via the patches here first, to make some progress. But the same problem seems to exist for any other form element that expects a string (regardless of whether it's a text-like field or not). So as a followup, unless we want every form element in Drupal core and contrib to deal with this via a custom #value_callback we talked about some other ideas:

  1. Fix the #maxlength issue specifically via a more direct fix (make the form API validation do something sensible if #maxlength is applied to an array element).
  2. Add an "#input_type" key (or similar) to the form API that allowed you to specify whether string vs. array is expected, to handle the simplest cases (rather than repeating the same validation in each #value_callback).
  3. As a fully automatic solution, keep a running tally of what goes through drupal_attributes() as the form is being rendered (looking for 'name' attributes), then compare that to $_POST when the form is submitted. So that for example if the form had <input name="some_name[some_key]"> on it, we expect $_POST['some_name']['some_key'] to be a string (if it's present at all) but not an array.

    This might be an ugly and fragile solution due to Drupal's architecture, but technically it's the correct, comprehensive fix - it ensures that no one messes with the array vs. string structure of any part of the form as it was built server-side.

David_Rothstein’s picture

Title: Posting an array as value of a textfield satisfies #required and #maxlength constraints » Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields

The last submitted patch, 7: post-textfield-array-2380053-7-TESTS-ONLY.patch, failed testing.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Changes looks fine, though we'll need to make the same adjustments to the Drupal 6 patch.

catch’s picture

Tagging as a reminder.

David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +7.35 release notes

Committed to 7.x - thanks!

So... this goes to Drupal 8 now to forward-port there first? (I'm not sure what the prospects of getting this committed to Drupal 6 are.)

  • David_Rothstein committed 8bbc2d2 on 7.x
    Issue #2380053 by klausi, pwolanin, tsphethean, sun, David_Rothstein:...
pwolanin’s picture

Did this go into 6.x also?

pwolanin’s picture

Version: 8.0.x-dev » 6.x-dev
Status: Patch (to be ported) » Needs review
FileSize
3.73 KB
3.21 KB

Takes David's changes into account for Drupal 6.x

David_Rothstein’s picture

Updating tags, since 7.35 was a security release instead.

David_Rothstein’s picture

Decided to add a change notice here, since there are some recommended changes as a result of this that probably apply to contrib modules who define their own form element types, or individual form elements with value callbacks, etc.

https://www.drupal.org/node/2462723

RajabNatshah’s picture

Testing :)

klausi’s picture

This caused a regression with input handling for textareas, see #2502263: Drupal 7.36 regression: hidden field textarea #default_value is ignored.

pwolanin’s picture

Can we get a RTBC for 6 or no?

dawehner’s picture

  1. +++ b/includes/form.inc
    @@ -1311,6 +1322,26 @@ function form_type_select_value($form, $edit = FALSE) {
    +function form_type_textarea_value($form, $edit = FALSE) {
    +  if ($edit !== FALSE) {
    +    // This should be a string, but allow other scalars since they might be
    +    // valid input in programmatic form submissions.
    +    return is_scalar($edit) ? (string) $edit : '';
    +  }
    +}
    

    I kinda like the explicit return NULL; for example

  2. +++ b/includes/form.inc
    @@ -1326,9 +1357,12 @@ function form_type_select_value($form, $edit = FALSE) {
    +    }
    +    return str_replace(array("\r", "\n"), '', (string) $edit);
    

    Mh out of scope, but I'm curious why we need to str_replace() here

pwolanin’s picture

FileSize
3.76 KB

Reroll for conflict due to DRUPAL-SA-CORE-2015-003

pwolanin’s picture

pwolanin’s picture

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.

dsnopek’s picture

Project: Drupal core » Drupal 6 Long Term Support
Version: 6.x-dev »
Component: forms system » Code
Status: Closed (outdated) » Needs review
Issue tags: -Needs forward port to D8, -7.36 release notes, -7.36 release announcement

Since this issue is now only about Drupal 6, moving to the D6LTS queue and re-opening.

dsnopek’s picture

Title: Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields » [core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields

Put [core] into the title for easier sorting in this queue

dalin’s picture

Priority: Critical » Normal

In #7 it was mentioned:

> For the fundamental issue here (arrays being allowed where string are expected) we could not think of any direct security implications.

And I don't see anything else in this thread that suggests otherwise, so I think it makes sense to demote this.