Information review of the Update module from Coder ... below are the results:

    _____________________________________________
    MODULES/UPDATE/UPDATE.MANAGER.INC
    update.manager.inc
    ============================
  1. severity: criticalreview: security_2Line 351: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_2]
    ---> drupal_set_message(theme('item_list', $error_list), 'error');
  2. severity: criticalreview: security_4Line 681: Potential problem: form_set_error() and form_error() only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_4]
    --->form_set_error($field, array_shift($archive_errors));
  3. severity: criticalreview: security_dsmLine 686: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_dsm]
    --->drupal_set_message($error, 'error');
  4. severity: normalreview: style_function_spacingLine 776: Functions should be called with no spaces between the function name and opening parentheses [style_function_spacing]
    --->throw new Exception(t('Cannot extract %file, not a valid archive.', array ('%file' => $file)));
    _________________________________________
    MODULES/UPDATE/UPDATE.TEST
    update.test
    ==========================
  1. severity: normalreview: i18n_0Line 101: The $text argument to l() should be enclosed within t() so that it is translatable. [i18n_0]
    --->$this->assertRaw(l('7.1', 'http://example.com/drupal-7-1-release'), 'Link to release appears.');
  2. severity: normalreview: i18n_0Line 116: The $text argument to l() should be enclosed within t() so that it is translatable. [i18n_0]
    --->$this->assertRaw(l('7.2', 'http://example.com/drupal-7-2-release'), 'Link to release appears.');
  3. severity: normalreview: style_comma_spacingLine 679: missing space after comma [style_comma_spacing]
    --->$this->assertText(t('Only files with the following extensions are allowed: @archive_extensions.', array('@archive_extensions' => archiver_get_extensions())),'Only valid archives can be uploaded.');

Comments

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Version: 7.36 » 7.x-dev
Priority: Critical » Normal

Thanks for reporting this, but in the future if there are potential security concerns please follow the procedure for reporting security issues - report them privately to the security team rather than in the public issue queue.

I looked through the three cases that it flagged as potential security issues, though, and I don't think any of them are a problem in practice. For the first it looks like the error messages that get printed come from update_manager_batch_project_get() and functions called therein. The expectation for all of these seems to be that the message is already a translated, sanitized string. The second two both come from update_manager_archive_verify() which is one of the functions called by the above, so the same analysis applies.

Perhaps it would be a good idea to document explicitly in hook_verify_update_archive() that it's supposed to return only sanitized strings.... But unless there's an implementation out there that already violates that, I don't see a security issue.

Several of the lower priority issues flagged by Coder look valid. I don't think it's necessary to make strings in tests translatable via t(), though.