search

vfremaux / moodle-local_shop

core plugin for a moodle integrated shop system. this is the community version
4 stars 3 forks source link

hard to assess if get_handler is safe #5

Open danmarsden opened 6 years ago

danmarsden commented 6 years ago

https://github.com/vfremaux/moodle-local_shop/blob/master/classes/CatalogItem.class.php#L348

Hard to see how enablehandler is set - can it be used to perform directory traversal? - might be good to add some validation in that function so that it is easy for a code reviewer to see that it only allows access to local_shop classes.

danmarsden commented 6 years ago

another similar function: https://github.com/vfremaux/moodle-local_shop/blob/master/classes/Product.class.php#L342

vfremaux commented 6 years ago

What needs to be known about architecture ::

  1. Handlers are pre-payment and post-payment processing additions that trigger when a product is purchased.

  2. The shop structurally accepts a set of handler presets (standard operations), but structurally will allow developers to develop "specific handlers" using the catalog item code as file and class base name. This is what get_handler() tries to do : find the adequate handler if there is any "specific handler" in the way.

  3. The shop administrator can at any moment enable or disable the handler for any catalog item. Disabling the handler will just disable the automated processing and will jsut register the purchase. The bill will remain in PENDING state and the commercial staff will have to process operations manually in the backoffice and in moodle administration.

danmarsden commented 6 years ago

cool - I'd still recommend you add some explicit cleaning of those params within the function so that it is really obvious to a code reviewer than no directory traversal using those parameters is possible.

vfremaux commented 6 years ago

would you mean something as using .. in some kind of input to reach some unwanted location in the server ? f.e. tweaking the product code ? I guess it is not possible as the product code is cleaned out and the specific handler name is an explicit construct based on product code name.

danmarsden commented 6 years ago

Anyone doing a security review of your code will see this and will have to investigate your code-path further to discover where these params are set and how safe they are. https://github.com/vfremaux/moodle-local_shop/blob/master/classes/CatalogItem.class.php#L366

if $thehandler comes from an "action" param, or is able to be manipulated outside your normal workflow then it might be possible for someone to put something like "../../.." into $thehandler param and perform a directory traversal attack. Potentially any file with .class.php would be vulnerable on the server.

You should really be validating that param before using it in an include path - using something like clean_param($var, PARAM_ALPHA) - or PARAM_PATH which protects from directory traversal too. and if you put that cleaning directly inside the function right before using it in the include it would make it clear to any reviewer that directory traversal is not possible.

make sense?