Some templates data shouldn't be exposed to access on a browser. This includes the "modules", "xslt" and "includes" dirs.
Created attachment 16098 [details] [review] Bug 9812 - Forbid access to several files through the browser This patch hides (-Indexes) and forbids (Deny from all) access to some stuff through a browser. Specifically "xlst", "modules" and "includes" dirs and its contents. This is just a quick fix we talked about at IRC. The proper solution would be to remove this from htdocs which will still be needed.
Test plan ========= - Install Koha from master - Go there in your browser: http://YOUR_DEV_KOHA_OPAC_FQDN/opac-tmpl/prog/en/ - You should be able to browse every dir in the templates dir - Apply the patch and reinstall using $ perl Makefile.PL --prev-install-log ~/path/to/koha-install-log - Retry, you shouldn't be able to browse or access any files in this dirs: * modules * xslt * includes For your convenience this paths should be valid for testing: /opac-tmpl/prog/en/modules/kohaerror.tt /opac-tmpl/prog/en/includes/usermenu.inc /opac-tmpl/prog/en/xslt/MARC21slim2OPACDetail.xsl /opac-tmpl/ccsr/en/includes/doc-head-close.inc This should do it for the staff client too, but... its too much work for such a trivial patch!
Validated on my sandbox (Koha 3.8.5, Ubuntu 12.04.2 LTS), but note I'm not sure of the role of /etc/koha-httpd.conf (upgrades, etc?) as for every tarball install of Koha that I've ever done, I prefer etc/apache2/{whatever_file[s]}. This requires: service apache2 reload /etc/init.d/apache2 restart I'm also not sure why memcached is mentioned in the patch; the relevant SetEnv directives probably don't need to be touched.
(In reply to comment #3) > Validated on my sandbox (Koha 3.8.5, Ubuntu 12.04.2 LTS), but note I'm not > sure of the role of /etc/koha-httpd.conf (upgrades, etc?) as for every > tarball install of Koha that I've ever done, I prefer > etc/apache2/{whatever_file[s]}. This requires: > > service apache2 reload > /etc/init.d/apache2 restart I'm glad it worked for you. > I'm also not sure why memcached is mentioned in the patch; the relevant > SetEnv directives probably don't need to be touched. They remain untouched, don't worry. The only modified lines are those that start with a plus (+) symbol.
(In reply to comment #4) > (In reply to comment #3) > > Validated on my sandbox (Koha 3.8.5, Ubuntu 12.04.2 LTS), but note I'm not > > sure of the role of /etc/koha-httpd.conf (upgrades, etc?) as for every > > tarball install of Koha that I've ever done, I prefer > > etc/apache2/{whatever_file[s]}. This requires: > > > > service apache2 reload > > /etc/init.d/apache2 restart > > I'm glad it worked for you. In fact, this only was tested on our 64-bit sandbox - our production server uses a slightly different approach (less elegant. A wider 'deny', then specific 'allows'), with the same result. I'll probably update production at next scheduled maintenance. > > > I'm also not sure why memcached is mentioned in the patch; the relevant > > SetEnv directives probably don't need to be touched. > > They remain untouched, don't worry. The only modified lines are those that > start with a plus (+) symbol. My bad - read it too quickly - but of course did not modify our memcached directives.
Created attachment 17090 [details] [review] Bug 9812 - Forbid access to several files through the browser This patch hides (-Indexes) and forbids (Deny from all) access to some stuff through a browser. Specifically "xlst", "modules" and "includes" dirs and its contents. This is just a quick fix we talked about at IRC. The proper solution would be to remove this from htdocs which will still be needed. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
QA comment: Tested on an existing install, it works great. I didn't find any reason not to pass qa this patch. Marked as Passed QA.
Created attachment 17166 [details] [review] Bug 9812 - Forbid access to several files through the browser This patch hides (-Indexes) and forbids (Deny from all) access to some stuff through a browser. Specifically "xlst", "modules" and "includes" dirs and its contents. This is just a quick fix we talked about at IRC. The proper solution would be to remove this from htdocs which will still be needed. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch has been pushed to master and 3.12.x.
Pushed to 3.10.x and 3.8.x will be in 3.10.6 and 3.8.13
This bug should be reverted until we find a solution for: - value_builder scripts denied access to their corresponding .xml files - browser is denied access to favicon.ico (available in includes/)
Moving this to Passed QA to make it show up in Jared's list - revert request from the patch author.
Just a *dumb* question: But why should these "open source files" -- by no means :) -- be exposed through the browser? Much of this stuff will be from the standard install, available online elsewhere. Some small customizations are probably not of a "to be hidden nature". The larger custom work that for some reason should not be public (pity btw! we encourage to submit patches) can be hidden by a pro :) Not in any way wanting to discourage your sending of patches!
Reverted from 3.10.x and 3.8.x
Reverted from 3.12.x. RM for 3.14 can make the call for master.
(In reply to comment #13) > Just a *dumb* question: But why should these "open source files" -- by no > means :) -- be exposed through the browser? > Much of this stuff will be from the standard install, available online > elsewhere. > Some small customizations are probably not of a "to be hidden nature". > The larger custom work that for some reason should not be public (pity btw! > we encourage to submit patches) can be hidden by a pro :) > > Not in any way wanting to discourage your sending of patches! Well, the motivation isn't to hide code or customizations per se, it's to reduce the risk that the webserver could be made to send out sensitive configuration information, e.g., DB passwords or the like. In this specific case, there isn't anything (to my knowledge) in modules, xslt, and includes that would be useful to an attacker, although I could certainly see a customizer getting lazy and (say) hardcoding credentials into a template. The upshot is that I see this patch as a useful direction to be thinking towards, and I'm not opposed to pushing it (once Tomás' concerns are addressed), but I think even better would be to move Since the revert is done, I'm setting this one to in discussion. I'm also setting the the criticality back to 'normal'. If there is a *specific* security issue that warrants blocker status, please let me know.
I just noticed that due to an apparent timing issue, this patch was never actually reverted from master. I'm going to run with leaving it in, as I have a patch series in progress in bug 10592 that fixes the value_builder and favicon issues.