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-07-28 22:09:49
|
Hi Ashley, All the problems reported by Catalyst were fixed in the current trunk of the Wimba Moodle integration. All these changes were not part of 3.1.3 because we have estimated that the risks to include them were too important for a maintenance release. We are currently working on the minor release 3.3 which will contain these fixes and also new features : · Add Voice Authoring Activity · Add Activity Workflow Changes · Course Copy Support · Adding Voice Email as an Activity and a Block We have also considered adding the Voice Authoring Applet to Moodle WYSIWYG Editor Best Thomas On 28 juil. 08, at 01:04, Ashley Holman wrote: > Hi all, > > I'm just following up on the code review that Catalyst did a while > ago and whether those issues have been addressed in the latest code. > > I've attached the PDF again to reference. > > Thomas do you think we can check any of these off? I had a read > through them and the issues still seem to be present in v3.1.3. > > To summarise the issues they found: > > - getXmlChoicePage.php - Doesn't check $USER or require_login() > - use of $_POST, $_GET, $_REQUEST instead of required_param(), > optional_param() > - some concern about include paths > - CSS declaration for voice recorder block should move into > styles.php and use .block_bvoicerecorder class > > More details are in the PDF that they compiled. > > Do these issues need to be fixed, or if not could I get an > explanation about why they don't need to be fixed, so that I can > give a response back to our customer about it. I believe that this > particular customer engaged Catalyst to do a security review of the > Wimba/Moodle module, and they are holding off on implementing Voice > Tools until these are addressed. > > Thanks very much > > Regards > Ashley > > Thomas Rollinger wrote: >> >> >> 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 >> >> >> >> >> >> >> ------------------------------------------------------------------------- >> This SF.net email is sponsored by: Microsoft >> Defy all challenges. Microsoft(R) Visual Studio 2008. >> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ >> >> _______________________________________________ >> Hw4mdl-devel mailing list >> Hw4...@li... >> https://lists.sourceforge.net/lists/listinfo/hw4mdl-devel >> > > > -- > 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 > > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's > challenge > Build the coolest Linux based applications with Moblin SDK & win > great prizes > Grand prize is a trip for two to an Open Source event anywhere in > the world > http://moblin-contest.org/redirect.php?banner_id=100&url=/_______________________________________________ > Hw4mdl-devel mailing list > Hw4...@li... > https://lists.sourceforge.net/lists/listinfo/hw4mdl-devel |
