q2a / question2answer

Question2Answer is a free and open source platform for Q&A sites, running on PHP/MySQL.
http://www.question2answer.org/
GNU General Public License v3.0
1.63k stars 628 forks source link

Security issue with qa-image.php #919

Closed q2apro closed 2 years ago

q2apro commented 2 years ago

Got a hack bot trying various things on my q2a installation.

One issue I noticed from the error log:

Question2Answer MySQL query error 1292: Truncated incorrect DOUBLE value: '1521039418985748911DBMS_PIPE.RECEIVE_MESSAGE(CHR(99)||CHR(99)||CHR(99),15)' - Query: UPDATE qa_cache SET lastread=NOW() WHERE type='i_30' AND cacheid='1521039418985748911DBMS_PIPE.RECEIVE_MESSAGE(CHR(99)||CHR(99)||CHR(99),15)'

In qa-include/qa-image.php

I am pretty sure we should make sure that the incoming blobid is alphanumeric only.

$blobid = qa_get('qa_blobid');
// make sure only valid chars!!!
q2apro commented 2 years ago

Thanks for the fix / rewrite!

So we can check by ctype_digit($blobid)?

I simply thought of filtering the incoming value with preg_replace('/[^0-9]/', '', $blobid). Are there any disadvantages doing it this way? (Despite the fact that you could add other chars to the blobid in the URL and it would still display the correct image.)

pupi1985 commented 2 years ago

You don't really need a replace. Just rejecting a match should be enough. Any replace you do will make things slower, and not necessarily better (more accurate). However, even if you had done a simple match and reject the non-matching results, still it should be slower because it is a regex. I haven't tested this in detail but, most likely, the ctype_digit() function should be faster.