Bug 9812 - Several files shouldn't be exposed or browseable through a URL
Summary: Several files shouldn't be exposed or browseable through a URL
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: 3.14
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 28236
  Show dependency treegraph
 
Reported: 2013-03-13 17:28 UTC by Tomás Cohen Arazi
Modified: 2021-04-27 11:58 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9812 - Forbid access to several files through the browser (1.63 KB, patch)
2013-03-13 17:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9812 - Forbid access to several files through the browser (1.68 KB, patch)
2013-04-01 06:54 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9812 - Forbid access to several files through the browser (1.74 KB, patch)
2013-04-03 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2013-03-13 17:28:02 UTC
Some templates data shouldn't be exposed to access on a browser. This includes the "modules", "xslt" and "includes" dirs.
Comment 1 Tomás Cohen Arazi 2013-03-13 17:35:54 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2013-03-13 17:52:05 UTC
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!
Comment 3 Paul A 2013-03-14 15:31:33 UTC
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.
Comment 4 Tomás Cohen Arazi 2013-03-14 15:35:35 UTC
(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.
Comment 5 Paul A 2013-03-14 20:02:11 UTC
(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.
Comment 6 Chris Cormack 2013-04-01 06:54:38 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-04-03 13:12:33 UTC
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.
Comment 8 Jonathan Druart 2013-04-03 13:12:45 UTC
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>
Comment 9 Jared Camins-Esakov 2013-04-07 17:44:52 UTC
This patch has been pushed to master and 3.12.x.
Comment 10 Chris Cormack 2013-04-25 06:07:13 UTC
Pushed to 3.10.x and 3.8.x will be in 3.10.6 and 3.8.13
Comment 11 Tomás Cohen Arazi 2013-04-26 15:11:47 UTC
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/)
Comment 12 Katrin Fischer 2013-04-26 15:19:30 UTC
Moving this to Passed QA to make it show up in Jared's list - revert request from the patch author.
Comment 13 Marcel de Rooy 2013-04-26 17:11:52 UTC
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!
Comment 14 Chris Cormack 2013-04-26 21:27:45 UTC
Reverted from 3.10.x and 3.8.x
Comment 15 Jared Camins-Esakov 2013-04-26 22:19:32 UTC
Reverted from 3.12.x. RM for 3.14 can make the call for master.
Comment 16 Galen Charlton 2013-05-21 18:29:25 UTC
(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.
Comment 17 Galen Charlton 2013-07-15 15:31:48 UTC
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.