susom/multi-signature-consent

[BUG] Multisignature module metadata SQL write is not sanitized using supported methods

Closed this issue · 4 comments

remifrazierucsf commented

Describe the bug

SQL queries called to write a metadata record in the redcap_surveys_pdf table are not sanitized using the standard methods provided in the REDCap external module framework to prevent SQL injection attack.

Expected behavior

SQL injection protection should be applied to all parameterized queries.

Code snippet

Provided because enhancements to this codebase are under development

                        // Add values to redcap_surveys_pdf_archive table
                        $survey_id = $Proj->forms[$pdf_form]['survey_id'];

                        $ip          = \System::clientIpAddress();
                        $nameDobText = $this->getModuleName();
                        $versionText = $typeText = "";
                        $sql         = "replace into redcap_surveys_pdf_archive (doc_id, record, event_id, survey_id, instance, identifier, version, type, ip) values
                                ($edoc_id, '" . db_escape($record) . "', '" . db_escape($event_id) . "', '" . db_escape($survey_id) . "', '" . db_escape($repeat_instance) . "',
                                " . checkNull($nameDobText) . ", " . checkNull($versionText) . ", " . checkNull($typeText) . ", " . checkNull($ip) . ")";
                        $q           = db_query($sql);
                        $this->emDebug($sql, $q);

Additional context

From REDCap Framework v4 release notes:

Parameterized Queries - Framework Version 4 requires that all query() method calls must include an additional $parameters argument. All queries should be refactored so that all dynamic values are passed as parameters. Please take a moment to read the Query Documentation page to get a feel for parameterized queries in general, then return here for the following specifics on transitioning modules to use them:

  • As of REDCap version v#.#.# (TDB in early 2020), the query() method supports (but does not require) the $parameters argument. This allows module queries to be updated incrementally over time, prior to updating to Framework Version 4.
  • Parameters are automatically escaped, which means manual escaping methods like db_escape() and db_real_escape_string() should be removed to avoid double escaping.
  • Updating many queries will be as simple as replacing all variables in the SQL with question marks, then specifying those variables in the $parameters argument instead. Cases that require additional handling are included below.
  • The db_affected_rows() method will no longer work. See the Query Documentation page for an alternative.
  • Methods that begin with mysqli_... and operate on the MySQL result object will no longer work. Please use the equivalent db_... methods instead.
  • Numeric column values will return as the int type in PHP where they previously returned as string. This can cause problems for any type sensitive operations like triple equals checking. The simplest solution to prevent potential issues without refactoring is to cast the numeric columns in either SQL or PHP.
    • In PHP you can cast all integer columns to strings manually, or by using the following utility method on each fetched row:
      • $row = $module->convertIntsToStrings($row);
    • In SQL you can cast values individually. For example:
      • Before: select project_id
      • After:   select cast(project_id as char) as project_id.
  • Queries using appended SQL strings can still use parameters. For example:
    • Before: $module->query("...where value like '{$value}%'");
    • After:   $module->query("...where value like ?)", [$value . '%']");
  • Please don't simply pass in an empty or partial parameter array while still manually appending parameters to your query string. Using prepared statements with parameterized queries is important for security as described by OWASP and security experts spanning many other development communities.
  • This may also be a good time replace any old db_query() calls as well. Manually calling db_query() is deprecated in modules, and may be disallowed in the future.
123andy commented

I copied the code from Survey.php ~ line 3221 in the source code - which exists today, I wouldn't necessarily call this a 'bug' in so far as you would call all of REDCap's code a bug, but that is a conversation for another day...

I'm happy to rewrite this ( 06f4e56 ) or you're also more than welcome to make a pull request in the future...

-Andy

remifrazierucsf commented
remifrazierucsf commented

Per the sibling issue on Biocatalyst - this was not submitted to ask you to do the work, but as a placeholder for a PR when I realized today that v4 of the framework has a built-in way to sanitize.

remifrazierucsf commented

I wouldn't necessarily call this a 'bug' in so far as you would call all of REDCap's code a bug, but that is a conversation for another day...

This is an exact summary of my opinion on the matter as well, by the way.