Re: [Hw4mdl-devel] Catalyst code review - comments please
Brought to you by:
jhlinder,
trollinger
Menu
▾
▴
Re: [Hw4mdl-devel] Catalyst code review - comments please
|
From: Thomas R. <tro...@wi...> - 2008-01-16 20:44:05
|
Hi all, I started to review the code of the moodle integration and fix the different problems. All this work is done on the branche "team/ Thomas" if you want to take a look and give feedback. Here is my comments about the review: > * [CRITICAL] getXMLChoicePage.php never checks $USER or > require_login() to the course. Is this intended? WimbaMoodleSession > seems to have an MD5 checksum but this seems to only be an internal > consistency check; the request can still be faked. All AJAX > interaction must check for a session key or similar shared secret. I will check if the user is really logged in by using the function required_login. I have to verify if this function still works when php is configured to not used the cookie for the session. > * [POTENTIALLY CRITICAL] Use of $_POST, $_GET, $_REQUEST > superglobals throughout the code, without escaping or filtering. > Although lib/setup.php is requiring or emulating magic_quotes_gpc() > behaviour, serious SQL injections and cross-site scripting > vulnerabilities may result from such use. Moodle has the built-in > functions required_param() and optional_param() that should be used > instead. See http://xkcd.com/327/ > I reviewed the management of the parameters by using the functions required_param and optional_param. On each page, I check if the different parameters are like expected and do not contain some critical elements which can cause problems. > * [MODERATE to CRITICAL] The "login" to Wimba backend is > <adminusername><courseid><first_lastname><teacher/student- > suffix> ... what about users with identical first and lastnames > (e.g. two users with the name "John Smith"), will they collide, and > if so, how would it be resolved? The name " John Smith" will appear twice in the list. We can use $USER->username instead of concatenate the first name and the last name. > In LiveClassroom, liveclassroom_get_student_userid() which populates > $userid in liveclassroom_create_session() does not include any user- > unique identifier. Is this intended (i.e. are users only ever > authenticated at a course-level)? > liveclassroom_get_student_userid() return the user id of the LC user. For each course, there are 2 generic users created in the live classroom. One user will be used for creating sessions for all instructors of this course and the other will be used for creating sessions for all students of this course. > * [MODERATE] The constant LIVECLASROOM_MOODLE_PREFIX is used in > several places in the code but is not defined (it has been commented > out). Perhaps this has been replaced by getPrefix() defined in > PrefixUtil.php (?) At the least, undefined constants will echo PHP > Warnings out to the browser. > The function which used the constant LIVECLASSROOM_MOODLE_PREFIX are deprecated. I will remove them. > * [MODERATE] The code has require, require_once, include and > include_once statements that do not anchor relative paths with a > dirname(__FILE__). This can trip up command-line execution (e.g. > cron jobs), as well as pose a potential security risk. For instance > > include "../../config.php"; > > should be > > include dirname(dirname(dirname(__FILE__))) . "/config.php"; > > or perhaps > > include dirname(__FILE__) . "/../../config.php"; > Moodle usually does: require_once('../config.php'); // defines $CFG require_once($CFG->dirroot.'/course/lib.php'); We've done what most of the other modules do. > * [MODERATE] HTTPS is recommended for communicating with the > VoiceTools server. The requests from Moodle are passing the Wimba > adminusername and password as cleartext over HTTP. Even though the > WVT server has an IP whitelist, this is still risking compromise by > an IP spoofing Man-In-The-Middle attack. > This is a known problem of our API. > * [MINOR] Bad style CSS declaration in the bvoicerecorder block. > Should be moved to the canonical /blocks/bvoicerecorder/styles.php > and use the .block_bvoicerecorder class. I will move the css into a stylesheet file > Since the review, we've realised ... this module needs to define some > capabilities and then use has_capability() Can you provide me more informations? Best Thomas Rollinger Software Engineer Wimba Inc tro...@wi... On 14 janv. 08, at 01:28, Ashley Holman wrote: > Hi All, > > Catalyst have done an analysis of the hw4mdl code and have > identified several issues which may need fixing/improving. I've > attached the PDF document that they prepared outlining the problems > they found. I'm a developer at NetSpot in Australia and we've > decided to give some time to fixing those problems, so I'd > appreciate any comments people have on each of those points in the > PDF - whether the issues are real concerns or not and what should be > considered in implementing a fix, so that I can take those into > consideration before starting development. > > Also there is an additional issue that was raised after the PDF was > written: > > > Since the review, we've realised ... this module needs to define > some > > capabilities and then use has_capability() > > Thanks very much for any thoughts/comments! > > Cheers > Ashley. > > -- > Ashley Holman > Software Engineer > NetSpot Pty Ltd > 183 Melbourne Street, North Adelaide, 5006 > Ph: +618 8361 6800 Fax: +618 8361 6811 > > <wimba-moodle- > review > .pdf > > > ------------------------------------------------------------------------- > Check out the new SourceForge.net Marketplace. > It's the best place to buy or sell services for > just about anything Open Source. > http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace_______________________________________________ > Hw4mdl-devel mailing list > Hw4...@li... > https://lists.sourceforge.net/lists/listinfo/hw4mdl-devel |
