[PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Neil Horman]

Ok, here we go

As promised, I'm reposting the core_pattern enhancements I've done over the past
few days.  These three patches replace and conintue the work contained in the
following patches, and can replace them:
update-coredump-path-in-kernel-to-not-check-coredump-rlim-if-core_pattern-is-a-pipe.patch
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe.patch
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix.patch
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2.patch
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2-fix.patch
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-sparc64-fix.patch
allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2-sparc64-fix.patch


Changelog:
For some time /proc/sys/kernel/core_pattern has been able to set its output
destination as a pipe, allowing a user space helper to receive and intellegently
process a core.  This infrastructure however has some shortcommings which can be
enhanced.  Specifically:

1) The coredump code in the kernel should ignore RLIMIT_CORE limitation when
core_pattern is a pipe, since file system resources are not being consumed in
this case, unless the user application wishes to save the core, at which point
the app is restricted by usual file system limits and restrictions.

2) The core_pattern code should be able to parse and pass options to the user
space helper as an argv array.  The real core limit of the uid of the crashing
proces should also be passable to the user space helper (since it is overridden
to zero when called).

3) Some miscelaneous bugs need to be cleaned up (specifically the recognition of
a recursive core dump, should the user mode helper itself crash.  Also, the core
dump code in the kernel should not wait for the user mode helper to exit, since
the same context is responsible for writing to the pipe, and a read of the pipe
by the user mode helper will result in a deadlock.

All patches tested by me, with successful results.

Thanks & Regards
Neil


-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Eugene Teo]

Neil Horman wrote:
> Ok, here we go
> 
> As promised, I'm reposting the core_pattern enhancements I've done over the past
> few days.  These three patches replace and conintue the work contained in the
> following patches, and can replace them:
> update-coredump-path-in-kernel-to-not-check-coredump-rlim-if-core_pattern-is-a-pipe.patch
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe.patch
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix.patch
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2.patch
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2-fix.patch
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-sparc64-fix.patch
> allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2-sparc64-fix.patch
[...]

You may want to improve your patches with style-related changes, including
removing trailing spaces, using tabs instead of spaces, and defining pointers
like char *ptr instead of char * ptr.

Also, it is probably good to think how we can "drop privileges" while piping
the core dump output to an external program. A malicious user can potentially
use it as a possible backdoor since anything that is executed by "|program" will
be executed with root privileges.

Eugene

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Neil Horman]

On Sun, Jul 29, 2007 at 06:40:43PM +0800, Eugene Teo wrote:
> Neil Horman wrote:
> > Ok, here we go
> > 
> > As promised, I'm reposting the core_pattern enhancements I've done over the past
> > few days.  These three patches replace and conintue the work contained in the
> > following patches, and can replace them:
> > update-coredump-path-in-kernel-to-not-check-coredump-rlim-if-core_pattern-is-a-pipe.patch
> > allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe.patch
> > allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix.patch
> > allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2.patch
> > allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2-fix.patch
> > allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-sparc64-fix.patch
> > allow-individual-core-dump-methods-to-be-unlimited-when-sending-to-a-pipe-fix-2-sparc64-fix.patch
> [...]
> 
> You may want to improve your patches with style-related changes, including
> removing trailing spaces, using tabs instead of spaces, and defining pointers
> like char *ptr instead of char * ptr.
> 
I assume this is just a general comment, since as far as I can see, I've
followed those guidelines.

> Also, it is probably good to think how we can "drop privileges" while piping
> the core dump output to an external program. A malicious user can potentially
> use it as a possible backdoor since anything that is executed by "|program" will
> be executed with root privileges.
> 
It was my understanding that apport already did this.

Thanks and Regards
Neil

> Eugene

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Eugene Teo]

Neil Horman wrote:
> On Sun, Jul 29, 2007 at 06:40:43PM +0800, Eugene Teo wrote:
>> Neil Horman wrote:
[...]
>> You may want to improve your patches with style-related changes, including
>> removing trailing spaces, using tabs instead of spaces, and defining pointers
>> like char *ptr instead of char * ptr.
>>
> I assume this is just a general comment, since as far as I can see, I've
> followed those guidelines.

Please see the next few emails.

>> Also, it is probably good to think how we can "drop privileges" while piping
>> the core dump output to an external program. A malicious user can potentially
>> use it as a possible backdoor since anything that is executed by "|program" will
>> be executed with root privileges.
>>
> It was my understanding that apport already did this.

I haven't looked at apport yet, but are you talking about the userspace portion of
apport or the kernel changes in the Ubuntu kernel?

Eugene

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Martin Pitt]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

Hi Eugene,

Eugene Teo [2007-07-29 21:03 +0800]:
> >> Also, it is probably good to think how we can "drop privileges" while piping
> >> the core dump output to an external program. A malicious user can potentially
> >> use it as a possible backdoor since anything that is executed by "|program" will
> >> be executed with root privileges.
> >>
> > It was my understanding that apport already did this.
> 
> I haven't looked at apport yet, but are you talking about the userspace portion of
> apport or the kernel changes in the Ubuntu kernel?

Similarly to Neil's patches, the Ubuntu kernel calls the userspace
helper as root, too. Apport drops privileges to the target process as
soon as possible (there are a few things it needs to do before, like
opening an fd to the crash file in /var/crash/ if that is only
writeable by root).

Martin

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Eugene Teo]

Hi Martin,

Martin Pitt wrote:
> Eugene Teo [2007-07-29 21:03 +0800]:
>>>> Also, it is probably good to think how we can "drop privileges" while piping
>>>> the core dump output to an external program. A malicious user can potentially
>>>> use it as a possible backdoor since anything that is executed by "|program" will
>>>> be executed with root privileges.
>>>>
>>> It was my understanding that apport already did this.
>> I haven't looked at apport yet, but are you talking about the userspace portion of
>> apport or the kernel changes in the Ubuntu kernel?
> 
> Similarly to Neil's patches, the Ubuntu kernel calls the userspace
> helper as root, too. Apport drops privileges to the target process as
> soon as possible (there are a few things it needs to do before, like
> opening an fd to the crash file in /var/crash/ if that is only
> writeable by root).

Just sharing some thoughts. Wouldn't it be more logical to drop the privileges first,
then call the userspace helper program? I know that this will limit tools like apport
to be able to read and/or write files that are only writable by root, but there ought
to be a better way to do this? What if the program piped is not a legitimate program?

Also, maybe it is good to make this portion of the code optional too, so that if no
one is using this "ispipe" feature, we just turn it off.

Eugene

Re: [PATCH 0/3] core_pattern: cleaned up repost/continuing post of core_pattern enhancements

[Neil Horman]

On Mon, Jul 30, 2007 at 07:45:39AM +0800, Eugene Teo wrote:
> Hi Martin,
> 
> Martin Pitt wrote:
> > Eugene Teo [2007-07-29 21:03 +0800]:
> >>>> Also, it is probably good to think how we can "drop privileges" while piping
> >>>> the core dump output to an external program. A malicious user can potentially
> >>>> use it as a possible backdoor since anything that is executed by "|program" will
> >>>> be executed with root privileges.
> >>>>
> >>> It was my understanding that apport already did this.
> >> I haven't looked at apport yet, but are you talking about the userspace portion of
> >> apport or the kernel changes in the Ubuntu kernel?
> > 
> > Similarly to Neil's patches, the Ubuntu kernel calls the userspace
> > helper as root, too. Apport drops privileges to the target process as
> > soon as possible (there are a few things it needs to do before, like
> > opening an fd to the crash file in /var/crash/ if that is only
> > writeable by root).
> 
> Just sharing some thoughts. Wouldn't it be more logical to drop the privileges first,
> then call the userspace helper program? I know that this will limit tools like apport
> to be able to read and/or write files that are only writable by root, but there ought
> to be a better way to do this? What if the program piped is not a legitimate program?
> 
We could do that I suppose, but /proc/<pid of crashing process>/* contains
informatino apport (and other apps need) to help diagnose problems during a
crash.  To provide that information, we would then need to build out
infrastructure to pipe that information in-band through the pipe (perhaps
through ELF notes).  Doable yes, but certainly not a small patch (consider
piping all of the files in /proc/<pid> as ELF notes).

Regarding security, and the use of non-legit programs: If the program pointed to
by core pattern does not exist, then the exec simply fails, and the core is
lost.  Beyond that, core_pattern is only writable by root, and its teh sysadmins
responsibility to ensure that it points to valid and secured program.

> Also, maybe it is good to make this portion of the code optional too, so that if no
> one is using this "ispipe" feature, we just turn it off.
> 
you mean like a build time config option? I'm not sure I see lots of value,
although, it seems like it would straightforward enough to do if you feel
strongly about it.

Regards
Neil

> Eugene

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/