tmccormi wrote on Friday, March 15, 2013:
There has been some off list discussion about the contrib directory, it contents and it’s effect on security. We decided this discussion should move to the forum. So here is the thread for public comsumption and discussion…
Original Message From: “Kevin Yeh” <kevin.h.yeh@gmail.com>
The “aribtrary” upload issue Tim points out is again from the contrib
directory. We really should just remove all that code from the
distributed package. Given that all of those files will still be
available in github and old archives, in my mind the benefit to
security far outweighs the “loss of knowledge/utility.”
Art
Contribs folder must die.
===
I will remove the contrib folders and commit those changes this weekend. We should post a blast on SF that
users should do the same on their own sites.
Tony
We can provide a zip/tar file for easy download for users to use “at their own risk” for the time being and them
provide these items back to the community after they have been scrubbed. Maybe in a new “plug in” format
like ZHS is proposing…
Tony
Tony,
I assume you mean the contrib/forms directory.
Don’t remove scanned_notes. That one is important to current users. If it has a vulnerability, let me know and
I’ll fix it.
Thanks,
Rod
Kevin Yeh
It should also be easy enough to create auxiliary github repo
something akin to this:
https://github.com/yehster/contribopenemr
=============
Kevin Yeh
Does it make sense to move the scanned_notes form into the “official
form directory” but not “fully installed?”
=============
Yes I think so.
Rod
I’ll limit it to forms, if you think I should, but I really think we should drop it all and add back in the things that
can pass the security code review and should be “included” officially.
Tony
=============
There’s lots of stuff in contrib that we need. However I don’t think any of it has any business being in the web
directory, so perhaps this is more a matter of changing installation procedures.
For the short term, this could be as simple as adding a note to the postinstallation instructions in setup.php,
indicating that the user should move the contrib directory somewhere else, or change its permissions so it
cannot be accessed by the web server. And of course installation scripts can automate this.
Rod
We could leave a README in that dir with a link to the location …
I don’t think most users even read that stuff… what if we “shipped” it compressed (zip file?) and put
instructions to unzip if needed …
Tony
=============
Sam Likins
Taking that one step further, have an option to uncompress during install to a safe directory.
There are other directories that should not be readable by the web server, such as sites/default/documents/.
The postinstallation instructions already talk about those. Agreed it’s an important thing that many users will
ignore but if our intent is to preempt things like that, then setup (or OpenEMR itself) could refuse to run if
such directories are unprotected. There are .htaccess files in some places already to help with this, and one
could be put into contrib as well.
I’d like to avoid mass disruption of the repository if at all possible. The change history has value.
So my tentative suggestions are:
1. Change setup.php and/or OpenEMR to refuse to run if any directory that should be protected is not.
2. Add a .htaccess file to contrib.
3. Change Linux distributions of OpenEMR to give only root permissions to contrib.
4. Otherwise leave contrib pretty much as it is.
Rod
I disagree. I still think it should all be ripped out and provided via the website.
For sure the forms need to be ripped out mostly, many of them don’t even work anymore and the rest don’t meet
security guidelines (at least).
Tony
=============
This is bad… from the contrib/acog directory README
“This was all gathered up in a hurry and is not fully debugged yet. If you
find that a form is broken because it refers to one of these files in a
different location, please fix the form to reference the file in this location
(contrib/acog/) instead.”
What about the RXNORM and SNOMED dirs? Are those used directly by code (ick!) or has this stuff been
moved to production locations now?
And this Zirmed thing from 2006 …
Zirmed.php copyright 2006 Mark Leeds
This script should be placed under the main openemr directory and can be accessed directly by typing in the
url in your browser or you can edit your main menu to place a link to it (that’s what I did). … It is not intended to
be used as is.
Tony
… formmaker and xmlform gen should be moved to util directory. They are not “forms” themselves
Tony
=============
The contrib/util directory needs to stay though I don’t think that the installation scripts should be here really, but I
putting them in a separate repo is over kill…
Oh, I do agree with adding a .htaccess file to the contrib dir whatever happens.
Tony
=============
The deidentification scripts and the code_systems scripts both reference contrib dirs … I don’t think ANY
production code should be doing that.
Tony
Might help to clearly state the problem(s) that we’re trying to solve in this discussion.
If it’s to keep the web server from accessing directories that it shouldn’t, then a mechanism can be built to solve
that, and as a side effect moving or “cleaning out” the contrib directory becomes unnecessary.
If it’s to take eyes away from stuff that doesn’t work very well, keep in mind that no matter how bad a form might
be, someone put work into it and it’s likely to encapsulate useful knowledge for someone who wants to get it
working or do something related. Some forms that may (or may not) just need security cleanup I happen to
know are already valuable and working, though not widely used (which is why they are in contrib). And there’s
also the issue I already mentioned about change history having value.
Anyway, let’s get some feedback from others and then do something. It’s not THAT big a deal.