Fix #15898 - escape tbl_storage_engine argument

[izsob]

Signed-off-by: William Desportes williamdes@wdes.fr
(cherry picked from commit ca42395)

Description

The SQL injection was discovered and fixed a long time ago. Can you include the fix in a subsequent 4.9 release? I just cherry-picked the fixing commit, and I think the original plan was to include it in the 4.9 branch, if applicable (#15898).

[kamil-tekiela]

This doesn't look like a correct fix. The ENGINE value is not a string literal and should not be quoted. The value should be validated against a list of known ENGINE types. The use of escapeString and by extension quoteString is invalid here.

[williamdes]

I am closing this one since no non security fix will enter 4.9
Why do you need it in 4.9 @izsob ?

[williamdes]

This doesn't look like a correct fix. The ENGINE value is not a string literal and should not be quoted. The value should be validated against a list of known ENGINE types. The use of escapeString and by extension quoteString is invalid here.

Can you make a PR to fix this in 5.2 ?

[izsob]

Hi @williamdes ! Thank you for the quick reply! Version 4.9 is still in use, and an SQL injection seems like a critical security vulnerability: https://nvd.nist.gov/vuln/detail/CVE-2020-22452
Can you include the fix in 4.9, or is it not affected? (Sorry, if I mistakenly referred to a wrong commit.)

[ibennetch]

On a quick look at the history of that other issue, it appears to me like we meant to target 4.9.x with the fix as well, but because it was handled outside of our usual security process I may have missed backporting it when performing the release. Good catch by @izsob.

I'll have to dig more in the commit history; I'm on mobile right now and it isn't easy to see all that information on one screen.

If that's true, we should fix this for QA_4_9, but we should also make sure the fix that's already in 5.x was correct. I agree with Kamil about the escaping here and appreciate the input.

[kamil-tekiela]

I made a PR to backport it to 5.2. I don't mind it if you want to merge it into 4.9 too, but I don't really see any reason for it. While this is SQL injection, the nature of PMA is that it's a tool that allows users to execute arbitrary SQL statements. SQL injection would only be a problem if it allowed gaining elevated privileges, which this bug doesn't.

[williamdes]

it appears to me like we meant to target 4.9.x with the fix as well

I have re-open this PR

Maybe you should try to adapt #18255 for 4.9 rather than the "wrong" fix I made ?

[williamdes]

Ref: https://security-tracker.debian.org/tracker/CVE-2020-22452

Introduced by 4d98851
All versions before 4.8.4 and after 4.1.0 are vulnerable since it was $_REQUEST: d6e04ca

/cc @carnil
/cc @ibennetch

please read #15898 (comment)