fixes: pid reuse attack when kill a exec process by lifubang · Pull Request #2832 · containerd/containerd

lifubang · 2018-11-26T01:33:04Z

Because of we use unix.Kill to kill a exec process, but after a exec process has been stopped, the pid in execProcess has not been deleted, we also can use execProcess.Kill to kill the process with the same pid.

use unix.Kill https://github.com/lifubang/containerd/blob/831a41b9585fc021c1036da17afa1513e7b4f908/runtime/v1/linux/proc/exec.go#L145-L153

don't delete pid in execProcess https://github.com/lifubang/containerd/blob/831a41b9585fc021c1036da17afa1513e7b4f908/runtime/v1/linux/proc/exec.go#L96-L101

can recall execProcess.Kill https://github.com/lifubang/containerd/blob/831a41b9585fc021c1036da17afa1513e7b4f908/runtime/v1/linux/proc/exec_state.go#L153-L155

So, if the pid is reused, the wrong process will be killed.

Signed-off-by: Lifubang lifubang@acmcoder.com

codecov-io · 2018-11-26T08:10:34Z

Codecov Report

Merging #2832 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2832   +/-   ##
=======================================
  Coverage   43.79%   43.79%           
=======================================
  Files         100      100           
  Lines       10741    10741           
=======================================
  Hits         4704     4704           
  Misses       5307     5307           
  Partials      730      730

Flag	Coverage Δ
#linux	`47.45% <ø> (ø)`	⬆️
#windows	`40.96% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75c490c...f31c097. Read the comment docs.

thaJeztah · 2018-11-27T23:44:18Z

I see you opened a backport for the release/1.1 branch; I guess this is also needed for the 1.2 branch (and possibly 1.0 branch?) (looking at https://github.com/containerd/containerd/blob/master/RELEASES.md#support-horizon, 1.0 is still supported)

Random-Liu

This can still happen:

exec kill pid;
pid got reused;
exec kill pid;
containerd-shim get the exit event, and setExited.

Do we really need to consider this case? Who are we trying to protect from?

Random-Liu · 2018-11-28T00:29:30Z

runtime/v1/linux/proc/exec.go

Yes, a not only small, but also big error. I'll push again.

lifubang · 2018-11-28T01:11:35Z

Do we really need to consider this case? Who are we trying to protect from?

I think this can protect some one who just use lib containerd in their program. And may protect moby when the event queue is blocked or slow for some reasons, because docker stop will call p.Kill first.

This can still happen:

exec kill pid;
pid got reused;
exec kill pid;
containerd-shim get the exit event, and setExited.

I think the pid reuse attack may happen needs a long time, in a short time it have very very small probability. If we want to fix this situation, we can use ProcessStartTime check like runc.

lifubang · 2018-11-28T01:13:15Z

I see you opened a backport for the release/1.1 branch; I guess this is also needed for the 1.2 branch (and possibly 1.0 branch?) (looking at https://github.com/containerd/containerd/blob/master/RELEASES.md#support-horizon, 1.0 is still supported)

Thanks, I'll open PR for these versions if this patch is taken by maintainers.

thaJeztah · 2018-11-28T01:26:40Z

Thanks, I'll open PR for these versions if this patch is taken by maintainers

Perfect, thanks!

lifubang · 2018-11-28T02:34:43Z

@Random-Liu in here https://github.com/containerd/containerd/blob/96eb3bfa5dda628678ec2edc6d6c0b99e0fb119d/runtime/v1/linux/proc/exec.go#L254-L257 , if the pid is reused, the process can't delete anymore.

Signed-off-by: Lifubang <lifubang@acmcoder.com>

lifubang · 2018-11-28T03:22:30Z

Because task.Delete also need check status of initProcess, so we need set pid to -1 when SetExited, please see:

containerd/task.go

Lines 265 to 288 in f489a95

    
           // Delete deletes the task and its runtime state 
        
           // it returns the exit status of the task and any errors that were encountered 
        
           // during cleanup 
        
           func (t *task) Delete(ctx context.Context, opts ...ProcessDeleteOpts) (*ExitStatus, error) { 
        
           	for _, o := range opts { 
        
           		if err := o(ctx, t); err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	status, err := t.Status(ctx) 
        
           	if err != nil && errdefs.IsNotFound(err) { 
        
           		return nil, err 
        
           	} 
        
           	switch status.Status { 
        
           	case Stopped, Unknown, "": 
        
           	case Created: 
        
           		if t.client.runtime == fmt.Sprintf("%s.%s", plugin.RuntimePlugin, "windows") { 
        
           			// On windows Created is akin to Stopped 
        
           			break 
        
           		} 
        
           		fallthrough 
        
           	default: 
        
           		return nil, errors.Wrapf(errdefs.ErrFailedPrecondition, "task must be stopped before deletion: %s", status.Status) 
        
           	}

containerd/services/tasks/local.go

Lines 297 to 303 in f489a95

    
           func (l *local) Get(ctx context.Context, r *api.GetRequest, _ ...grpc.CallOption) (*api.GetResponse, error) { 
        
           	task, err := l.getTask(ctx, r.ContainerID) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	p := runtime.Process(task) 
        
           	if r.ExecID != "" {

containerd/runtime/v1/linux/task.go

Lines 304 to 309 in f489a95

    
           func (t *Task) Process(ctx context.Context, id string) (runtime.Process, error) { 
        
           	p := &Process{ 
        
           		id: id, 
        
           		t:  t, 
        
           	} 
        
           	if _, err := p.State(ctx); err != nil {

Closes containerd#2832 Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Closes containerd#2832 Signed-off-by: Michael Crosby <crosbymichael@gmail.com> (cherry picked from commit 719a2c5) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Closes containerd#2832 Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

lifubang mentioned this pull request Nov 26, 2018

fixes: pid reuse attack when kill a exec process (release/1.1) #2833

Merged

lifubang changed the title ~~fixex: pid reuse attack when kill a exec process~~ fixes: pid reuse attack when kill a exec process Nov 26, 2018

lifubang force-pushed the pidreuseattack branch from 4359386 to f31c097 Compare November 26, 2018 07:53

Random-Liu reviewed Nov 28, 2018

View reviewed changes

lifubang force-pushed the pidreuseattack branch from f31c097 to 96eb3bf Compare November 28, 2018 00:59

lifubang force-pushed the pidreuseattack branch from 96eb3bf to f489a95 Compare November 28, 2018 03:10

fix pid reuse attack when kill a exec process

f489a95

Signed-off-by: Lifubang <lifubang@acmcoder.com>

crosbymichael mentioned this pull request Jun 21, 2019

Robust pid locking for shim processes #3366

Merged

crosbymichael added a commit to crosbymichael/containerd that referenced this pull request Jun 26, 2019

Robust pid locking for shim processes

719a2c5

Closes containerd#2832 Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

dmcgowan closed this in #3366 Jun 29, 2019

thaJeztah mentioned this pull request Oct 14, 2019

[release/1.2 backport] backport exec fixes #3755

Merged

tussennet pushed a commit to tussennet/containerd that referenced this pull request Sep 11, 2020

Robust pid locking for shim processes

10e3e9c

Closes containerd#2832 Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes: pid reuse attack when kill a exec process#2832

fixes: pid reuse attack when kill a exec process#2832
lifubang wants to merge 1 commit intocontainerd:masterfrom
lifubang:pidreuseattack

lifubang commented Nov 26, 2018 •

edited

Loading

Uh oh!

codecov-io commented Nov 26, 2018

Uh oh!

thaJeztah commented Nov 27, 2018

Uh oh!

Random-Liu left a comment

Uh oh!

Random-Liu Nov 28, 2018

Uh oh!

lifubang Nov 28, 2018 •

edited

Loading

Uh oh!

lifubang commented Nov 28, 2018

Uh oh!

lifubang commented Nov 28, 2018

Uh oh!

thaJeztah commented Nov 28, 2018

Uh oh!

lifubang commented Nov 28, 2018 •

edited

Loading

Uh oh!

lifubang commented Nov 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lifubang commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 26, 2018

Codecov Report

Uh oh!

thaJeztah commented Nov 27, 2018

Uh oh!

Random-Liu left a comment

Choose a reason for hiding this comment

Uh oh!

Random-Liu Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

lifubang Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifubang commented Nov 28, 2018

Uh oh!

lifubang commented Nov 28, 2018

Uh oh!

thaJeztah commented Nov 28, 2018

Uh oh!

lifubang commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented Nov 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lifubang commented Nov 26, 2018 •

edited

Loading

lifubang Nov 28, 2018 •

edited

Loading

lifubang commented Nov 28, 2018 •

edited

Loading