Re: [Hw4mdl-devel] Catalyst code review - comments please
Brought to you by:
jhlinder,
trollinger
Menu
▾
▴
Re: [Hw4mdl-devel] Catalyst code review - comments please
|
From: Ashley H. <as...@ne...> - 2008-07-29 06:52:53
|
Hi Thomas,
That's great to hear, much appreciated. I would be happy to help QA the
3.3 code when you're ready to release it (on sourceforge?).
I noticed in the 3.1.3 release that there are still several occurences
of PHP short tags, which will cause issues for servers that have
short_tags turned off. These are all the short tags I could find with grep:
./mod/voicetools/manageAction.php:196: <?
./mod/voicetools/error.php:39: <?require_once("../../config.php"); ?>
./mod/voicetools/view.php:159: <?
./mod/voicetools/view.php:164: doOpenPopup("<?php echo
$servername ?>/<? echo
$resource->getType();?>?action=display_popup&nid=<?php echo
$vtSession->getNid() ?>","<?php echo $resource->getType()?>");
./mod/voicetools/view.php:172: <?}else{
./mod/voicetools/view.php:175: doOpenPopup("<?php echo
$servername ?>/<? echo
$resource->getType();?>?action=display_popup&nid=<?php echo
$vtSession->getNid() ?>","<?php echo $resource->getType()?>");
./mod/voicetools/view.php:177: <?}?>
./mod/voicetools/mod.html:573: <?}?>
./mod/voicetools/mod.html:935:
<?
./mod/voicetools/mod.html:967:
<? }?>
./mod/voicetools/mod.html:994:
<font class="fontCurrent"><?echo
get_string('duration_calendar','voicetools')?></font>
./mod/voicetools/mod.html:1023:
<font class="fontCurrent"><? echo get_string('description_calendar',
'voicetools')?></font>
./mod/voicetools/mod.html:1149:<?
./mod/voicetools/mod.html:1158:<?
./mod/liveclassroom/view.php:274:<?
./mod/liveclassroom/doAction.php:156: <?
./mod/liveclassroom/mod.html:447: <?
./mod/liveclassroom/mod.html:573: <? }
./mod/liveclassroom/mod.html:869:
<?
./mod/liveclassroom/mod.html:901:
<? }?>
./mod/liveclassroom/mod.html:929:
<font class="fontCurrent"><?echo
get_string('duration_calendar','liveclassroom')?></font>
./mod/liveclassroom/mod.html:958:
<font class="fontCurrent"><? echo get_string('description_calendar',
'liveclassroom')?></font>
Regarding version 3.3, I had a discussion with Jeff O'Connell about a
feature suggested to us by one of our customers. They wanted to be able
to use Moodle's "groupings" feature to be able to restrict access to
VT/LC activities to specific groupings within Moodle. This
functionality is implemented in the standard Moodle modules, eg.
assignments and resources, however it is missing in the Wimba module.
Is it possible that this feature could be included in the spec for
v3.3? I can provide more detailed information on the
requirements/implementation of this feature if you like. If you can't
fit this in the plans for v3.3, I may be able to spend some time
implementing it and send you a patch if you are able to share the trunk
code with me.
Thanks as always
Ashley
Thomas Rollinger wrote:
> 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... <mailto: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=/_______________________________________________
>> <http://moblin-contest.org/redirect.php?banner_id=100&url=/_______________________________________________>
>> Hw4mdl-devel mailing list
>> Hw4...@li...
>> https://lists.sourceforge.net/lists/listinfo/hw4mdl-devel
>
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> 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
>
--
Ashley Holman
Software Engineer
NetSpot Pty Ltd
183 Melbourne Street, North Adelaide, 5006
Ph: +618 8361 6800, Fax: +618 8361 6811
|
