New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix delete other file bug when container id is .. #1883

Merged
merged 3 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@lifubang
Contributor

lifubang commented Aug 31, 2018

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

There is a big bug which can delete files not belonging to the container when use runc delete --force.
Command history is:

/opt/runc/runcroot is ruc's root dir.

root@dockerdemo:/opt/runc# ls -lh
total 16K
drwxr-xr-x 2 root root 4.0K Aug 31 11:19 dbback
drwx------ 2 root root 4.0K Aug 31 10:49 runcroot
-rw-r--r-- 1 root root    5 Aug 31 11:19 test
-rw-r--r-- 1 root root    6 Aug 31 11:19 test1

While dbback dir have very important db backup files.

run two container:

root@dockerdemo:/opt/runctest/redis# runc --root /opt/runc/runcroot run -d redis
root@dockerdemo:/opt/runctest/redis# 6:C 31 Aug 03:24:24.741 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo

root@dockerdemo:/opt/runctest/redis# runc --root /opt/runc/runcroot run -d ..0
root@dockerdemo:/opt/runctest/redis# 6:C 31 Aug 03:24:33.669 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo

root@dockerdemo:/opt/runctest/redis# runc --root /opt/runc/runcroot list
ID          PID         STATUS      BUNDLE                CREATED                          OWNER
..0         4170        running     /opt/runctest/redis   2018-08-31T03:24:33.664173003Z   root
redis       4121        running     /opt/runctest/redis   2018-08-31T03:24:24.73616634Z    root
root@dockerdemo:/opt/runctest/redis#

backup a state.json to /opt/runc

root@dockerdemo:/opt/runctest/redis# runc --root /opt/runc/runcroot list
ID          PID         STATUS      BUNDLE                CREATED                          OWNER
..0         4170        running     /opt/runctest/redis   2018-08-31T03:24:33.664173003Z   root
redis       4121        running     /opt/runctest/redis   2018-08-31T03:24:24.73616634Z    root
root@dockerdemo:/opt/runctest/redis# cp /opt/runc/runcroot/redis/state.json /opt/runc/
root@dockerdemo:/opt/runctest/redis# ls /opt/runc
dbback  runcroot  state.json  test  test1
root@dockerdemo:/opt/runctest/redis#

When I want to delete ..0, but I typed a wrong word: ..0 -> .. lost a 0

root@dockerdemo:/opt/runctest/redis# runc --root /opt/runc/runcroot delete --force ..
root@dockerdemo:/opt/runctest/redis# runc --root /opt/runc/runcroot list
ID          PID         STATUS      BUNDLE      CREATED     OWNER
root@dockerdemo:/opt/runctest/redis# ls /opt/runc
runcroot
root@dockerdemo:/opt/runctest/redis# ls /opt/runc/runcroot/
root@dockerdemo:/opt/runctest/redis#

Everything is deleted, include my dbback dir.

This is because there is no strict id validate method in Container Load function in "libcontainer/factory_linux.go", especially when delete.

Please check it, thanks.

fix unexpected delete bug when container id is ..
Signed-off-by: Lifubang <lifubang@acmcoder.com>

@lifubang lifubang changed the title from fix unexpected delete bug when container id is .. to fix delete other file bug when container id is .. Aug 31, 2018

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 3, 2018

Member

Jesus. Yeah, that's a very bad bug.

I'm not a fan of this patch (it looks a bit overly complicated -- filepath.* validation is quite hard to get right). I would recommend doing something like checking whether utils.CleanPath(containerid) == containerid and erroring out if that's not the case.

However there are more places that this needs to be fixed -- most of libcontainer/factory_linux.go appears to use filepath.Join in an unsafe way.

Member

cyphar commented Sep 3, 2018

Jesus. Yeah, that's a very bad bug.

I'm not a fan of this patch (it looks a bit overly complicated -- filepath.* validation is quite hard to get right). I would recommend doing something like checking whether utils.CleanPath(containerid) == containerid and erroring out if that's not the case.

However there are more places that this needs to be fixed -- most of libcontainer/factory_linux.go appears to use filepath.Join in an unsafe way.

@lifubang

This comment has been minimized.

Show comment
Hide comment
@lifubang

lifubang Sep 3, 2018

Contributor

@cyphar Thanks for your reply.
utils.CleanPath(containerid) is still unsafe for containerid. For example:
runc --root /opt/runc/runcroot delete --force .

Because utils.CleanPath(".") = "."

So, I said:
//For other unforeseen invalid id situations, can checked by is SubDir?
Because we don't know what is other unforeseen invalid id situations.

Contributor

lifubang commented Sep 3, 2018

@cyphar Thanks for your reply.
utils.CleanPath(containerid) is still unsafe for containerid. For example:
runc --root /opt/runc/runcroot delete --force .

Because utils.CleanPath(".") = "."

So, I said:
//For other unforeseen invalid id situations, can checked by is SubDir?
Because we don't know what is other unforeseen invalid id situations.

code optimization after review
Signed-off-by: Lifubang <lifubang@acmcoder.com>
@lifubang

This comment has been minimized.

Show comment
Hide comment
@lifubang

lifubang Sep 3, 2018

Contributor

@cyphar There are 3 filepath.join in libcontainer/factory_linux.go

  1. https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L198 Because there is validateID before filepath.join, so it is safe.
  2. https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L232 There is no validateID before filepath.join, so I add it.
  3. https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L342 Because stateFilename is a const, so it is safe.
    Please check it. Thanks.
Contributor

lifubang commented Sep 3, 2018

@cyphar There are 3 filepath.join in libcontainer/factory_linux.go

  1. https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L198 Because there is validateID before filepath.join, so it is safe.
  2. https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L232 There is no validateID before filepath.join, so I add it.
  3. https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L342 Because stateFilename is a const, so it is safe.
    Please check it. Thanks.
@lifubang

@cyphar @crosbymichael I think this bug needs to be fixed as soon as possible. Because runc is installed as docker-runc with docker. If I run docker-runc --root /run/docker/runtime-runc/moby delete --force .., the docker will be crashed, and the host need to restart.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Sep 3, 2018

Member

@lifubang

You could fix that by doing

string(os.Separator)+containerId == utils.CleanPath(string(os.Separator)+containerid)

My reason for wanting to do it this way is that it makes it clearer that we are making sure that the name doesn't contain any special lexical path tokens. However, since container IDs must match a regular expression that doesn't include / we could make it even easier and just disallow container IDs that are equal to . or .. (without any filepath checks). We could even add that to the regular expression if we wanted to.

Because we don't know what is other unforeseen invalid id situations.

But we do know what type of issue they would be caused by -- having filesystem lexical characters in the name. Another possible attack is having a symlink in the state directory (in which case we'd need to use securejoin.SecureJoin but I imagine that this is not a useful attack since you need root to get runc to mess with things as root).

Member

cyphar commented Sep 3, 2018

@lifubang

You could fix that by doing

string(os.Separator)+containerId == utils.CleanPath(string(os.Separator)+containerid)

My reason for wanting to do it this way is that it makes it clearer that we are making sure that the name doesn't contain any special lexical path tokens. However, since container IDs must match a regular expression that doesn't include / we could make it even easier and just disallow container IDs that are equal to . or .. (without any filepath checks). We could even add that to the regular expression if we wanted to.

Because we don't know what is other unforeseen invalid id situations.

But we do know what type of issue they would be caused by -- having filesystem lexical characters in the name. Another possible attack is having a symlink in the state directory (in which case we'd need to use securejoin.SecureJoin but I imagine that this is not a useful attack since you need root to get runc to mess with things as root).

code optimization: use securejoin.SecureJoin and CleanPath
Signed-off-by: Lifubang <lifubang@acmcoder.com>
@lifubang

Thanks for your review. I have finished.

@crosbymichael crosbymichael added this to the 1.0.0 milestone Sep 4, 2018

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael
Member

crosbymichael commented Sep 4, 2018

LGTM

Approved with PullApprove

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Sep 5, 2018

Contributor

LGTM

Approved with PullApprove

Contributor

mrunalp commented Sep 5, 2018

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 70ca035 into opencontainers:master Sep 5, 2018

2 checks passed

code-review/pullapprove Approved by crosbymichael, mrunalp
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment