Last modified: 2014-06-30 17:24:32 UTC
As documented in this bug [1] and in these logs [2], a user with an ID of 0 was added to a course. This shouldn't happen. The bug report also shows that this can have unexpected results with security implications. I haven't yet found the route by which this happened. While permissions checking for student enrollment is not really what it should be, so far it seems that existing checks should have prevented this. [1] https://bugzilla.wikimedia.org/show_bug.cgi?id=66624 [2] https://en.wikipedia.org/w/index.php?title=Special:Log&page=Education+Program%3ASimmons+College%2FIntellectual+Freedom+-+LIS+493+%28Summer+2014%29
Created attachment 15661 [details] [PATCH] Prevent ApiAddStudents enrolling invalid users Found it! ApiAddStudents is not controlling for malformed usernames (though it does control for non-existent usernames). The issue hadn't been noticed until now since the API is only called via a JS module and most of the time other validation prevents the posting of a malformed username enrollment request. To recreate the problem: log in as an administrator, go to a course page, expand the "Add students to this course" and type in an IP address or something similar. Then click on "Add". In most circumstances the API will be called with the invalid user name, leading to the DB issues described here: https://bugzilla.wikimedia.org/show_bug.cgi?id=66624. (Enable dev tools in the browser to check that the API post was sent.) This patch should work for the versions of the extension on production and test2.
Later on more improvements can be made in the extension's handling of the ep-enroll right. Also, I don't think I can do step 5: https://wikitech.wikimedia.org/wiki/How_to_deploy_code#Security_patches
Thanks Andrew! The patch looks sane, thanks for putting that together. I'll deploy this asap.
17:36 logmsgbot: csteipp Synchronized php-1.24wmf8/extensions/EducationProgram/includes/api/ApiAddStudents.php: Bug66631 (duration: 00m 05s) 17:34 logmsgbot: csteipp Synchronized php-1.24wmf9/extensions/EducationProgram/includes/api/ApiAddStudents.php: (no message) (duration: 00m 05s)
I'm not too worried about this being public, since it would require a hostile ep admin to exploit. So feel free to put this into gerrit whenever you like, preferably before Thursday.
(In reply to Chris Steipp from comment #5) > I'm not too worried about this being public, since it would require a > hostile ep admin to exploit. So feel free to put this into gerrit whenever > you like, preferably before Thursday. OK, thanks! I'll put it in Gerrit then. Please note that it was a bit more accessible than that. In addition to users with the 'ep-addstudent' right, instructors and volunteers can use the add students feature for the course(s) they're associated with. Still, I agree, it's probably fine--especially since the fix is deployed. :)
Note also that the log entry mentioned above has been suppressed.
https://gerrit.wikimedia.org/r/#/c/140137/
It doesn't look like that got merged.
(In reply to Alex Monk from comment #9) > It doesn't look like that got merged. Ooops, yeah you're right... Can anyone review and (hopefully) merge it before tomorrow's deploy train leaves the station? Thanks!!
This is not yet resolved. Please do not mark it as such until the patch has been merged into master.
(In reply to Alex Monk from comment #11) > This is not yet resolved. Please do not mark it as such until the patch has > been merged into master. Alex: Sorry!!! I don't know what happened, something funny with the Web interface/form values remembered by my Web browser, I guess. 8p It was *not* my intention to close it again, rather just to say thank you for re-opening it. Thank you for re-re-opening it, apologies for this once again!!
Gerrit change #140137 is now merged and will go out with the wmf10 train. Marking as such. (Once wmf9 is no longer on the cluster this bug should be moved out of the Security component to the Education Programme extension.)
Thanks so much, everyone! Apologies again for the premature closing and accidental re-closing 8p
