Skip to content
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

Smackfs interfaces #1513

Merged
merged 3 commits into from Nov 22, 2019
Merged

Smackfs interfaces #1513

merged 3 commits into from Nov 22, 2019

Conversation

@evdenis
Copy link
Contributor

evdenis commented Nov 21, 2019

Add descriptions for /sys/fs/smackfs/* interfaces.
Allowed to find out-of-bounds read in smk_set_cipso(): https://lkml.org/lkml/2019/11/20/633

Signed-off-by: Denis Efremov efremov@linux.com

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 21, 2019

Codecov Report

Merging #1513 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1513      +/-   ##
==========================================
- Coverage    56.2%   56.16%   -0.05%     
==========================================
  Files         142      142              
  Lines       26415    26415              
==========================================
- Hits        14847    14835      -12     
- Misses      10859    10869      +10     
- Partials      709      711       +2
Impacted Files Coverage Δ
prog/checksum.go 87.91% <0%> (-2.2%) ⬇️
prog/rand.go 87.56% <0%> (-0.92%) ⬇️
prog/encoding.go 83.5% <0%> (-0.9%) ⬇️
pkg/csource/csource.go 75.28% <0%> (+0.57%) ⬆️

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 8098ea0...7b4b4e1. Read the comment docs.

level fmt[dec, int8[0:SMACK_CIPSO_MAXLEVEL]]
sp1 const[' ', int8]
# SMK_DIGITLEN
# TODO: num fmt[dec, len[cats, int8]]

This comment has been minimized.

Copy link
@dvyukov

dvyukov Nov 21, 2019

Collaborator

Why doesn't this work? It should, no?

This comment has been minimized.

Copy link
@evdenis

evdenis Nov 21, 2019

Author Contributor

It doesn't work:

$ make generate
smack.txt:88:15: wrong number of arguments for type len, expect len target

This works:

num	fmt[dec, len[cats]]

I think that it will use intptr in this case.

However, we need int8 here because of: https://www.kernel.org/doc/html/latest/admin-guide/LSM/Smack.html

cipso2
This interface allows a specific CIPSO header to be assigned to a Smack label. The format accepted on write is:

"%s%4d%4d"["%4d"]...

Actually, it's not a "%4d" it's "%3d" with whitespace. This len is in [0:184] https://elixir.bootlin.com/linux/v5.4-rc8/source/security/smack/smackfs.c#L887

This comment has been minimized.

Copy link
@dvyukov

dvyukov Nov 21, 2019

Collaborator

Using fmt[dec, len[cats]] looks better to me. We will have the correct length and unless the array itself contains more than 184 elements, this value will be correct as well.

This comment has been minimized.

Copy link
@evdenis

evdenis Nov 21, 2019

Author Contributor

Done. I just hope it will not add leading zeroes. It's not clear to me what input will be generated in this case.

"fmt": a string representation of an integer (not zero-terminated), type-options:
format (one of "dec", "hex", "oct") and the value (a resource, int, flags, const or proc)
the resulting data is always fixed-size (formatted as "%020llu", "0x%016llx" or "%023llo", respectively)

Smack expects here exactly 3-size number, e.g.: 001, 183, 010...

This comment has been minimized.

Copy link
@dvyukov

dvyukov Nov 22, 2019

Collaborator

Ah, I see. Yes, it won't format numbers this ways, but int8 won't help either.
The problem is that we still don't support for dynamically-sized arguments. Say an array is variable size, but once a program is generated its size is known. Whereas if we want to sprintf a dynamic number (e.g. a pid) we don't know until the runtime how many digits it will take. So the current format is a simplified version that always pads numbers to max size.
But actually this case is yet different b/c for %03d we do know the resulting size.

This comment has been minimized.

Copy link
@dvyukov

dvyukov Nov 22, 2019

Collaborator

I've clarified the comment a bit:
0199dc9

sys/linux/smack.txt Outdated Show resolved Hide resolved
sys/linux/smack.txt Outdated Show resolved Hide resolved
@dvyukov

This comment has been minimized.

Copy link
Collaborator

dvyukov commented Nov 21, 2019

Cool! I see /sys/fs/smackfs is automounted when smack is enabled, so we shouldn't need anything else for syzbot to test this.

@evdenis

This comment has been minimized.

Copy link
Contributor Author

evdenis commented Nov 21, 2019

Cool! I see /sys/fs/smackfs is automounted when smack is enabled, so we shouldn't need anything else for syzbot to test this.

As far as I understand there is no "permissive" mode in smack. Thus, I use this patch to force it:

diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 38ac3da4e791..ec59c06cd491 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -12,6 +12,9 @@
 #include <linux/sched.h>
 #include "smack.h"
 
+#undef EACCES
+#define EACCES 0
+
 struct smack_known smack_known_huh = {
        .smk_known      = "?",
        .smk_secid      = 2,
@@ -638,7 +641,7 @@ bool smack_privileged_cred(int cap, const struct cred *cred)
 
        rc = cap_capable(cred, &init_user_ns, cap, CAP_OPT_NONE);
        if (rc)
-               return false;
+               return true;
 
        rcu_read_lock();
        if (list_empty(&smack_onlycap_list)) {
@@ -654,7 +657,7 @@ bool smack_privileged_cred(int cap, const struct cred *cred)
        }
        rcu_read_unlock();
 
-       return false;
+       return true;
 }
 
 /**
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index abeb09c30633..1c0f72525ad6 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -44,6 +44,9 @@
 #include <linux/fs_parser.h>
 #include "smack.h"
 
+#undef EACCES
+#define EACCES 0
+
 #define TRANS_TRUE     "TRUE"
 #define TRANS_TRUE_SIZE        4

Otherwise it is better to not touch onlycap interface (and maybe some others, syslog?, ptrace?) during fuzzing. It will result in connection loss and no-output errors.

onlycap
This contains labels processes must have for CAP_MAC_ADMIN and CAP_MAC_OVERRIDE to be effective.

Maybe @cschaufler could provide some hints.

@evdenis evdenis force-pushed the evdenis:smack branch from 9ccf20c to 7b4b4e1 Nov 21, 2019
@cschaufler

This comment has been minimized.

Copy link

cschaufler commented Nov 21, 2019

@dvyukov

This comment has been minimized.

Copy link
Collaborator

dvyukov commented Nov 21, 2019

You would suggest to disable /sys/fs/smackfs/onlycap on syzbot then, right? Is so, please split the openat that opens it into 2, so that we can disable only onlycap and not relabel-self. And also add a comment that it changes global restrictive policy and may need to be disabled.

evdenis added 3 commits Nov 18, 2019
Add descriptions for /sys/fs/smackfs/* interfaces.

Signed-off-by: Denis Efremov <efremov@linux.com>
Signed-off-by: Denis Efremov <efremov@linux.com>
Signed-off-by: Denis Efremov <efremov@linux.com>
@evdenis evdenis force-pushed the evdenis:smack branch from 7b4b4e1 to c3e19a2 Nov 21, 2019
@evdenis

This comment has been minimized.

Copy link
Contributor Author

evdenis commented Nov 21, 2019

You would suggest to disable /sys/fs/smackfs/onlycap on syzbot then, right?

In my experience openat$smackfs_ambient and openat$smackfs_unconfined results in frequent non-reproducible connection loss reports. I don't know is this a real issue or not, but if you want to avoid it then you need to disable them. I think that disabling onlycap will result in better coverage.

Is so, please split the openat that opens it into 2, so that we can disable only onlycap and not relabel-self. And also add a comment that it changes global restrictive policy and may need to be disabled.

Done.

@cschaufler

This comment has been minimized.

Copy link

cschaufler commented Nov 21, 2019

@dvyukov dvyukov merged commit e89749e into google:master Nov 22, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
cla/google All necessary CLAs are signed
@dvyukov

This comment has been minimized.

Copy link
Collaborator

dvyukov commented Nov 22, 2019

I will do:

				"disable_syscalls": [
					"openat$smackfs_onlycap",
					"openat$smackfs_ambient",
					"openat$smackfs_unconfined"
				]

on syzbot. The smack instance tests whole kernel, so breaking machines frequently won't be good.

@evdenis

This comment has been minimized.

Copy link
Contributor Author

evdenis commented Nov 22, 2019

Thanks for the clarification!

Yes, I think this will be a right choice to disable them. Thanks!

@evdenis evdenis deleted the evdenis:smack branch Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.