Bug 6627 - [security] insecure file creation
Summary: [security] insecure file creation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.6
Hardware: All All
: P1 - high blocker (vote)
Assignee: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-25 08:02 UTC by Sébastien Marie
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch to ensure higher security with relation to log files (4.01 KB, patch)
2011-12-21 02:14 UTC, Duncan
Details | Diff | Splinter Review
Bug 6627 [Signed Off] - Update to improve security in specified log files - bug 6627 (4.25 KB, patch)
2011-12-30 19:39 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sébastien Marie 2011-07-25 08:02:48 UTC
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).
Comment 1 Duncan 2011-12-21 02:14:24 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-12-21 13:01:24 UTC
(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..
Comment 3 Chris Cormack 2011-12-22 19:16:27 UTC
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.
Comment 4 Katrin Fischer 2011-12-22 19:34:24 UTC
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?
Comment 5 Chris Cormack 2011-12-22 19:35:18 UTC
Or an antifeature in this case.
Comment 6 Sébastien Marie 2011-12-23 05:38:53 UTC
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...)
Comment 7 Liz Rea 2011-12-30 19:39:39 UTC
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.
Comment 8 Ian Walls 2012-01-05 12:58:14 UTC
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
Comment 9 Paul Poulain 2012-01-06 13:29:51 UTC
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 !
Comment 10 Paul Poulain 2012-01-06 13:35:43 UTC
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 !
Comment 11 Jonathan Druart 2012-01-11 11:11:02 UTC
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 :-/
Comment 12 Chris Cormack 2012-01-11 18:21:18 UTC
(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.
Comment 13 Paul Poulain 2012-01-13 09:49:56 UTC
(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 ?
Comment 14 Jonathan Druart 2012-01-13 09:59:55 UTC
@Paul: sorry, it's ok in master. But the patch doesn't comment all lines
Comment 15 Sébastien Marie 2012-02-07 15:12:00 UTC
Thanks