maths / moodle-qtype_stack

Stack question type for Moodle
GNU General Public License v3.0
142 stars 149 forks source link

Set a maximum length on inputs to prevent security issue #1263

Closed helmo-nico closed 1 month ago

helmo-nico commented 2 months ago

One of our students managed to insert the character "a" repeatedly, resulting in a weight of 2.8 MB, into an input field for a Stack-type question response. Their attempt remained open. The scheduled task that closes overdue attempts (\mod_quiz\task\update_overdue_attempts) every minute could not close this attempt. The task behaved such that it ran indefinitely on this attempt, and the next minute the scheduled task tried to process the same attempt again. The tasks accumulated, eventually leading to the server running out of resources and crashing.

The attempt was also inaccessible from the web interface. I resolved the issue by performing an UPDATE on the student's response directly in the database, in the table mdl_question_attempt_step_data, column "value". It seems appropriate to set a maximum size and validate the inputs in Stack-type questions.

We are using version 2024060300 of the qtype_stack plugin.

Here are the parameters of the input : capture_parametres_input

Here is the answer form where the student was able to insert their response, after I truncated their response so that I could access it again : capture_bug_reponse_question

aharjula commented 2 months ago

We might not want to have an author tunable maximum value (so as to not have DOSsable materials), so this might be something that each input type needs to consider separately and apply always. For example, a singular algebraic input might very well have a reasonable limit of e.g. 500 chars, but for a matrix that might need to use a significant number of chars for just the syntax of the matrix, the limit might be significantly higher or a function of the size of the matrix.

But yes, a limit needs to exist and luckily we have many ways for adding one. We can simply add logic to the inputs to invalidate the answer at that level, which would be the cleaner way to do this. But if one wants to hot-fix things quickly, then one could add length limits to the AST-container validation logic for student-sourced things. As long as the limit leads to validation error then that auto-submit should know to continue forward.

But as long as the limit is hard-coded, we need to keep it large enough so that students who manage to generate silly long answers with some CAS can still submit them, even when they were probably meant to submit properly simplified and grouped versions. But not megabytes...

helmo-nico commented 2 months ago

Perhaps the length of the expected response could be used as a basis for calculating a maximum response length that is considered reasonable?

In any case, I wouldn't be opposed to setting an absolute maximum size at the platform level through the administrative settings to give administrators the option to avoid this issue in the future, even if it risks limiting the questions they allow teachers to ask. That seems reasonable to me because the size of the data we can allow for processing will also depend on the server configuration and resources.

sangwinc commented 2 months ago

Currently many authors are using the "String" input to store JSON objects, e.g. from JSXGraph, GeoGebra and the newer Parsons block inputs.

I've been talking with @smmercuri about adding a dedicated "JSON" input type for this purpose. I think it would be sensible to be explicit that this kind of input is different from a string a user might type. It would also be reasonable to expect this to be considerably longer than an input typed.

I agree each input should have a "max input length", and this should be hard-wired into STACK.

When this breaks something it will be interesting to see why a user needs to type in such a long expression!

(Sorry this has caused you problems).

@christianp does Numbas have an input limit, and if so what have you used?

Chris

sangwinc commented 1 month ago

@aharjula, what about setting the limit here, just before attempt to parse student input?

aharjula commented 1 month ago

@sangwinc , I would rather have it in the inputs. They have a better idea about what is sensible and what is not. And 64k is not outside the realm of some JSON monstrosity coming through a string input, but for algebraic, it sure is way too large a limit.

sangwinc commented 1 month ago

OK, thanks @aharjula. I started looking at the inputs, but we would have to put this in a lot of places. This position results in much less code duplication.

Yes, I take your point about 64k for JSON. I did wonder about that. Thanks for confirming.

Leave this with me.