Page MenuHomePhabricator

Upgrading: Hardening of qsprintf()
Closed, ResolvedPublic

Description

qsprintf() now returns a PhutilQueryString object instead of a raw string. This affects construction of some queries with qsprintf(), and execution of some queries with queryfx(), queryfx_one(), queryfx_all(), LiskDAO->loadOneWhere(...) and LiskDAO->loadAllWhere(...).

This makes unsafe query construction more difficult and supports a %P (password/secret) conversion. See T6960.

This change primarily affects custom code which uses %Q. If you pass a raw string to %Q, you will now get a warning:

queryfx_all(
  $conn,
  'SELECT * FROM ... WHERE %Q',
  'id > 3');

PHLOG: 'UNSAFE: Raw string ("id > 3") passed to query ("SELECT * FROM ... WHERE %Q") for "%Q" conversion. %Q should be passed a query string.' at ...

If you see this warning in custom code: Your custom code should be updated. See the rest of this task for guidance.

If you see this warning in upstream code: First, make sure you're up to date! Whatever you're seeing may already have been fixed. If it reproduces with the most recent version, You can report any warnings you run across as bugs on Discourse.


Guide for Updating Custom Code

Instead of passing strings to %Q as above, build the query subcomponent with qsprintf():

queryfx_all(
  $conn,
  'SELECT * FROM ... WHERE %Q',
  qsprintf($conn, 'id > %d', 3));

Practically, it's unlikely you were doing any of this, since it's easier to write WHERE id > %d in the first place. However, you may have used %Q to combine subclauses with AND or OR, or to combine value subclauses (like (X, Y)) for INSERT .... In these cases, you would generally pass implode( ... ) to queryfx*(). These can now be rewritten with:

  • %LA: List of subclauses joined by AND: X, Y -> ((X) AND (Y))
  • %LO: List of subclauses joined by OR: X, Y -> ((X) OR (Y))
  • %LQ: List of subclauses joined by , (comma): X, Y -> X, Y (useful for composing INSERT INTO ... VALUES ...).
  • %LJ: List of subclauses joined by (space): X, Y -> X Y (useful for composing JOIN clauses).

For example, if you had code like this which used implode():

$where[] = qsprintf(...);
$where[] = qsprintf(...);

...

queryfx_all(
  $conn,
  'SELECT ... WHERE %Q',
  implode(' AND ', $where));

...it can now be rewritten with %LA instead:

$where[] = qsprintf(...);
$where[] = qsprintf(...);

...

queryfx_all(
  $conn,
  'SELECT ... WHERE %LA',
  $where);

Likewise, if you had code like this:

queryfx(
  $conn,
  'INSERT INTO ... (x, y) VALUES %Q',
  implode(', ', $chunk_of_values));

...it can now be rewritten with %LQ instead:

queryfx(
  $conn,
  'INSERT INTO ... (x, y) VALUES %LQ',
  $chunk_of_values);

Concurrently, there are a few other qsprintf() changes to be aware of:

  • The new %P conversion can be used to build queries with passwords or secrets. It will prevent the value from printing in logs, --trace, etc.
  • The new %R (database + table ref) conversion should generally replace %T (table). See T11908 for discussion.
  • The PhabricatorLiskDAO::chunkSQL() method has changed behavior. It no longer accepts a joiner string and now returns a list. In most cases, the correct change for this new behavior should be to replace %Q with %LQ.
  • The signatures of some PhabricatorQuery building blocks have changed. These methods generally either now accept a $conn parameter or no longer exist (because %LA, %LO, or similar provide the same behavior). Newer code is likely unaffected since direct use of these methods is less conventional in modern code than it was in the past.
  • On AphrontDatabaseConnection, executeRawQuery($raw_query) is now executeQuery(PhutilQueryString $query). It is unlikely any third-party code is affected by this change.
  • The new %LK conversion is a very niche conversion for creating and altering keys on tables. It functions like %LC but handles columname(32) properly.

In a small number of cases, a %... conversion may not exist. We currently have two (a dynamic number of clauses joined by UNION DISTINCT, and a list of fields being summed). You can "implode" these unusual queries by building the query strings step by step:

$unions = null;
foreach ($selects as $select) {
  if (!$unions) {
    $unions = $select;
    continue;
  }

  $unions = qsprintf(
    $conn,
    '%Q UNION DISTINCT %Q',
    $unions,
    $select);
}

In extreme cases, the new %Z conversion behaves like the old %Q conversion did and allows you to use arbitrary query text. We currently have two of these: one for running .sql migrations (which are arbitrary text) and one for column types in bin/storage adjust (which can be removed eventually, it's just a lot of work to hard-code a big list of valid column types).

Revisions and Commits

Restricted Differential Revision
Restricted Differential Revision
rPHU libphutil
D19882
D19811
D19800
D19788
D19787
D19786
D19783
D19782
D19781
rP Phabricator
D19872
D19869
D19837
D19820
D19814
D19812
D19801
D19790
D19789
D19785
D19784

Event Timeline

epriestley triaged this task as Low priority.Nov 7 2018, 12:19 AM
epriestley created this task.
epriestley updated the task description. (Show Details)Nov 7 2018, 12:51 AM
epriestley updated the task description. (Show Details)Nov 9 2018, 12:42 PM

I'm going to start landing this stuff now. master will start complaining about unsafe queries all over the place (although much less frequently than it was when I first added the warning). Depending on how much complaining still exists on Friday I might make the warning developer-only, but I'm currently hopeful that I can clean up most of it before the next release promotes.

hskiba added a subscriber: hskiba.Nov 14 2018, 2:08 PM
epriestley updated the task description. (Show Details)Nov 15 2018, 1:26 PM
hskiba removed a subscriber: hskiba.Nov 15 2018, 1:35 PM
epriestley added a revision: Restricted Differential Revision.Nov 17 2018, 1:12 AM
epriestley added a revision: Restricted Differential Revision.
epriestley updated the task description. (Show Details)Nov 17 2018, 1:35 AM
epriestley added a commit: Restricted Diffusion Commit.Nov 19 2018, 3:39 PM
epriestley added a commit: Restricted Diffusion Commit.Nov 20 2018, 4:46 PM
epriestley closed this task as Resolved.Dec 12 2018, 6:19 PM
epriestley claimed this task.

There are probably some stragglers that have yet to turn up, but we appear to have survived this largely unscathed.