Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1441531 > unrolled thread

[PATCH] prctl: remove one-shot limitation for changing exe link

Back to article view | Back to linux.kernel

Contents

  [PATCH] prctl: remove one-shot limitation for changing exe link Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> - 2016-07-12 18:20 +0200
    Re: [PATCH] prctl: remove one-shot limitation for changing exe link Oleg Nesterov <oleg@redhat.com> - 2016-07-12 18:50 +0200
      Re: [PATCH] prctl: remove one-shot limitation for changing exe link Oleg Nesterov <oleg@redhat.com> - 2016-07-12 19:10 +0200
    Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-07-12 18:50 +0200
      Re: [PATCH] prctl: remove one-shot limitation for changing exe link ebiederm@xmission.com (Eric W. Biederman) - 2016-07-12 19:10 +0200
        Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-07-12 19:30 +0200
          Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-07-12 23:50 +0200
        Re: [PATCH] prctl: remove one-shot limitation for changing exe link One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-07-18 22:20 +0200
    Re: [PATCH] prctl: remove one-shot limitation for changing exe link ebiederm@xmission.com (Eric W. Biederman) - 2016-07-25 20:40 +0200
      Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-07-25 21:30 +0200
        Re: [PATCH] prctl: remove one-shot limitation for changing exe link ebiederm@xmission.com (Eric W. Biederman) - 2016-07-25 22:20 +0200
          Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-07-26 10:40 +0200
            Re: [PATCH] prctl: remove one-shot limitation for changing exe link ebiederm@xmission.com (Eric W. Biederman) - 2016-07-30 19:50 +0200
              Re: [PATCH] prctl: remove one-shot limitation for changing exe link Mateusz Guzik <mguzik@redhat.com> - 2016-07-30 22:30 +0200
                Re: [PATCH] prctl: remove one-shot limitation for changing exe link ebiederm@xmission.com (Eric W. Biederman) - 2016-07-31 21:00 +0200
              Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-08-01 00:50 +0200
                Re: [PATCH] prctl: remove one-shot limitation for changing exe link Andy Lutomirski <luto@amacapital.net> - 2016-08-01 01:00 +0200
              Re: [PATCH] prctl: remove one-shot limitation for changing exe link Cyrill Gorcunov <gorcunov@gmail.com> - 2016-08-01 11:30 +0200

#1441531 — [PATCH] prctl: remove one-shot limitation for changing exe link

This limitation came with the reason to remove "another
way for malicious code to obscure a compromised program and
masquerade as a benign process" by allowing "security-concious program can use
this prctl once during its early initialization to ensure the prctl cannot
later be abused for this purpose":

http://marc.info/?l=linux-kernel&m=133160684517468&w=2

But the way how the feature can be used is the following:

1) Attach to process via ptrace (protected by CAP_SYS_PTRACE)
2) Unmap all the process file mappings, related to "exe" file.
3) Change exe link (protected by CAP_SYS_RESOURCE).

IOW, some other process already has an access to process internals (and thus
it's already compromised), and can inject fork and use the child of the
compromised program to masquerade.
Which means this limitation doesn't solve the problem it was aimed to.

While removing this limitation allow to replace files from underneath of a
running process as many times as required. One of the use cases is network
file systems migration (NFS, to be precise) by CRIU.

NFS mount can't be mounted on restore stage because network is locked.
To overcome this limitation, another file system (FUSE-based) is used. Then
opened files replaced by the proper ones NFS is remounted.
Thus exe link replace has to be done twice: first on restore stage and second
- when actual NFS was remounted.

Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
---
 include/linux/sched.h |    4 +++-
 kernel/sys.c          |   10 ----------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 553af29..83b5f2d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -518,7 +518,9 @@ static inline int get_dumpable(struct mm_struct *mm)
 					/* leave room for more dump flags */
 #define MMF_VM_MERGEABLE	16	/* KSM may merge identical pages */
 #define MMF_VM_HUGEPAGE		17	/* set when VM_HUGEPAGE is set on vma */
-#define MMF_EXE_FILE_CHANGED	18	/* see prctl_set_mm_exe_file() */
+/* This ine-shot flag is droped due to necessivity of changing exe once again
+ * on NFS restore */
+//#define MMF_EXE_FILE_CHANGED	18	/* see prctl_set_mm_exe_file() */
 
 #define MMF_HAS_UPROBES		19	/* has uprobes */
 #define MMF_RECALC_UPROBES	20	/* MMF_HAS_UPROBES can be wrong */
diff --git a/kernel/sys.c b/kernel/sys.c
index 89d5be4..fd6f508 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1696,16 +1696,6 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
 		fput(exe_file);
 	}
 
-	/*
-	 * The symlink can be changed only once, just to disallow arbitrary
-	 * transitions malicious software might bring in. This means one
-	 * could make a snapshot over all processes running and monitor
-	 * /proc/pid/exe changes to notice unusual activity if needed.
-	 */
-	err = -EPERM;
-	if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags))
-		goto exit;
-
 	err = 0;
 	/* set the new file, lockless */
 	get_file(exe.file);

[toc] | [next] | [standalone]

#1441554

On 07/12, Stanislav Kinsburskiy wrote:
>
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1696,16 +1696,6 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  		fput(exe_file);
>  	}
>  
> -	/*
> -	 * The symlink can be changed only once, just to disallow arbitrary
> -	 * transitions malicious software might bring in. This means one
> -	 * could make a snapshot over all processes running and monitor
> -	 * /proc/pid/exe changes to notice unusual activity if needed.
> -	 */
> -	err = -EPERM;
> -	if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags))
> -		goto exit;
> -

I didn't even try to read the changelog so I do not know why do you
want this change ;)

But I would like to ack it in any case. I never understood why do we
want/need this MMF_EXE_FILE_CHANGED check, I suggested to remove it
many times.

And can't resist, please note the xchg() below. Currently (before this
patch) we do not need it. I was specially added to ensure that we can
just remove this test_and_set_bit(MMF_EXE_FILE_CHANGED) without adding
a race.

Acked-by: Oleg Nesterov <oleg@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1441598

On 07/12, Stanislav Kinsburskiy wrote:
>
> 12.07.2016 18:42, Oleg Nesterov пишет:
>> But I would like to ack it in any case. I never understood why do we
>> want/need this MMF_EXE_FILE_CHANGED check, I suggested to remove it
>> many times.
>>
>> And can't resist, please note the xchg() below. Currently (before this
>> patch) we do not need it. I was specially added to ensure that we can
>> just remove this test_and_set_bit(MMF_EXE_FILE_CHANGED) without adding
>> a race.
>
> Thanks, Oleg. I'll take a look.
> But should this be addressed in this patch? Especially if it's not
> needed even now (before this patch)?

Sorry for confusion...

Yes, it is not needed now (before this patch). Because only one caller
of prctl_set_mm_exe_file() can succced and update mm->exe_file. So we
could just do

	fput(mm->exe_file);
	mm->exe_file = get_file(exe.file);


But after this patch we do need this xchg(), otherwise 2 callers of
prctl_set_mm_exe_file() can race with each other. And this was the
actual reason for xchg: simplify the MMF_EXE_FILE_CHANGED removal in
future.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1441556

On Tue, Jul 12, 2016 at 07:30:29PM +0400, Stanislav Kinsburskiy wrote:
> This limitation came with the reason to remove "another
> way for malicious code to obscure a compromised program and
> masquerade as a benign process" by allowing "security-concious program can use
> this prctl once during its early initialization to ensure the prctl cannot
> later be abused for this purpose":
> 
> http://marc.info/?l=linux-kernel&m=133160684517468&w=2
> 
> But the way how the feature can be used is the following:
> 
> 1) Attach to process via ptrace (protected by CAP_SYS_PTRACE)
> 2) Unmap all the process file mappings, related to "exe" file.
> 3) Change exe link (protected by CAP_SYS_RESOURCE).
> 
> IOW, some other process already has an access to process internals (and thus
> it's already compromised), and can inject fork and use the child of the
> compromised program to masquerade.
> Which means this limitation doesn't solve the problem it was aimed to.
> 
> While removing this limitation allow to replace files from underneath of a
> running process as many times as required. One of the use cases is network
> file systems migration (NFS, to be precise) by CRIU.
> 
> NFS mount can't be mounted on restore stage because network is locked.
> To overcome this limitation, another file system (FUSE-based) is used. Then
> opened files replaced by the proper ones NFS is remounted.
> Thus exe link replace has to be done twice: first on restore stage and second
> - when actual NFS was remounted.
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>

Persistent exe-link doesn't guarantee anything if you have rights to ptrace
task and inject own code into (from security POV). So lets rip it out.

Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

[toc] | [prev] | [next] | [standalone]

#1441593

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Tue, Jul 12, 2016 at 07:30:29PM +0400, Stanislav Kinsburskiy wrote:
>> This limitation came with the reason to remove "another
>> way for malicious code to obscure a compromised program and
>> masquerade as a benign process" by allowing "security-concious program can use
>> this prctl once during its early initialization to ensure the prctl cannot
>> later be abused for this purpose":
>> 
>> http://marc.info/?l=linux-kernel&m=133160684517468&w=2
>> 
>> But the way how the feature can be used is the following:
>> 
>> 1) Attach to process via ptrace (protected by CAP_SYS_PTRACE)
>> 2) Unmap all the process file mappings, related to "exe" file.
>> 3) Change exe link (protected by CAP_SYS_RESOURCE).
>> 
>> IOW, some other process already has an access to process internals (and thus
>> it's already compromised), and can inject fork and use the child of the
>> compromised program to masquerade.
>> Which means this limitation doesn't solve the problem it was aimed to.
>> 
>> While removing this limitation allow to replace files from underneath of a
>> running process as many times as required. One of the use cases is network
>> file systems migration (NFS, to be precise) by CRIU.
>> 
>> NFS mount can't be mounted on restore stage because network is locked.
>> To overcome this limitation, another file system (FUSE-based) is used. Then
>> opened files replaced by the proper ones NFS is remounted.
>> Thus exe link replace has to be done twice: first on restore stage and second
>> - when actual NFS was remounted.
>> 
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>
> Persistent exe-link doesn't guarantee anything if you have rights to ptrace
> task and inject own code into (from security POV). So lets rip it out.
>
> Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

I believe the original concern was someone injecting a code into a
process and playing silly buggers with the exe link.  Someone who does
not have ptrace capability.

It is completely not ok to change this until someone goes back to the
original conversation and looks at the original threat model, and
refutes it.

Eric

[toc] | [prev] | [next] | [standalone]

#1441609

On Tue, Jul 12, 2016 at 11:52:09AM -0500, Eric W. Biederman wrote:
> >
> > Persistent exe-link doesn't guarantee anything if you have rights to ptrace
> > task and inject own code into (from security POV). So lets rip it out.
> >
> > Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
> 
> I believe the original concern was someone injecting a code into a
> process and playing silly buggers with the exe link.  Someone who does
> not have ptrace capability.

If you manage to inject code into a process, that's all, you're
compromised, preventing changing exe-link several times wont help
much I fear. Current limit -- one may change it once, Stas' patch
simply removes this limitation. The ability to change it only _once_
may be suitable for some kind of monitor daemon I guess but this
monitor should detect any change in exe-link state and notify
node's admin, otherwise it's simply useless.

> It is completely not ok to change this until someone goes back to the
> original conversation and looks at the original threat model, and
> refutes it.

[toc] | [prev] | [next] | [standalone]

#1441756

On Tue, Jul 12, 2016 at 08:29:33PM +0300, Cyrill Gorcunov wrote:
> On Tue, Jul 12, 2016 at 11:52:09AM -0500, Eric W. Biederman wrote:
> > >
> > > Persistent exe-link doesn't guarantee anything if you have rights to ptrace
> > > task and inject own code into (from security POV). So lets rip it out.
> > >
> > > Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
> > 
> > I believe the original concern was someone injecting a code into a
> > process and playing silly buggers with the exe link.  Someone who does
> > not have ptrace capability.
> 
> If you manage to inject code into a process, that's all, you're
> compromised, preventing changing exe-link several times wont help
> much I fear. Current limit -- one may change it once, Stas' patch
> simply removes this limitation. The ability to change it only _once_
> may be suitable for some kind of monitor daemon I guess but this
> monitor should detect any change in exe-link state and notify
> node's admin, otherwise it's simply useless.
> 
> > It is completely not ok to change this until someone goes back to the
> > original conversation and looks at the original threat model, and
> > refutes it.

Btw, if the persistency of exe link is _that_ important (in which I'm
really doubting) we always can use some of sysctl flag on host which
would control it [by default it might be turned off but for those
who really rely on exelink status, for some reason, the sysctl might
be set up and prevent any exelink modification].

[toc] | [prev] | [next] | [standalone]

#1445817

> >> 1) Attach to process via ptrace (protected by CAP_SYS_PTRACE)
> >> 2) Unmap all the process file mappings, related to "exe" file.
> >> 3) Change exe link (protected by CAP_SYS_RESOURCE).
> >> 
> >> IOW, some other process already has an access to process internals (and thus
> >> it's already compromised), and can inject fork and use the child of the
> >> compromised program to masquerade.
> >> Which means this limitation doesn't solve the problem it was aimed to.

IFF it is the same uid or root (in which case you already lost). In the
case of cross uid activity this is not true.

Alan

[toc] | [prev] | [next] | [standalone]

#1449734

Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> writes:

> Gentlemen,
>
> Looks like there are no objections to this patch.

There has been objection.

The only justification for the change that has been put forward is
someone doing a restore lazily.  I don't see a reason why you can't call
prctl_set_mm_exe_file until you have the file in place instead of a
place holder that sounds like a trivial solution to any restore issues.

The truth is an unlimited settable exe link is essentially meaningless,
as you can't depend on it for anything.  One shot seems the best
compromise I have seen put forward between the definite
checkpoint/restart requirement to set the this value and the general
need to have something that makes sense and people can depend on for
system management.

Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
validate that the new file is a actually mmaped executable.  We would
definitely need that to be fixed before even considering removing the
limit.

Right now all I see is people involved in the implementation details of
their own little feature

So for the patch I am responding to:
Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Plus the merge window is open so no one is taking any patches right now.
It is the time to take what has already been staged and get that code
merged.

Eric

[toc] | [prev] | [next] | [standalone]

#1449770

On Mon, Jul 25, 2016 at 01:21:51PM -0500, Eric W. Biederman wrote:
> Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> writes:
> 
> > Gentlemen,
> >
> > Looks like there are no objections to this patch.
> 
> There has been objection.
> 
> The only justification for the change that has been put forward is
> someone doing a restore lazily.  I don't see a reason why you can't call
> prctl_set_mm_exe_file until you have the file in place instead of a
> place holder that sounds like a trivial solution to any restore issues.
> 
> The truth is an unlimited settable exe link is essentially meaningless,
> as you can't depend on it for anything.  One shot seems the best
> compromise I have seen put forward between the definite
> checkpoint/restart requirement to set the this value and the general
> need to have something that makes sense and people can depend on for
> system management.
>
> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
> validate that the new file is a actually mmaped executable.  We would
> definitely need that to be fixed before even considering removing the
> limit.

Could you please elaborate? We check for inode being executable,
what else needed?

> Right now all I see is people involved in the implementation details of
> their own little feature
> 
> So for the patch I am responding to:
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> Plus the merge window is open so no one is taking any patches right now.
> It is the time to take what has already been staged and get that code
> merged.
> 
> Eric
> 

	Cyrill

[toc] | [prev] | [next] | [standalone]

#1449782

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Mon, Jul 25, 2016 at 01:21:51PM -0500, Eric W. Biederman wrote:
>> Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> writes:
>> 
>> > Gentlemen,
>> >
>> > Looks like there are no objections to this patch.
>> 
>> There has been objection.
>> 
>> The only justification for the change that has been put forward is
>> someone doing a restore lazily.  I don't see a reason why you can't call
>> prctl_set_mm_exe_file until you have the file in place instead of a
>> place holder that sounds like a trivial solution to any restore issues.
>> 
>> The truth is an unlimited settable exe link is essentially meaningless,
>> as you can't depend on it for anything.  One shot seems the best
>> compromise I have seen put forward between the definite
>> checkpoint/restart requirement to set the this value and the general
>> need to have something that makes sense and people can depend on for
>> system management.
>>
>> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
>> validate that the new file is a actually mmaped executable.  We would
>> definitely need that to be fixed before even considering removing the
>> limit.
>
> Could you please elaborate? We check for inode being executable,
> what else needed?

That the inode is mmaped into the process with executable mappings.

Effectively what we check the old mapping for and refuse to remove the old
mm_exe_file if it exists.

I think a reasonable argument can be made that if the file is
executable, and it is mmaped with executable pages that exe_file is not
a complete lie.

Which is the important part.  At the end of the day how much can
userspace trust /proc/pid/exe?  If we are too lax it is just a random
file descriptor we can not trust at all.  At which point there is
exactly no point in preserving it in checkpoint/restart, because nothing
will trust or look at it.

If the only user is checkpoint/restart perhaps it should be only ptrace
that can set this and not the process itself with a prctl.  I don't
know.  All I know is that we should work on making it a very trustable
value even though in some specific instances we can set it.

Eric

[toc] | [prev] | [next] | [standalone]

#1450475

On Mon, Jul 25, 2016 at 02:56:43PM -0500, Eric W. Biederman wrote:
...
> >>
> >> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
> >> validate that the new file is a actually mmaped executable.  We would
> >> definitely need that to be fixed before even considering removing the
> >> limit.
> >
> > Could you please elaborate? We check for inode being executable,
> > what else needed?
> 
> That the inode is mmaped into the process with executable mappings.
> 
> Effectively what we check the old mapping for and refuse to remove the old
> mm_exe_file if it exists.
> 
> I think a reasonable argument can be made that if the file is
> executable, and it is mmaped with executable pages that exe_file is not
> a complete lie.

I might be missing something obvious, so sorry for the question --
when criu setups old exe link the inode we obtain from file open
is not mapped into memory, the old exe not read by anyone because
it's not even executed anyhow. So I don't really understand which
mapping we should check here. Mind to point me?

> Which is the important part.  At the end of the day how much can
> userspace trust /proc/pid/exe?  If we are too lax it is just a random
> file descriptor we can not trust at all.  At which point there is
> exactly no point in preserving it in checkpoint/restart, because nothing
> will trust or look at it.

You know, I think we should not trust exe link much, and in real we
never could: this link is rather a hint pointing which executable a
process has been using on execve call, once the process start working
one can't be sure if the code currently running is exactly from the
file pointed by exe link. It just a hint suitable for debuggin and
obtain clean view of which processes are running on noncompromised
system. Monitoring exe link change won't help much if there are
malicious software running on the system.

> If the only user is checkpoint/restart perhaps it should be only ptrace
> that can set this and not the process itself with a prctl.  I don't
> know.  All I know is that we should work on making it a very trustable
> value even though in some specific instances we can set it.

Since as I said I suppose nobody except us using this feature, we can
setup some sysctl trigger for it (I personally think this is an
overkill, but OTOH if people rely on the exe link and not going
to use criu at all, this trigger will help).

	Cyrill

[toc] | [prev] | [next] | [standalone]

#1452733

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Mon, Jul 25, 2016 at 02:56:43PM -0500, Eric W. Biederman wrote:
> ...
>> >>
>> >> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
>> >> validate that the new file is a actually mmaped executable.  We would
>> >> definitely need that to be fixed before even considering removing the
>> >> limit.
>> >
>> > Could you please elaborate? We check for inode being executable,
>> > what else needed?
>> 
>> That the inode is mmaped into the process with executable mappings.
>> 
>> Effectively what we check the old mapping for and refuse to remove the old
>> mm_exe_file if it exists.
>> 
>> I think a reasonable argument can be made that if the file is
>> executable, and it is mmaped with executable pages that exe_file is not
>> a complete lie.
>
> I might be missing something obvious, so sorry for the question --
> when criu setups old exe link the inode we obtain from file open
> is not mapped into memory, the old exe not read by anyone because
> it's not even executed anyhow. So I don't really understand which
> mapping we should check here. Mind to point me?

That sounds like an out and out bug that should not be preserved.
Of course we should mmap the executable and set it up so that it can be
executed (at least as much as the executable was previously mapped).
Anything else is a buggy restart, and lying to userspace.

>> Which is the important part.  At the end of the day how much can
>> userspace trust /proc/pid/exe?  If we are too lax it is just a random
>> file descriptor we can not trust at all.  At which point there is
>> exactly no point in preserving it in checkpoint/restart, because nothing
>> will trust or look at it.
>
> You know, I think we should not trust exe link much, and in real we
> never could: this link is rather a hint pointing which executable a
> process has been using on execve call, once the process start working
> one can't be sure if the code currently running is exactly from the
> file pointed by exe link. It just a hint suitable for debuggin and
> obtain clean view of which processes are running on noncompromised
> system. Monitoring exe link change won't help much if there are
> malicious software running on the system.

But it is not just a hint.  It is a record of which executable we called
execve on.  Knowing which file was executed doesn't guarantee what is
running now but it provides a very strong hint.

At then end of a restart the state of a process should be (by
definition) exactly the state the process was before a checkpoint
and thus a state the original executable could have gotten into.

I admit it is possible for an application to unmap itself.  I honestly
have not met that application (except perhaps criu).

>> If the only user is checkpoint/restart perhaps it should be only ptrace
>> that can set this and not the process itself with a prctl.  I don't
>> know.  All I know is that we should work on making it a very trustable
>> value even though in some specific instances we can set it.
>
> Since as I said I suppose nobody except us using this feature, we can
> setup some sysctl trigger for it (I personally think this is an
> overkill, but OTOH if people rely on the exe link and not going
> to use criu at all, this trigger will help).

Some clarity of thought came to me, and I apologise for not replying
sooner with it sooner.

My problem with the original patch submission is that it was
justifying changing prctl_set_mm_exe_file based on what
prctl_set_mm_exe_file does today.  As prctl_set_mm_exe_file was added
for the checkpoint/restart case that is justifying changing code based
on a buggy implementation.

It is necessary to look at the ordinary situation.  Without
prctl_set_mm_exe /proc/[pid]/exe can be counted on as a record
of which executable was last passed to execve.  Furthermore the state of
a process can be counted on to be a state reachable from calling
execve on /proc/[pid]/exe.

Which means to preserve those expectations prctl_set_mm_exe_file should
in practice just be a nicer less cumbersome interface to things you can
already achieve with execve.

Justifying removale of the one-short nature for prctl_set_mm_exe_file
is as straight forward as noting that a process can call execve on
any executable file.

However when I compare the invariants that execve has on a file (such as
the executable being mmaped) I see some noticable disparities between
what prctl_set_mm_exe_file allows and what execve allows.  With
prctl_set_mm_exe being less strict.

So what I am requesting is very simple.  That the checks in
prctl_set_mm_exe_file be tightened up to more closely approach what
execve requires.  Thus preserving the value of the /proc/[pid]/exe for
the applications that want to use the exe link.

Once the checks in prctl_set_mm_exe_file are tightened up please feel
free to remove the one shot test.

Eric

[toc] | [prev] | [next] | [standalone]

#1452744

On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
> So what I am requesting is very simple.  That the checks in
> prctl_set_mm_exe_file be tightened up to more closely approach what
> execve requires.  Thus preserving the value of the /proc/[pid]/exe for
> the applications that want to use the exe link.
> 
> Once the checks in prctl_set_mm_exe_file are tightened up please feel
> free to remove the one shot test.
> 

This is more fishy.

First of all exe_file is used by the audit subsystem. So someone has to
ask audit people what is the significance (if any) of the field.

All exe_file users but one use get_mm_exe_file and handle NULL
gracefully.

Even with the current limit of changing the field once, the user can
cause a transient failure of get_mm_exe_file which can fail to increment
the refcount before it drops to 0.

This transient failure can be used to get a NULL value stored in
->exe_file during fork (in dup_mmap):
RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm));

The one place which is not using get_mm_exe_file to get to the pointer
is audit_exe_compare:
        rcu_read_lock();
        exe_file = rcu_dereference(tsk->mm->exe_file);
        ino = exe_file->f_inode->i_ino;
        dev = exe_file->f_inode->i_sb->s_dev;
        rcu_read_unlock();

This is buggy on 2 accounts:
1. exe_file can be NULL
2. rcu does not protect f_inode

The issue is made worse with allowing arbitrary number changes.

Modifying get_mm_exe_file to retry is trivial and in effect never return
NULL is trivial. With arbitrary number of changes allowed this may
require some cond_resched() or something.

For comments I cc'ed Richard Guy Briggs, who is both an audit person and
the author of audit_exe_compare.
-- 
Mateusz Guzik

[toc] | [prev] | [next] | [standalone]

#1452863

Mateusz Guzik <mguzik@redhat.com> writes:

> On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
>> So what I am requesting is very simple.  That the checks in
>> prctl_set_mm_exe_file be tightened up to more closely approach what
>> execve requires.  Thus preserving the value of the /proc/[pid]/exe for
>> the applications that want to use the exe link.
>> 
>> Once the checks in prctl_set_mm_exe_file are tightened up please feel
>> free to remove the one shot test.
>> 
>
> This is more fishy.
>
> First of all exe_file is used by the audit subsystem. So someone has to
> ask audit people what is the significance (if any) of the field.
>
> All exe_file users but one use get_mm_exe_file and handle NULL
> gracefully.
>
> Even with the current limit of changing the field once, the user can
> cause a transient failure of get_mm_exe_file which can fail to increment
> the refcount before it drops to 0.
>
> This transient failure can be used to get a NULL value stored in
> ->exe_file during fork (in dup_mmap):
> RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm));
>
> The one place which is not using get_mm_exe_file to get to the pointer
> is audit_exe_compare:
>         rcu_read_lock();
>         exe_file = rcu_dereference(tsk->mm->exe_file);
>         ino = exe_file->f_inode->i_ino;
>         dev = exe_file->f_inode->i_sb->s_dev;
>         rcu_read_unlock();
>
> This is buggy on 2 accounts:
> 1. exe_file can be NULL
> 2. rcu does not protect f_inode
>
> The issue is made worse with allowing arbitrary number changes.
>
> Modifying get_mm_exe_file to retry is trivial and in effect never return
> NULL is trivial. With arbitrary number of changes allowed this may
> require some cond_resched() or something.
>
> For comments I cc'ed Richard Guy Briggs, who is both an audit person and
> the author of audit_exe_compare.

That is fair.  Keeping the existing users working is what needs to
happen.

At the same time we have an arbitrary number of possible changes with
exec, but I guess that works differently because the mm is changed as
well.

So yes let's bug fix this piece of code and then we can see about
relaxing constraints.

Eric

[toc] | [prev] | [next] | [standalone]

#1452904

On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
> Cyrill Gorcunov <gorcunov@gmail.com> writes:
> 
> > On Mon, Jul 25, 2016 at 02:56:43PM -0500, Eric W. Biederman wrote:
> > ...
> >> >>
> >> >> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
> >> >> validate that the new file is a actually mmaped executable.  We would
> >> >> definitely need that to be fixed before even considering removing the
> >> >> limit.
> >> >
> >> > Could you please elaborate? We check for inode being executable,
> >> > what else needed?
> >> 
> >> That the inode is mmaped into the process with executable mappings.

Eric, thanks for clarification. Let me talk from CRIU perspective (because
the interface came from its need) -- the former executable may no longer
exist, completely: for such cases in CRIU we simply create that named
"ghost" files wich are just literally removed upon open. So we simply
can't mmap the former executable into memory.

Moreover I would really _like_ to not do this check -- the former
intarface has been done exactly to behave as it does now: don't
read original file into memory (in criu, when we setup this exelink,
the original memory of a process already restored so additional
mmap for every executable is purely waste of time).

> >> 
> >> Effectively what we check the old mapping for and refuse to remove the old
> >> mm_exe_file if it exists.
> >> 
> >> I think a reasonable argument can be made that if the file is
> >> executable, and it is mmaped with executable pages that exe_file is not
> >> a complete lie.
> >
> > I might be missing something obvious, so sorry for the question --
> > when criu setups old exe link the inode we obtain from file open
> > is not mapped into memory, the old exe not read by anyone because
> > it's not even executed anyhow. So I don't really understand which
> > mapping we should check here. Mind to point me?
> 
> That sounds like an out and out bug that should not be preserved.

No, it's done on intent, as I explained above -- we would like to
escape double reading (which not always possible in case of deleted
files).

> Of course we should mmap the executable and set it up so that it can be
> executed (at least as much as the executable was previously mapped).
> Anything else is a buggy restart, and lying to userspace.

Same way once someone ptraces the process it makes exelink to lie
into userspace. exelink is valid only small time moment: when
kernel reads elf and maps it. After that, once you jump back into
userspace that's end of game, there might be anything else running
instead of former executable as we already know.

> 
> >> Which is the important part.  At the end of the day how much can
> >> userspace trust /proc/pid/exe?  If we are too lax it is just a random
> >> file descriptor we can not trust at all.  At which point there is
> >> exactly no point in preserving it in checkpoint/restart, because nothing
> >> will trust or look at it.
> >
> > You know, I think we should not trust exe link much, and in real we
> > never could: this link is rather a hint pointing which executable a
> > process has been using on execve call, once the process start working
> > one can't be sure if the code currently running is exactly from the
> > file pointed by exe link. It just a hint suitable for debuggin and
> > obtain clean view of which processes are running on noncompromised
> > system. Monitoring exe link change won't help much if there are
> > malicious software running on the system.
> 
> But it is not just a hint.  It is a record of which executable we called
> execve on.  Knowing which file was executed doesn't guarantee what is
> running now but it provides a very strong hint.

Exactly, its a record of what been valid when kernel did execve
and run new content. Once we're in userspace back this data may
be informative but not representative.

> At then end of a restart the state of a process should be (by
> definition) exactly the state the process was before a checkpoint
> and thus a state the original executable could have gotten into.
> 
> I admit it is possible for an application to unmap itself.  I honestly
> have not met that application (except perhaps criu).

It's not common practice on modern machines indeed. But you can do that,
you can even continue running when former executable no longer present
on the disk.

> >> If the only user is checkpoint/restart perhaps it should be only ptrace
> >> that can set this and not the process itself with a prctl.  I don't
> >> know.  All I know is that we should work on making it a very trustable
> >> value even though in some specific instances we can set it.
> >
> > Since as I said I suppose nobody except us using this feature, we can
> > setup some sysctl trigger for it (I personally think this is an
> > overkill, but OTOH if people rely on the exe link and not going
> > to use criu at all, this trigger will help).
> 
> Some clarity of thought came to me, and I apologise for not replying
> sooner with it sooner.
> 
> My problem with the original patch submission is that it was
> justifying changing prctl_set_mm_exe_file based on what
> prctl_set_mm_exe_file does today.  As prctl_set_mm_exe_file was added
> for the checkpoint/restart case that is justifying changing code based
> on a buggy implementation.

I dont think so: it's not buggy but the minimum we need to be able
to restore deleted files. I'll read the rest of the email tomorrow,
thank you for comments!

[toc] | [prev] | [next] | [standalone]

#1452906

On Sun, Jul 31, 2016 at 3:43 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
>> Cyrill Gorcunov <gorcunov@gmail.com> writes:
>>
>> > On Mon, Jul 25, 2016 at 02:56:43PM -0500, Eric W. Biederman wrote:
>> > ...
>> >> >>
>> >> >> Also there is a big fat bug in prctl_set_mm_exe_file.  It doesn't
>> >> >> validate that the new file is a actually mmaped executable.  We would
>> >> >> definitely need that to be fixed before even considering removing the
>> >> >> limit.
>> >> >
>> >> > Could you please elaborate? We check for inode being executable,
>> >> > what else needed?
>> >>
>> >> That the inode is mmaped into the process with executable mappings.
>
> Eric, thanks for clarification. Let me talk from CRIU perspective (because
> the interface came from its need) -- the former executable may no longer
> exist, completely: for such cases in CRIU we simply create that named
> "ghost" files wich are just literally removed upon open. So we simply
> can't mmap the former executable into memory.
>
> Moreover I would really _like_ to not do this check -- the former
> intarface has been done exactly to behave as it does now: don't
> read original file into memory (in criu, when we setup this exelink,
> the original memory of a process already restored so additional
> mmap for every executable is purely waste of time).

Eric, I think I generally disagree with you here.  I see no compelling
reason that we shouldn't allow unlimited changes to exe_file so long
as the selected files are executable.  (It might make sense to check
that LSM is okay, but even that is a dubious requirement, I think.)
What kind of attack are you worried about?

That being said, we should probably make it so that audit logs
indicate the real executable that was execve'd, but I'd imagine that
this is already the case.  But I don't think the original executable
needs to be directly visible in /proc.

--Andy

[toc] | [prev] | [next] | [standalone]

#1453060

On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
...
> 
> It is necessary to look at the ordinary situation.  Without
> prctl_set_mm_exe /proc/[pid]/exe can be counted on as a record
> of which executable was last passed to execve.

True.

> Furthermore the state of a process can be counted on to be a state
> reachable from calling execve on /proc/[pid]/exe.

Absolutely not. The state is valid until kernel jumped back to userspace
and give control to an interpretator.

> Which means to preserve those expectations prctl_set_mm_exe_file should
> in practice just be a nicer less cumbersome interface to things you can
> already achieve with execve.
> 
> Justifying removale of the one-short nature for prctl_set_mm_exe_file
> is as straight forward as noting that a process can call execve on
> any executable file.
> 
> However when I compare the invariants that execve has on a file (such as
> the executable being mmaped) I see some noticable disparities between
> what prctl_set_mm_exe_file allows and what execve allows.  With
> prctl_set_mm_exe being less strict.
> 
> So what I am requesting is very simple.  That the checks in
> prctl_set_mm_exe_file be tightened up to more closely approach what
> execve requires.  Thus preserving the value of the /proc/[pid]/exe for
> the applications that want to use the exe link.
> 
> Once the checks in prctl_set_mm_exe_file are tightened up please feel
> free to remove the one shot test.

Thanks a huge for the detailed explanation, but i don't agree here because
assuming that state of a process reachable from calling execve on
/proc/[pid]/exe is not always true.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web