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).

Details

Commits
D19872 / rP2814d340367c: Fix a stray qsprintf() in the Herald rules engine when recording rule…
D19869 / rPd8e2bb9f0f51: Fix some straggling qsprintf() warnings in repository import
D19837 / rP88189f723f05: Make a Feed query construction less clever/sneaky for new qsprintf() semantics
D19811 / rPHU35d0ec2dfa59: Keep the new "%P" query conversion out of the service call profiler by…
Restricted Differential Revision / Restricted Diffusion Commit
D19820 / rP4967cd6ab941: Fix some "%Q" behavior in PhortuneMerchantQuery
Restricted Differential Revision / Restricted Diffusion Commit
D19814 / rP933462b4873b: Continue cleaning up queries in the wake of changes to "%Q"
D19812 / rP49483bdb4823: Use "%P" to protect session key hashes in SessionEngine queries from DarkConsole
D19801 / rP86fd2041484f: Fix all query warnings in "arc unit --everything"
D19800 / rPHU0e6ee5937ca5: Add "%Z" (Raw Query) and "%LK" (List of Columns for Keys) to qsprintf()
D19790 / rP2f10d4adebf9: Continue making application fixes to Phabricator for changes to %Q semantics
D19789 / rP98690ee326ce: Update many Phabricator queries for new %Q query semantics
D19785 / rP64b52b9952dd: Make SELECT construction in PolicyAwareQuery safer
D19784 / rPda40f8074106: Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics
D19788 / rPHU2ec24ec0d3d2: Make "%LO" and "%LA" more readable when there is only one subclause
D19787 / rPHU86992b42b1cb: Make %LO, %LA, %LQ and %LJ more lax in what they accept (warnings instead of…
D19786 / rPHU3c1f820b6356: Add %LJ (joined with spaces) to qsprintf() for merging JOIN clauses
D19783 / rPHUbe5a87463ac5: Support %LA (AND), %LO (OR) and %LQ (comma) conversions for qsprintf() to…
D19782 / rPHUf842247de41a: Support %P (Password or Secret) in qsprintf()
D19781 / rPHUe1e8d3ba95b2: Make qsprintf() return an object, not a string, to support %P and hardening of…

Event Timeline

epriestley triaged this task as Low priority.
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.Wed, Nov 14, 2:08 PM
epriestley updated the task description. (Show Details)Thu, Nov 15, 1:26 PM
hskiba removed a subscriber: hskiba.Thu, Nov 15, 1:35 PM
epriestley added a revision: Restricted Differential Revision.Sat, Nov 17, 1:12 AM
epriestley added a revision: Restricted Differential Revision.
epriestley updated the task description. (Show Details)Sat, Nov 17, 1:35 AM
epriestley added a commit: Restricted Diffusion Commit.Mon, Nov 19, 3:39 PM
epriestley added a commit: Restricted Diffusion Commit.Tue, Nov 20, 4:46 PM
epriestley closed this task as Resolved.Wed, Dec 12, 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.