Skip to content
This repository has been archived by the owner on Feb 8, 2021. It is now read-only.

Security: fix a issue (similar to runc CVE-2016-3697) #348

Merged
merged 1 commit into from Jan 8, 2018
Merged

Security: fix a issue (similar to runc CVE-2016-3697) #348

merged 1 commit into from Jan 8, 2018

Conversation

keloyang
Copy link
Contributor

@keloyang keloyang commented Jan 5, 2018

It's a issue that is similar to runc CVE-2016-3697.

Before this pr,

[root@localhost ~]# docker run --rm -ti ... 797fd343ef02 bash
[root@167b67fea278 /]# echo "10:x:0:0:root:/root:/bin/bash">/etc/passwd
[root@167b67fea278 /]# cat /etc/passwd
10:x:0:0:root:/root:/bin/bash

another console,
[root@localhost ~]# docker exec -ti -u 10 hello  bash
bash: /etc/profile.d/colorgrep.sh: Interrupted system call
[10@167b67fea278 /]# id
uid=0(10) gid=0(root) groups=0(root)

After this pr,

[root@localhost ~]# docker run --rm -ti ... 797fd343ef02 bash
[root@167b67fea278 /]# echo "10:x:0:0:root:/root:/bin/bash">/etc/passwd
[root@167b67fea278 /]# cat /etc/passwd
10:x:0:0:root:/root:/bin/bash

another console,
[root@localhost ~]# docker exec -ti -u 10 hello  bash
bash: /etc/profile.d/colorgrep.sh: Interrupted system call
[10@167b67fea278 /]# id
uid=10(10) gid=0(root) groups=0(root)

Signed-off-by: y00316549 yangshukui@huawei.com

@Jimmy-Xu
Copy link

Jimmy-Xu commented Jan 5, 2018

Can one of the admins verify this patch?

@gnawux
Copy link
Member

gnawux commented Jan 5, 2018

LGTM
cc @laijs

@laijs
Copy link
Contributor

laijs commented Jan 5, 2018

Hello, Thanks for the fixing.

Could you fix hyper_getpwnam() hyper_getgrnam() instead?
(change it to: if the id is numeric, don't compare the name)

Other info is needed from /etc/passwd even the id is numeric.

@keloyang
Copy link
Contributor Author

keloyang commented Jan 5, 2018

I have improved it. thank you.

@laijs
Copy link
Contributor

laijs commented Jan 5, 2018

I don't think there are any differents in the new code. How do you test it?

src/util.c Outdated
@@ -162,20 +168,27 @@ struct passwd *hyper_getpwnam(const char *name)
struct passwd *pwd = fgetpwent(file);
if (!pwd)
break;
if (!strcmp(pwd->pw_name, name) || pwd->pw_uid == uid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just
if ((INVALID_UGID == uid && !strcmp(pwd->pw_name, name)) || pwd->pw_uid == uid) {
is enough.
INVALID_UGID should be changed to a better name.

Copy link
Contributor Author

@keloyang keloyang Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case is above this pr, take hyper_getpwnam(1000) for example

/etc/passwd                              
1000:x:1001:0:*:/*:/bin/bash
                   |
                   V
hyper_getpwnam(1000)  = struct passwd {pw_name: 1000, uid: 1001} 
/etc/passwd                              
1000:x:1001:0:*:/*:/bin/bash
hello:x:1000:0:*:/*:/bin/bash
                   |
                   V
hyper_getpwnam(1000)  = struct passwd {pw_name: hello, uid: 1000} 
/etc/passwd                              
have no 1000 as name or uid
                   |
                   V
hyper_getpwnam(1000)  = NULL

And what do you think we can use to replace INVALID_UGID ?

src/util.c Outdated
fclose(file);
return pwd;
}
if (!strcmp(pwd->pw_name, name)) {
user_pwd = pwd;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please covert the logic to
if ((INVALID_UGID == uid && !strcmp(pwd->pw_name, name)) || pwd->pw_uid == uid) {

so that "10" is always uid instead name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated , thanks.

Signed-off-by: y00316549 <yangshukui@huawei.com>
@laijs laijs merged commit 7670d5e into hyperhq:master Jan 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants