CrossRef data should be sanitized

Thank you for you comments @gaellafond, it's always good to have some extra eyes reviewing the code.

If you would like to submit some patches to fix the issues you mentioned, I would be happy to commit them and attribute the work to you. I couldn't help but notice that after more than 5 years in the Drupal community, you don't have any commits to your credit, so this might be an excellent opportunity...

I've converted this to a patch and am queueing for test and further review. I only did the 2 fixes mentioned in the original post, to the modules/crossref/biblio.crossref.client.php file. I did not review any of the other code.

just a quick note that automatic review also finds a lot of problems:
http://pareview.sh/pareview/httpsgitdrupalorgprojectbibliogit-7x-1x

as far as drupalSecure standards go here is what phpcs says

Name
biblio_handler_argument_many_to_one.inc
Location
file [biblio] - .../views/biblio_handler_argument_many_to_one.inc
Problem synopsis
phpcs: Possible SQL injection in db_query() via variable $this (at line 19)

Name
biblio.install

Location
file [biblio] - .../biblio.install

Problem synopsis

phpcs: Possible SQL injection in db_query() via variable implode (at line 1513)
phpcs: Possible SQL injection in db_query() via variable implode (at line 1517)
phpcs: Possible SQL injection in db_query() via variable implode (at line 1521)
phpcs: Possible SQL injection in db_query() via variable implode (at line 1524)
phpcs: Possible SQL injection in db_query() via variable implode (at line 1540)
phpcs: Possible SQL injection in db_query() via variable $build (at line 1786)
phpcs: Possible SQL injection in db_query() via variable implode (at line 1961)

!doi = check_plain() was a bit redundant, re-rolled.

@ and % both use check_plain(), % also wraps it in EM tags

Also missed that the CrossRef error text was not sanitized.

Personally, use % @ for all message variables, even if they are trust worthy, it is simpler to check / test for these issues.

Safe data in these two too, but I would also do:

-      drupal_set_message(t('HTTP error: !error when trying to contact crossref.org for XML input', array('!error' => $result->code)),'error');
+      drupal_set_message(t('HTTP error: %error when trying to contact crossref.org for XML input', array('%error' => $result->code)),'error');

-      drupal_set_message(sprintf("XML error: %s at line %d",
-      xml_error_string(xml_get_error_code($this->parser)),
-      xml_get_current_line_number($this->parser)),'error');
+      drupal_set_message('XML error: %error at line @line', array(
+        '%error' => xml_error_string(xml_get_error_code($this->parser)),
+        '@line' => xml_get_current_line_number($this->parser),
+      ), 'error');

Note that these are not overly critical (excluding discipolo's ones) unless crossRef itself get's hacked. i think we need a critical security meta issue for this module while it is in RC....

I think this is ok, no followup issue created (feel free to is some one does consider this a risk)

FILE: ...reviewsh/pareview_temp/views/biblio_handler_argument_many_to_one.inc
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
13 | WARNING | Possible SQL injection in db_query() via variable $this

False hits...

1513 | WARNING | Possible SQL injection in db_query() via variable
| | implode
1517 | WARNING | Possible SQL injection in db_query() via variable
| | implode
1521 | WARNING | Possible SQL injection in db_query() via variable
| | implode
1524 | WARNING | Possible SQL injection in db_query() via variable
| | implode
1536 | WARNING | Possible SQL injection in db_query() via variable
| | implode

  # all internally defined
  $schema = biblio_schema();
  $fieldnames = array_keys($schema['biblio_fields']['fields']);
  $field_type_fieldnames = array_keys($schema['biblio_field_type']['fields']);
  $field_type_data_fieldnames = array_keys($schema['biblio_field_type_data']['fields']);
  $xml_file = drupal_get_path('module', 'biblio') . '/field_data.xml';
  $xml = simplexml_load_file($xml_file);
  foreach ($xml->field as $field) {}

    db_query("INSERT INTO {biblio_field_type} (" . implode(", ", $field_type_fieldnames) . ")
                  VALUES (%d, %d, %d, %d, %d, %d, %d, %d, %d)", $link_data);
      db_query("INSERT INTO {biblio_field_type} (" . implode(", ", $field_type_fieldnames) . ")
                      VALUES('" . implode("', '", $values) . "')");
    $ftd = array($field['fid'], $field->default_name, $field->hint);
    db_query("INSERT INTO {biblio_field_type_data} (" . implode(", ", $field_type_data_fieldnames) . ")
                    VALUES('" . implode("', '", $ftd) . "')");
    $field_data = array($field['fid'], $field->field_name, $field->type, $field->width, $field->maxlength);
    db_query("INSERT INTO {biblio_fields} (" . implode(", ", $fieldnames) . ")
                    VALUES('" . implode("', '", $field_data) . "')");

          db_query("INSERT INTO {biblio_field_type_data} (" . implode(", ", $field_type_data_fieldnames) . ")
                        VALUES (%d, '%s', '%s')", $ftd);

Think this is a false hit too, _id_by_name() appears to use internal data only.

1744 | WARNING | Possible SQL injection in db_query() via variable
| | $build

Actual security issue if an untrusted user was allowed to set the block title...

1811 | WARNING | Possible SQL injection in db_query() via variable
| | $block

I created this patch. #2856487: Drupal 7 biblio_update_6026() is broken, but non-critical as the DB table blocks doesn't exist and can never be reached from a D7 site!

Another false hit, self defined data again in biblio_update_6031().

1877 | WARNING | Possible SQL injection in db_query() via variable
| | implode

This one adds the error message, closing off #1556516: HTTP error: 500 when trying to contact crossref.org for XML input by providing as much info as possible about a failed request. :)

Thanks!