Some files are insecurely created in /tmp system-directory. File: C4/Auth.pm '/tmp/sessionlog' File: installer/InstallAuth.pm '/tmp/sessionlog' File: installer/externalmodules.pl '/tmp/modulesKoha.log' File: C4/Print.pm '/tmp/kohares' As all have well-know names, don't survive a reboot and are hosted in 1777 directory (/tmp), it is possible, for *any* user on the host, to create a symlink, that koha will use to alter any files (respecting his permissions). I suggest to create (and use) a special directory for all of them. The debian place should be /var/lib/koha/. This directory should be readable/writeable by the apache user (www-data) only (and eventually, by the owner of crontab, if need).
Created attachment 6897 [details] [review] Patch to ensure higher security with relation to log files Removed session logging, no useful information was being stored and was just a liability. Left 'externalmodules.pl' untouched, does anyone know whether this is useful or not, is it still wanted? Placed returns in the print methods for Print.pm as they were not being used except by themselves or tests. The printslip method was being used elsewhere but was calling remoteprint which is currently broken and needs repairing in order for it to be of any use if it is even wanted.
(In reply to comment #1) > Removed session logging, no useful information was being stored and was just a > liability. Thanks for your patch. I agree that current situation could be unsafe, but I do not agree just removing session log information under the flag of not useful information. It could be useful to know who was logged in at at a certain time. Could you write a follow up that gives me that information in a table? That table could be cleaned up regularly in cleanup_database as has been done for more tables..
I actually told Duncan to just remove it as it was useless in it's current form and insecure. It won't work with multiple sites on the same machine. I think if people want a follow up putting the information in the DB, they could write that. But I definitely do not think that warrants not stopping the current very insecure practice. So I posit very strongly, that we should accept this patch, then accept a follow up later.
I was not even aware of those logs and I am not sure anybody is using them. I think security tops a feature that is perhaps not used at all?
Or an antifeature in this case.
M. de Rooy, I agreed with you: know who was logged at specific time is important. It is a security measure (logging auth information). But, I also agreed with Chris Cormack... It is better to remove file logging now, and accept a rewrite later. So, +1 for remove current file logging implementation. Some elements for secure and useful logging: - use syslog (but what is the windows compatibility ?) - use koha logging facility (like for modification of patron, syspref...)
Created attachment 6984 [details] [review] Bug 6627 [Signed Off] - Update to improve security in specified log files - bug 6627 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Verified sessionlog file is not written to /tmp when patch is applied. Also verified original bug - logging in did in fact log to /tmp.
I would also agree that security is more important than an uncommonly used "feature". This patch comments out lines rather than deleting them, and sometimes adds returns before prints would otherwise be executed. All in all, this is a safe way of handling this issue until we get a more robust followup to re-enable (or reimplement) the lost logging Passed QA
patch pushed, please test I've added a followup on InstallAuth.pm to comment another occurrence of this buggy code QA comment: i'm not sure we should have commented, I think/feel removing the lines would be better here. But as it's a real problem, i push. If you think it would be better to completly remove those lines, let me know !
The more I think of it, the more I think we should remove and not just comment. And we should also remove sub _session_log !
In installer/InstallAuth.pm, the patch comments the line : # open L, ">>/tmp/sessionlog"; but L is used : printf L "%20s from %16s logged out at %30s (manually).\n", $userid, $ip, $time; I think perl doesn't like it :-/
(In reply to comment #11) > In installer/InstallAuth.pm, the patch comments the line : > # open L, ">>/tmp/sessionlog"; > > but L is used : > printf L "%20s from %16s logged out at %30s (manually).\n", $userid, $ip, > $time; > > I think perl doesn't like it :-/ Ahhh looks like the original patch was, the followup broke it. Paul can you fix your followup please.
(In reply to comment #11) > In installer/InstallAuth.pm, the patch comments the line : > # open L, ">>/tmp/sessionlog"; > > but L is used : > printf L "%20s from %16s logged out at %30s (manually).\n", $userid, $ip, > $time; > > I think perl doesn't like it :-/ I can't find this line (uncommented) anywhere in the code. I also made a reinstall and could not see anything wrong. Could you give me more details ?
@Paul: sorry, it's ok in master. But the patch doesn't comment all lines
Thanks