0

Let's say a malicious user has no access to some path (e.g. path/element/1) based on 'access callback' as set in hook_menu() or altered in hook_menu_alter(). What happens if he has access to path/element/2?

Could he simply not change the values that are POSTed in the form submission to the server? I thought they can (even though there are form tokens). If I do several access checks to determine who can view a page, and that page contains a form that can be submitted by anyone who has access to that page, do I need to do those same access checks in the #submit or #validate handler or anything like that? Or can I rely on Drupal's form API security to keep me from having to tediously duplicate all that access code twice for pretty much every custom form!

I know Drupal has some form tokens and whatnot, but doesn't this depend on the developer who output that form (assume a contrib or custom module) setting things up right? I remember a while back I created a custom form and tried changing the POSTed values of one of the elements, and the altered value is what got picked up by Drupal! There was no rejection of the form based on altered values! So clearly I did something wrong there (what might it have been?) - and if I did, then a developer of a contrib module could do the same. So what's the best practice here?

1
  • 1
    Can you provide more context here? An example module or something? I don't think this question can be answered in the manner that you've stated it.
    – greggles
    Nov 10, 2011 at 17:41

2 Answers 2

1

It is not necessary to control if the user has access to a form in its submission handler.

Drupal protects its forms from form forgery, and verifies the values contained in the submitted form. The first control is done in drupal_validate_form(), where the form token (which is automatically assigned to every form built through the form API) is verified; if the value of $form_state['values']['form_token'] is different from the value of drupal_hmac_base64($form['#token'], session_id() . drupal_get_private_key() . drupal_get_hash_salt()), the form is rejected. The submitted values are also validated in _form_validate(), where the length of the entered value is checked against the maximum length reported for the form field, and the value entered for a "checkboxes," or a "tableselect" form field is checked against its acceptable values. In both the cases, the value used to verify the submitted values is not taken from the submitted values.

To be more sure, you should not pass sensible data, such as the ID of the node being edited as hidden field; it is preferable to set it inside the $form variable, using an array index that starts with "#" and that is unique for that form (such as "#nid"), or use a "value" form field; in that way, the value will not be passed to the HTML form rendered in the browser, and a forged form would not be able to alter that value to effectuate an operation on an entity for which the user doesn't have access. For example, this is the code used for a confirmation form that allows to delete a node:

function node_delete_confirm($form, &$form_state, $node) {
  $form['#node'] = $node;
  // Always provide entity id in the same form key as in the entity edit form.
  $form['nid'] = array(
    '#type' => 'value',
    '#value' => $node->nid,
  );
  return confirm_form($form, 
    t('Are you sure you want to delete %title?', array('%title' => $node->title)), 
    'node/' . $node->nid, 
    t('This action cannot be undone.'), 
    t('Delete'), 
    t('Cancel')
  );
}

Both $form['#node'] and $form['nid'] are not contained in the form passed to the browser.

The submission handler for that form builder is the following one:

function node_delete_confirm_submit($form, &$form_state) {
  if ($form_state['values']['confirm']) {
    $node = node_load($form_state['values']['nid']);
    node_delete($form_state['values']['nid']);
    watchdog('content', '@type: deleted %title.', array('@type' => $node->type, '%title' => $node->title));
    drupal_set_message(t('@type %title has been deleted.', array('@type' => node_type_get_name($node), '%title' => $node->title)));
  }

  $form_state['redirect'] = '<front>';
}

The value of $form passed to the form handlers is not passed to the browser, but it is retrieved in the Drupal cache from drupal_build_form(), which calls form_get_cache() when the form has been submitted. The documentation of drupal_build_form() contains the following sentence:

The form may also be retrieved from the cache if the form was built in a previous page-load. The form is then passed on for processing, validation and submission if there is proper input.

If you look at Drupal code, you will notice that none of the core modules check if the user has the permission of accessing the form in the form submission; the only code you will find is the code that checks if an error message must be output, in the case that error message should be visible to a group of users that is smaller than the group of users that has access to the form.

As side note, Drupal.org has a security team that investigates on security issues reported for Drupal core code, and any third-party modules hosted on Drupal.org; for third-party modules, they don't investigate on reports for non official releases of the modules (e.g. alpha, beta, and development snapshots). If you find a security issue in Drupal, you should report it in the project queue for Drupal core.

4
  • If I use the forms API to add a hidden field specifying a node id to be updated by this form, and a user modifies the HTTP POST to change the value of that id, the form would still submit properly. It would be up to my custom access checks to determine the user isn't allowed to edit that value. How could one enforce this in the form instead? e.g. Is there a way to make the hidden form field part of the token, so any changes to that prevent the form submission? (By the way, $form_state['values']['form_token'] is from the submitted values, but where does $form['#token'] come from?)
    – user779159
    Nov 11, 2011 at 12:07
  • You should not use a hidden field in that case. You should use $form['#nid'], for example; that value would not be passed to the browser, and there would not be the risk that a forged form would allow to alter a different node.
    – apaderno
    Nov 11, 2011 at 12:16
  • Is only 1 of $form['#node'] = $node; and $form['nid'] = array( '#type' => 'value', '#value' => $node->nid, ); needed? Or do you need both? If you just used the first one, then in your code above could $node = node_load($form_state['values']['nid']); change to $node = node_load($form['#nid']);?
    – user779159
    Nov 11, 2011 at 14:30
  • I have already submitted a patch for Drupal 8. The purpose was showing that Drupal core modules don't use an hidden form field, for such cases. To notice that a "value" form field is not passed to the form shown in the browser, differently from a hidden form field.
    – apaderno
    Nov 11, 2011 at 14:41
0

The user has total control of what he sends to your server; always. That's the nature of client-server computing.

It is the responsibility of the access callback, to determine if the user has access to the URL. The access callback does usually not look at the posted values for this, although such an access callback could be implemented.

It is the responsibility of the validation and submit handlers to not update entities that the user is not allowed to update if that is so desired. This can be achieved in two ways, depending on where the entity that is to be updated comes from.

  1. The entity can be determined from the drupal path and passed to the submit handler from the menu system. In this case the submit handler should not update the primary key.
  2. The entity can be determined by the form values. In this case the submit handler must determine whether the user is allowed to update the entity.
2
  • When the form POSTs to the same URL as the page containing the form (e.g. node/8/custommodule in revisioning module is the page URL and the form's 'action' parameter), then are no access checks required? Because the access callback that was used to determine whether the user could access the page will also be called when the form is submitted.So in point #1 in your response, where the entity is specified in the path (e.g. id of 8 in node/8/custommodule), there's no additional checks needed for this?
    – user779159
    Nov 11, 2011 at 11:59
  • Form submissions are processed by the page callback calling drupal_get_form() (or the page callback itself being drupal_get_form()). So usually the same access callback is called for displaying and submitting a form. I say usually because the user might submit the form to a different URL than you intended. It depends entirely on your module how to deal with this case. Then again, I did not yet look into the drupal_validate_form() function that kiamlaluno mentioned and it might be that Drupal itself relieves you of any further checking. I don't go this way in my on modules, though.
    – Oswald
    Nov 11, 2011 at 16:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.