Ineffective pcrud (and probably cstore) input sanitization

[Lebbeous Fogle-Weekley]

Probably all versions, but confirmed in master using srfsh:

{EXAMPLE OF SQL INJECTION}

Thanks to Dan Scott for noticing something fishy in his logs that made this obvious.

[Dan Scott (denials)]

BTW, after conferring briefly with Galen, I've excised the comment I made that sparked Lebbeous' investigation from the IRC logs.

[Mike Rylander (mrylander)]

Possible fix, for testing.

tl;dr: if the user-supplied value and the db column are both numbers (jsonObject->type == JSON_NUMBER, get_primitive(field) == "number") then don't quote. Otherwise, quote.

[Jeff Godin (jgodin)]

I was able to confirm on a 2.2 system using a method different from that which Lebbeous used, and by applying Mike's patch my method no longer resulted in Bad Things.

There is what appears to be a new compile-time warning which could probably be resolved with a revised patch:

oils_sql.c:2445: warning: unused variable ‘numtype’

I would consider the testing that I did "light", but it shows good results.

[Dan Scott (denials)]

Thanks for jumping on this, everyone. It's a big, bad one.

I created a formal commit out of Mike's patch. In a separate commit, I also removed the numtype variable declaration to reduce a bit of noise at compile-time, per Jeff's note, and returned some spaces to tabs (sigh) in one line of Mike's patch.

I've pushed those two commits to a branch called lp1164575 in the security repo at <email address hidden>:security/Evergreen. And I've applied the patch to our production system as of an hour ago (using the hot-fix methods documented at http://evergreen-ils.org/blog/?p=884) to get some real-world testing in.

[Dan Scott (denials)]

I do have to say, though, maybe it's the early morning hours, but it seems like accepting a non-numeric value in a function called
jsonNumberToDBString might be applying a fix at the wrong point?

searchINPredicate seems to be applying the same tests as jsonNumberToDBString before going ahead and calling jsonNumberToDBString anyway; actually, these tests are duplicated several times throughout oils_sql.c. 7,000 lines of custom C code providing database access could probably stand a thorough re-examination on a regular basis :/

In any case, I haven't triggered the same problem again, so the fix seems to work for this case. Huzzah!

[Mike Rylander (mrylander)]

Dan,

I consider this fix a backstop only, and that you're right and the call points should be evaluated closely. It should plug any holes caused by jsonNumberToDBString callers, but not all potential users of that function may be making use of it.

The problem, ISTM, stems from an assumption about for what purposes jsonNumberToDBString should be used. The function itself was relying on the caller to make sure that it was really passed a number-ish user-supplied value, and therefore was just testing that the DB field to which that value would be tied in some way was, indeed, number-ish as well. It's used like that in some cases, but (as we now know) the IN-predicate case wants to use it the other way around -- we know this is a number-ish DB field, now quote some user-supplied value if it is non-numeric. But, of course, that's not what the function does.

So, we can look at it two ways: retain the old definition and fix the call sites to check the user-supplied value, or make jsonNumberToDBString more robust (but, potentially, badly named) and have it quote anything that's either 1) not a number-ish user-supplied value or 2) is trying to be used with a non-number-ish DB field.

The path of least code is the latter, obv, but a plan to attack it from the former angle (or, fresh eyes on the internal architecture of the query building in oils_sql.c, leading to a plan first, and action later) wouldn't be a bad thing. It would be a time-costly thing, I think.

Anyway, thanks much for testing and commit-ifying that, Dan. How shall we coordinate the back-branch releases we'll need to do? And, shall we wait on those before pushing out 2.4RC? I think yes on the latter, so we don't risk widening the exposure window to those on 2.3 and before.

Also, I know we're outside the 2.1 maintenance window, but should we consider putting out a source branch and instructions on how to build and install an updated oils_sql.so?

[Dan Scott (denials)]

Thanks for the comments, Mike. I agree with you on all counts; while revisiting oils_sql.c's overall structure is probably a really good idea, time is of the essence here, for this level of vulnerability and I think a 2.1 release is warranted for something of this potential magnitude. (It may be that TPAC exposed the vulnerability in a way that wouldn't be as easily tripped across in 2.1 via the JSPAC, but...)

I would suggest for 2.1 that we take the existing release tarball, drop in the patched oils_sql.c, add an entry to the changelog / release notes, and retar it. We could bump the release numbers in the various places, too. But I would not want to go through the full checkout & build a tarball from scratch process for 2.1; that seems unwarranted and actually riskier to me. I'm willing to create the new 2.1 release tarball. (And we can prep the 2.1 release branch over in the security repo, so that we can just push the same branch into the real repo when it's release time).

[Bill Erickson (berick)]

I've pushed security/user/berick/rel_2_3_6 -- which contains a sign-off for Dan's last commit (compiler warning). I've also confirmed my injection attempts results in a quoted value in the PG logs (vs. un-quoted sans patch).

[Lebbeous Fogle-Weekley (lebbeous)]

Prepared here: security/user/senator/rel_2_2_8

[Galen Charlton (gmc)]

Fixes for 2.1 pushed to rel_2_1 and tags/rel_2_1_6

[Mike Rylander (mrylander)]

Master has this now, as well.

[Bill Erickson (berick)]

Ditto rel_2_3 and tags/rel_2_3_6

[Bill Erickson (berick)]

ditto rel_2_2 and confirmed for tags/rel_2_2_8