SA-CONTRIB-2015-065 - Fix

I am one of the 1K users of this module.
I really need it and there seems to be no other drupal module that has this functionality.
I wish you success with the patch.
Thank you.

I applied the 7.x patch and everything seems fine. I did get a Hunk failed and had to apply part of it manually but I have a two other patches on the this module already so that probably had something to do with it.

patching file regcode_voucher/regcode_voucher.module
Hunk #1 FAILED at 82.

Thanks for doing this Ayesh.

Thank you for looking into this. Please see my comments below for the 7.x patch:

diff --git a/regcode.admin.php b/regcode.admin.php
index 16849bf..06fbf31 100644
--- a/regcode.admin.php
+++ b/regcode.admin.php
@@ -166,12 +166,12 @@ function regcode_admin_create() {
     '#element_validate' => array('_regcode_date_validate'),
   );
 
-  $params = array('!url' => url('admin/structure/taxonomy/regcode'));
+  $params = array('@url' => url('admin/structure/taxonomy/regcode'));

This does not need to be sanitized because it does not contain user input. It's safe already.

   $form['regcode_create']['regcode_create_tags'] = array(
     '#type'          => 'checkboxes',
     '#title'         => t("Tags"),
     '#options'       => regcode_get_vocab_terms(),
-    '#description'   => t('You may assign tags to help manage your codes (or <a href="!url">create some tags</a>).', $params),
+    '#description'   => t('You may assign tags to help manage your codes (or <a href="@url">create some tags</a>).', $params),
   );

Same here.

 
   // Bulk
diff --git a/regcode.module b/regcode.module
index 33a1126..be0007e 100644
--- a/regcode.module
+++ b/regcode.module
@@ -161,8 +161,8 @@ function regcode_form_user_register_form_alter(&$form, &$form_state) {
 
   $form['regcode'] = array(
     '#type' => 'textfield',
-    '#title' => variable_get('regcode_field_title', t('Registration Code')),
-    '#description' => variable_get('regcode_field_description', t('Please enter your registration code.')),
+    '#title' => check_plain(variable_get('regcode_field_title', t('Registration Code'))),
+    '#description' => check_plain(variable_get('regcode_field_description', t('Please enter your registration code.'))),

Can these variables be inserted by a user via the UI? If not, this shouldn't be sanitized.

     '#required' => !($code_optional || user_access('administer users')),
     '#element_validate' => array('regcode_code_element_validate'),
   );
@@ -666,8 +666,8 @@ function regcode_tag_action_form() {
   );
 
   $vid  = variable_get('regcode_vocabulary', 1);
-  $text = t('You can <a href="!url">create tags</a> through the taxonomy module.',
-    array('!url' => url('admin/structure/taxonomy/' . $vid . '/add/term')));
+  $text = t('You can <a href="@url">create tags</a> through the taxonomy module.',
+    array('@url' => url('admin/structure/taxonomy/' . $vid . '/add/term')));

That looks unnecessary as well.

   $form['regcode_taxonomy'] = array(
     '#value' => '<p>' . $text . '</p>',
   );
@@ -800,7 +800,7 @@ function regcode_tokens($type, $tokens, array $data = array(), array $options =
           break;
 
         case 'code':
-          $replacements[$original] = $regcode->code;
+          $replacements[$original] = check_plain($regcode->code);
           break;

I think that's ok.

         case 'regurl':
diff --git a/regcode_roles/regcode_roles.module b/regcode_roles/regcode_roles.module
index 09a2bdc..7ad05ba 100644
--- a/regcode_roles/regcode_roles.module
+++ b/regcode_roles/regcode_roles.module
@@ -29,7 +29,7 @@ function regcode_roles_menu() {
   );
 
   $items['admin/config/people/regcode/roles/delete/%'] = array(
-    'page callback' => 'regcode_roles_delete_rule',
+    'page callback' => 'regcode_roles_delete_rule_confirm',

Probably ok. But consider using confirm_form() like this:

-    'page callback' => 'regcode_roles_delete_rule',
-    'page arguments' => array(6),
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('regcode_roles_delete_rule_form', 6),
     'type' => MENU_CALLBACK,
     'access arguments' => array('administer registration codes'),
   );
@@ -38,17 +38,36 @@ function regcode_roles_menu() {
   return $items;
 }
 
+/**
+ * Callback: Generate form to confirm rule deletion.
+ */
+function regcode_roles_delete_rule_form($form, $form_state, $rid) {
+  $form['rule'] = array(
+    '#type' => 'value',
+    '#value' => $rid,
+  );
+
+  return confirm_form($form,
+                      t('Are you sure you want to delete rule %rule?',
+                        array('%rule' => $rid)),
+                      'admin/config/people/regcode/roles',
+                      t('This action cannot be undone.'),
+                      t('Delete'),
+                      t('Cancel'));
+
+}
 

(That's an idea only)

     'page arguments' => array(6),
     'type' => MENU_CALLBACK,
     'access arguments' => array('administer registration codes'),
@@ -38,14 +38,24 @@ function regcode_roles_menu() {
   return $items;
 }
 
+/**
+ * Page callback for role-rule deletion.
+ */
+function regcode_roles_delete_rule_confirm($rid) {
+  if (empty($_GET['token']) || !drupal_valid_token($_GET['token'], $rid)) {
+    drupal_access_denied();
+    return;
+  }

If you prefer to use tokens and not a confirmation page, you should probably return MENU_ACCESS_DENIED here.

+  regcode_roles_delete_rule($rid);
+  drupal_set_message(t('Rule was deleted (Rule #@rule)', array('@rule' => $rid)));
+  drupal_goto('admin/config/people/regcode/roles');
+}
 

Ok

 /**
  * Delete a rule
  */
 function regcode_roles_delete_rule($rid) {
   db_delete('regcode_roles')->condition('id', $rid)->execute();
-  drupal_set_message(t('Rule was deleted (Rule ##rule)', array('#rule' => $rid)));
-  drupal_goto('admin/config/people/regcode/roles');

Ok

 }
 
 
@@ -62,12 +72,14 @@ function regcode_roles_admin($form, &$form_state) {
 
   $form['regcode_roles']['new']['role_id'] = array(
     '#type' => 'select',
+    '#required' => TRUE,

Is that better? Is that security related?

     '#title' => 'Assign role',
     '#options' => user_roles(),
   );
 
   $form['regcode_roles']['new']['term_id'] = array(
     '#type' => 'select',
+    '#required' => TRUE,

Same question as above.

     '#title' => 'When a user uses a regcode with tag',
     '#options' => regcode_get_vocab_terms(),
   );
@@ -121,10 +133,15 @@ function regcode_roles_get_list_markup() {
 
   // Add actions
   foreach ($rows as &$row) {
+    $row = array_map('check_plain', $row);

That fixes an issue, but you are unnecessarily sanitizing extra things. As far as I can see only taxonomy term names could be dangerous.

     if (!empty($row['expire_date'])) {
       $row['expire_date'] = format_date($row['expire_date'], 'short');
     }
-    $row['action'] = l('Remove', 'admin/config/people/regcode/roles/delete/' . $row['id']);
+    $row['action'] = l('Remove', 'admin/config/people/regcode/roles/delete/' . $row['id'], array(
+      'query' => array(
+        'token' => drupal_get_token($row['id']),
+      ),
+    ));
   }

Ok if not using confirm_form().

 
   return theme('table', array('header' => $headings, 'rows' => $rows));
diff --git a/regcode_voucher/regcode_voucher.module b/regcode_voucher/regcode_voucher.module
index 5a78207..0582269 100644
--- a/regcode_voucher/regcode_voucher.module
+++ b/regcode_voucher/regcode_voucher.module
@@ -82,13 +82,13 @@ function regcode_voucher_profiletab_form() {
   $form = array();
 
   $form['regcode_introtext'] = array(
-    '#markup' => variable_get('regcode_voucher_introtext', ''),
+    '#markup' => filter_xss_admin(variable_get('regcode_voucher_introtext', '')),

I don't think that's necessary. Is it user provided input?

   );
 
   $form['regcode_code'] = array(
     '#type' => 'textfield',
-    '#title' => variable_get('regcode_voucher_field_title', t('Registration Code')),
-    '#description' => variable_get('regcode_voucher_field_description', t('Please enter your registration code.')),
+    '#title' => check_plain(variable_get('regcode_voucher_field_title', t('Registration Code'))),
+    '#description' => check_plain(variable_get('regcode_voucher_field_description', t('Please enter your registration code.'))),
     '#required' => FALSE,
   );

Same as above.

 
@@ -180,7 +180,7 @@ function regcode_voucher_profiletab_form_submit($form, $form_state) {
 
   $code = regcode_code_consume($form_state['values']['regcode_code'], $account->uid);
   if (is_object($code)) {
-    drupal_set_message(variable_get('regcode_voucher_message', t('Voucher code used successfully.')));
+    drupal_set_message(check_plain(variable_get('regcode_voucher_message', t('Voucher code used successfully.'))));
   }

Same.

..

That's a good start, but it's incomplete. Other submodules had similar issues that are not covered by the patches.

Please can you submit new patches?

- I agree with you about the @ vs ! modifiers. They are practically not real security issues. But all examples and best practices lead to use the @ modifier for URLs. Doc page says the same. This will however require translations of these strings to be replaced.
What do you think? Should we go for the security best practice, or ignore this for this patch ?

Personally I don't do that, but I see the point.

However, as this is supposed to fix the security vulnerability, I think we should not change this now.

- check_plain() for variable_get(). I think this is the main security issue reported. Access to the admin page is covered by administer registration codes permission (not a higher level access level). I did a complete test with all modules, and wrapped every field that was success when I enter

alert('damn')

in the input field.

Good catch then, I will have a deeper look at this later.

- Also, this module allows admins to create custom reg codes with 1-255 char length. Not sanitized when displaying. That is why I array_map'd the table row values. This happens before processing date fields (which are UNIX timestamp integers at that moment), and a check_plain() wouldn't hurt them.

At least on D7 version, this was already being sanitized. I tried to inject XSS in that page and reg codes were being escaped properly (not like the term names)

- drupal_set_message does not sanitize the text either, so it was necessary to sanitize it too.

I agree, in the case the content comes from user input.

- In the role-rule admin UI, module was functioning with clickable URLs. A form would work too, but I think it would be unnecessary since tokens are secure enough to be used in many places in core, including the theme admin UI.

Ok

I will make amends and upload 3 patches again for all the versions. Thanks again for your review.

Cool thanks

These patches look good to me. You were right about the variables being unsafe, I did not detect them in my original report.

I tested 7.x patch and I found that the UX was not great: there was not redirection after deleting entries. For some reason the calls you put to drupal_goto() were not working and I was seeing a blank page - maybe another reason to use confirmation forms?

Also, in 7.x at least OG integration became broken at some point, so it may be a good idea to fix #2278707: Intergration with OG Broken before next release.

Anyway the vulnerabilities should now be fixed, so marking it RTBC.

Many thanks

When using patch from #11, I get the following error when admin tries to create a user via:
admin/people/create

Fatal error: Cannot break/continue 1 level in .... sites/all/modules/regcode/regcode.module on line 199

@Ayesh Can you create an issue in https://www.drupal.org/project/issues/projectownership ?

@Ayesh
Many thanks