This was original reported privately as https://security.drupal.org/node/82273 but it was determined that since there was no exploitable vulnerability, this is could be made a public "security hardening" issue.

Original issue summary from @Heine:

We noticed that when code in _db_query_callback raises a warning (eg obj/array as %s argument), dblog's db_query call will wipe the args static.

When control returns from the error_handler, the remainder of the query arguments will use a corresponding empty value and the query will be executed.

See also #2380053: [core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields on how form textfields can get unexpected arrays into a submit handler. Not sure this is exploitable in core, but I'm glad that our superuser account is not uid 0.

On the private issue the rough consensus was that passing an array or object as '%s' was an incorrect use of the API, and core shouldn't babysit bad code, but that it would be better of db_query() did something more sensible, like terminating the request, rather than blindly executing a bad SQL query.

CommentFileSizeAuthor
#2 drupal6-db_query-bad-args-2705455-2.patch1.94 KBdsnopek

Comments

dsnopek created an issue. See original summary.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs review
StatusFileSize
new drupal6-db_query-bad-args-2705455-2.patch1.94 KB

Here is the latest patch from the private issue by @pwolanin.

heine’s picture

heine commented

This is an issue with form submit handlers due to #2380053: [core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields.

izmeez’s picture

izmeez commented

Does the patch in this issue complement #2380053: [core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields or is it not needed when the other is applied?

izmeez’s picture

izmeez commented

Patch in #2 applies cleanly to Drupal 6.44 LTS, along with that in the other issue, and the logic to do "something more sensible, like terminating the request, rather than blindly executing a bad SQL query." is compelling.

izmeez’s picture

izmeez commented

Both the patch in comment #2 and the patch in #2380053-23: [core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields still apply cleanly to 6.46 and do not cause any problems on php 7.2 or php 5.6

izmeez’s picture

izmeez commented
Status: Needs review » Reviewed & tested by the community

Using patch in #2 for sometime now without problems on php 7.3

izmeez’s picture

izmeez commented

Bumping this up queue for attention. Isn't it time for this to be committed?

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

FWIW: RTBC +1 will definitely run on my live sites.

The patch is clearly technically good / an improvement, that's not an issue. What we are dealing here is a question of whether we should introduce a behavior change. We have a choice between:

  • applying can in theory mean that modules which happened to work correctly in the old situation, break.
  • not applying leaves room for modules that in theory are now corrupting data without us knowing, without throwing errors.

I prefer the first. It's quite unlikely that something happens to work correctly. Also I've done some fairly extensive testing on a few sites before upgrading their hosting/PHP version, with this patch applied.

izmeez’s picture

izmeez commented

Using both the patch here in comment #2 and the patch in the other issue for quite awhile now with php 7.3 and php 7.4 on sites with a number of modules without problems.