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

Removing/Avoiding possible injection errors #939

Closed q2apro closed 1 year ago

q2apro commented 2 years ago

I got another Russian hack bot on my site, it caused the following errors by attempting to POST malicious db queries. The errors might be helpful to solve some security issues:

These two errors are hundreds of times in the error_log:

  1. PHP Notice: Trying to access array offset on value of type null in /qa-include/pages/question-view.php on line 810

  2. PHP Notice: Trying to access array offset on value of type null in /qa-include/pages/question-view.php on line 656

These errors only a few times:

From my Q2A files:

question-view.php on line 810

$showduplicate = $question['hidden'] && $commentfollow['closedbyid'] == $parentid;

question-view.php on line 656

$htmloptions['q_request'] = qa_q_request($question['postid'], $question['title']);

qa-base.php on line 729

eval('?' . '>' . $eval);

qa-base.php on line 1861

header('Location: ' . $url);

I hope someone can see the issues and maybe fix them.

pupi1985 commented 2 years ago

I searched in the dev, master and bugfix branches for the line 810 in question-view.php file. However, none of them had the same line you have. This suggests you've made changes to that file (or others). Those changes might be the cause of the issues.

Also, probably PHP 8.1 might bring some warnings or notices as well. Things work pretty well with PHP 7.x versions and the bugfix branch.

q2apro commented 2 years ago

Here it is: https://github.com/q2a/question2answer/blob/dev/qa-include/pages/question-view.php#L815

$showduplicate = $question['hidden'] && $commentfollow['closedbyid'] == $parentid;
pupi1985 commented 2 years ago

I never said I couldn't find the line. What I said is that it is not where it should be, and that might be the cause of your issues.

What are the step by step instructions to replicate this issue in any of the branches I mentioned?

q2apro commented 2 years ago

I guess this would help converting the incoming data to int would help to prevent some of the errors above.

/qa-include/ajax/click-comment.php

$commentid = (int)qa_post_text('commentid');
$questionid = (int)qa_post_text('questionid');
$parentid = (int)qa_post_text('parentid');

And also in the same file after list($comment, ...):

if (empty($comment) || empty($question) || empty($parent))
{
    echo "QA_AJAX_RESPONSE\nError\n";
    return;
}

Casting to int for incoming data is also needed for: