@GICodeWarrior - thanks for pointing this out. I couldn't replicate that specific problem, and your two mock URLs look identical, but I assume both of those are just due to transcription errors. I just checked in this fix:

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cargo/+/1269701

I think it fixes this specific problem, although there might be other parameters that need validation.

Hi @Yaron_Koren, do you happen to have a public test site I could demonstrate this on for you? I prefer not to link to a random vulnerable instance.

The difference between the two URLs is one character near the end AND ('1'='1 versus AND ('2'='1 (but URL encoded).

I see your patch is casting these couple fields to integers. However, there are several other injectable fields which you did not cast.

I recommend using SelectQueryBuilder throughout, rather than relying on targeted input validation.

Ah, yes, I missed that character. Well, switching to SelectQueryBuilder makes sense, though I think the validation is useful regardless, so that the malicious text is not displayed on the page.

What are the other injectable fields?

It's time consuming to validate each possible SQLi when string concatenation is used like this. That's (in part) why I recommend using the formal SQL APIs.

Here's one more example:

All records:

Special:Drilldown/TABLENAME?DATEFIELDNAME[]=2010%09OR%091=1

No records:

Special:Drilldown/TABLENAME?DATEFIELDNAME[]=2010%09AND%091=2

Note the use of tab characters (to allow this to be parsed as a year).

Hey all - feel free to push any patches for this through gerrit. We can include them in the next supplemental release (T421273) if they get merged this quarter.

Hi @Yaron_Koren, is this still in progress?

Sorry, I let this slip. I did add in validation for that 2nd date issue, so that should no longer be a security leak either. If you know of other specific security issues with Special:Drilldown, please let me know. It would be great to change the code to use the SelectQueryBuilder, but I haven't done it yet. I'd be happy to accept a patch from anyone else that does this changeover, by the way.

Thanks for the update and patches @Yaron_Koren.

Those are the only two examples I have.

I'm not planning more research on this project right now, so if those are fixed, I'm OK with this ticket being closed.

@sbassett is there anything else you need before this ticket should be published?

In T422774#12016075, @GICodeWarrior wrote:

@sbassett is there anything else you need before this ticket should be published?

Nope. We can make the task public now, if you'd like. It will also get a CVE and be re-announced with the supplemental release T421273 in another week or two.