http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #1 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 10358 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10358&action=edit Bug 8268: Add database dump to export tool This patch builds on work by Lars Wirzenius for the Koha packages. To date, the only way for a Koha librarian to obtain a complete backup of their system has been to log into the system via SSH (or FTP) to download the mysqldump file. This patch makes it possible for superlibrarians in properly configured systems to download night backups via the staff client's Export tool. Recognizing that this is functionality with potentially very grave security implications, system administrators must manually enable these features in the koha-conf.xml configuration file. The following configuration settings have been added to the koha-conf.xml file: * backupdir => directory where backups should be stored. * backup_db_via_tools => whether to allow superlibrarians to download database backups via the Export tool. The default is disabled, and there is no way -- by design -- to enable this option without manually editing koha-conf.xml. * backup_conf_via_tools => whether to allow superlibrarians to download configuration backups via the Export tool (this may be applicable to packages only). The default is disabled, and there is no way -- by design -- to enable this option without manually editing koha-conf.xml. This commit modifies the following scripts to make use of the new backupdir configuration option: * koha-dump and koha-run-backups in the Debian packages * The sample backup script misc/cronjobs/backup.sh Note that for security reasons, superlibrarians will not be allowed to download files that are not owned by the web server's effective user. This imposes a de facto dependency on ITK (for Apache) or running the web server as the Koha user (as is done with Plack). To test: 1. Apply patch. 2. Go to export page as a superlibrarian. Notice that no additional export options appear because they have not been enabled. 3. Add <backupdir>$KOHADEV/var/spool</backup> to the <config> section of your koha-conf.xml (note that you will need to adjust that so that it is pointing at a logical directory). 4. Create the aforementioned directory. 5. Go to export page as a superlibrarian. Notice that no additional export options appear because they have not been enabled. 6. Add <backup_db_via_tools>1</backup_db_via_tools> to the <config> section of your koha-conf.xml 7. Go to the export page as a superlibrarian. Notice the new tab. 8. Go to the export page as a non-superlibrarian. Notice there is no new tab. 9. Run: mysqldump -u koha -p koha | gzip > $BACKUPDIR/backup.sql.gz (substituting appropriate user, password, and database name) 10. Go to the export page as a superlibrarian, and look at the "Export database" tab. If you are running the web server as your Koha user, and ran the above command as your Koha user, you should now see the file listed as an option for download. 11. If you *did* see the file listed, change the ownership to something else: sudo chown root:root $BACKUPDIR/backup.sql.gz 11a. Confirm that you no longer see the file listed when you look at the "Export database" tab. 12. Change the ownership on the file to your web server (or Koha) user: sudo chown www-data:www-data backup.sql.gz 13. Go to the export page as a superlibrarian, and look at the "Export database" tab. You should now see backup.sql.gz listed. 14. Choose to download backup.sql.gz 15. Confirm that the downloaded file is what you were expecting. If you are interested, you can repeat the above steps but replace <backup_db_via_tools> with <backup_conf_via_tools>, and instead of creating an sql file, create a tar file. To test packaging: run koha-dump, confirm that it still creates a usable backup. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Marc Balmer <marc@msys.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |chris@bigballofwax.co.nz --- Comment #5 from Chris Cormack <chris@bigballofwax.co.nz> --- Applying: Bug 8268: Add database dump to export tool fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/export.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 8268: Add database dump to export tool -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #7 from Robin Sheat <robin@catalyst.net.nz> --- It applies for me: robin@momomoto:~/catalyst/koha$ git bz apply 8202 Bug 8202 - Add authorities to export tool [SIGNED-OFF] Bug 8202: Add authority export to tools/export.pl Apply? [yn] y Applying: Bug 8202: Add authority export to tools/export.pl robin@momomoto:~/catalyst/koha$ git bz apply 8268 Bug 8268 - Koha should offer way to backup entire db Bug 8268: Add database dump to export tool Apply? [yn] y Applying: Bug 8268: Add database dump to export tool /home/robin/catalyst/koha/.git/rebase-apply/patch:117: trailing whitespace. ls "$backupdir/" | /home/robin/catalyst/koha/.git/rebase-apply/patch:307: trailing whitespace. print $query->header( -type => $mimetype, warning: 2 lines add whitespace errors. robin@momomoto:~/catalyst/koha$ -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off --- Comment #9 from Robin Sheat <robin@catalyst.net.nz> --- Things noted: 10-1. If you forgot to run the backup command, then there is nothing to choose from but the download button is still there. Not a big issue, just slightly cosmetic. Also, the "no downloads exist" message is missing, I think due to different semantics between T::T and perl about whether an empty array is considered "true." 10-2. This doesn't work with the packages, as it's testing -o (do I own this file) not -r (can I read this file.) Dumps provided by the packages are owned by root, but readable by the webserver due to group permissions. This needs to be fixed in getbackupfilelist and download_backup. My signed off patch fixes these. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |paul.poulain@biblibre.com, | |semarie-koha@latrappe.fr Version|master |rel_3_10 --- Comment #11 from Paul Poulain <paul.poulain@biblibre.com> --- QA comment: You add 2 subs: getbackupfilelist and download_backup => bad point, we usually use capitals (GetBackupFileList and DownloadBackup). However, this is not in a package, so I won't fail QA for that. Other than this, I can't find anything wrong in your code, the security triple check (ENV and unix permission, and superlibrarian) is perfect. I'm adding semarie, that is a boss at finding security holes. (Cher frère, si tu trouves un problème, ne te prive pas !) Marking passed QA, I'll delay pushing, I'll wait for some feedback from semarie, just in case -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #12 from Frère Sébastien Marie <semarie-koha@latrappe.fr> --- (En réponse au commentaire 11)

Marking passed QA, I'll delay pushing, I'll wait for some feedback from semarie, just in case

The code seems globally ok: just 4 remarks. Some security remarks (in tools/export.pl): 1. a problem (common in koha): use of user-input in warn. An attacker can forge entries in log file (see CWE-93) (resume: if variable contains CRLF, the log file too... and the next line is forged: so who write in log ? koha or attacker ?)

warn "A suspicious attempt was made to download the db at '$filename' by ..."

2. about the regex for $filename verification

return unless ( ... && not $filename =~ m#(^\.\.|/)# );

The regex could be translated as: $filename should not: - start with '..' OR - contains '/' I don't understand the purpose of "^..", if '/' is forbidden; as you can't escape from directory without '/'. So I think m#/# is suffisent (please correct me if not). Some generals remarks: 3. the directory for backup is not consitent in differents files: in debian/templates/koha-conf-site.xml.in, the directory use for backupdir is:

<backupdir>/var/lib/koha/__KOHASITE__</backupdir>

in Makefile.PL, the directory seems to be in /var/spool :

'./skel/var/spool/koha' => { target => 'BACKUP_DIR', trimdir => -1 },

in debian/scripts/koha-dump:

[ -z "$backupdir" ] && backupdir="/var/spool/koha/$name"

4. in misc/cronjobs/backup.sh The lasts lines are for mail admin if ok, or mail admin if not ok.

[ -f $KOHA_BACKUP] && echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP || echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP

First, there are *two* lines. I haven't test it, but it should be an error as "cmd_a && cmd_b ||" is not a valid shell expression. Should be one-line "cmd_a && cmd_b || cmd_c" Secondly, I'm not sure that the behaviour is correct. "cmd_a && cmd_b || cmd_c" could be expanded as: if cmd_a ; then if ! cmd_b ; then cmd_c fi else cmd_c fi so if cmd_b failed (echo "backup ok" | mail admin), the cmd_c (echo "backup err" | mail admin) will be run (and expected to failed too, as the first try). Proposed correction:

if [ -f $KOHA_BACKUP] ; then echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP else echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP fi

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #14 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 10682 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10682&action=edit Bug 8268 follow-up: incorporate QA comments Fixes the following things: 1. Sanitizes log output to prevent an attacker from using a specially crafted POST to add extra lines to the log 2. Simplify a regular expression since "..file" cannot be used to escape the current directory 3. Makes sure directories are consistent 4. Correct logic issues in misc/cronjobs/backup.sh Thanks to Frère Sébastien Marie for catching these issues. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |ASSIGNED --- Comment #16 from Paul Poulain <paul.poulain@biblibre.com> --- Since the follow-up changes sensible things like: + <backupdir>/var/spool/koha/__KOHASITE__</backupdir> I think it's worth a signoff before pushing. Robin, could you please signoff the follow-up ? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #17 from Robin Sheat <robin@catalyst.net.nz> --- Jared, something you did causes this in your patch: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="UTF-8" which causes git to say: fatal: cannot convert from UTF-8UTF-8 to UTF-8 I should be able to work around it easily, but just so you know you did something weird :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10682|0 |1 is obsolete| | --- Comment #19 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 10749 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10749&action=edit Bug 8268 follow-up: incorporate QA comments Fixes the following things: 1. Sanitizes log output to prevent an attacker from using a specially crafted POST to add extra lines to the log 2. Simplify a regular expression since "..file" cannot be used to escape the current directory 3. Makes sure directories are consistent 4. Correct logic issues in misc/cronjobs/backup.sh Thanks to Frere Sebastien Marie for catching these issues. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10749|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #21 from Paul Poulain <paul.poulain@biblibre.com> --- QA comments: Frere Sebastien Marie suggestions/concern taken in account, passed QA About

...

You add 2 subs: getbackupfilelist and download_backup => bad point, we usually use capitals (GetBackupFileList and DownloadBackup). However, this is not in a package, so I won't fail QA for that.

Our standard is to use lowercase in .pl files and uppercase in packages, I think. I suppose that makes sense, since it ensures that there will be a visual distinction between local functions and core functions.

You're right, sorry for the annoyance. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #23 from Paul Poulain <paul.poulain@biblibre.com> --- I've found a problem with this patch, that you can see when you run plack: Variable "$backupdir" is not available at /home/paul/koha.dev/koha-community/tools/export.pl line 334. => the backup features won't work under plack (problem of variable scoping with Plack that put the script in a sub, making main() variable unaccessible in other subs. Problem solved everywhere with a our instead of my. The best option, that I would prefer -from far- for this new feature, being that we don't use such a our variable, but define backupdir where it's needed (+ it's just reading of a syspref, so not something costly) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #24 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Suspecting this recently pushed patch from following: Koha installation crashes on not existent directory: var/spool. This was a dev install. chmod: cannot access `/usr/share/kohadev/newclone/var/spool': No such file or directory make: *** [set_koha_permissions] Error 1 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #26 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 10869 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10869&action=edit Followup for fixing install Adds a file to skel/var/spool/koha to create subdir -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10471|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P2 Severity|enhancement |critical --- Comment #27 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This follow-up is required for new, non-package installations to work. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #28 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- *** Bug 8467 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #29 from Paul Poulain <paul.poulain@biblibre.com> --- Trivial and neede follow-up, passed QA -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #30 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Jared, In a manual dev install I do still come across this line in koha-conf.xml: <backupdir>__BACKUP_DIR__</backupdir> Note that install finished apparently correct.. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 --- Comment #34 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- Created attachment 11208 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11208&action=edit Bug 8268: improve error checking Valid download attempts were being denied thanks to an incorrect regular expression. This patch fixes that, and makes it easier to understand what's going on in the code. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #35 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- The first patch has been signed off and passed QA, but the second has not. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master Severity|critical |enhancement --- Comment #38 from Paul Poulain <paul.poulain@biblibre.com> --- follow-ups http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11187 and http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11262 pushed today (status switched back to "ENH", as this bug describe an enhancement -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268 Bug 8268 depends on bug 8202, which changed state. Bug 8202 Summary: Add authorities to export tool http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8202 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.