Merge lp:~jdstrand/snap-confine/security-review-fixes into lp:~snappy-dev/snap-confine/trunk
- security-review-fixes
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 110 |
Proposed branch: | lp:~jdstrand/snap-confine/security-review-fixes |
Merge into: | lp:~snappy-dev/snap-confine/trunk |
Diff against target: |
492 lines (+150/-110) 8 files modified
debian/changelog (+3/-0) src/main.c (+25/-25) src/seccomp.c (+46/-33) src/utils.c (+2/-1) tests/test_bad_seccomp_filter_length (+20/-0) tests/test_bad_seccomp_filter_missing_trailing_newline (+17/-0) tests/test_create_user_data (+7/-51) tests/test_unrestricted_missed (+30/-0) |
To merge this branch: | bzr merge lp:~jdstrand/snap-confine/security-review-fixes |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tyler Hicks (community) | Needs Fixing | ||
Snappy Developers | Pending | ||
Review via email:
|
Commit message
Description of the change
Various fixes to address Seth's feedback:
- cleaned up 'out' handling and die instead
- fixed the typo
- verify snprint() >= 512 (I increased this length) and added tests
- add test for filter missing trailing newline (I didn't change the line-too-long handling, but did add tests and verify we must end with trailing newline
- use strcmp() with "@unrestricted". add @unrestricted near miss tests
- use getresuid() instead of UBUNTU_
- replace getenv() with secure_getenv() everywhere we can
- check return code of fork()
- verify return code of fclose()
- simplified final execv()
In addition:
- don't support obsoleted SNAP_APP_TMPDIR and SNAP_APP_
- use uid_t and gid_t instead of unsigned
- check return codes of other (f)close()s to help futureproof
- 122. By Jamie Strandboge
-
merge from trunk
- 123. By Jamie Strandboge
-
rename recently add tests
- 124. By Jamie Strandboge
-
merge from trunk
- 125. By Jamie Strandboge
-
fix debian/changelog which had improper syntax
- 126. By Jamie Strandboge
-
merge from trunk
- 127. By Jamie Strandboge
-
merge from trunk
- 128. By Jamie Strandboge
-
merge from trunk
- 129. By Jamie Strandboge
-
merge from trunk
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tyler Hicks (tyhicks) wrote : | # |
The seccomp_
- 130. By Jamie Strandboge
-
update a few comments based on tyhicks feedback
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tyler Hicks (tyhicks) wrote : | # |
I've completed my review now. I only had one additional comment regarding another fclose() error check.
- 131. By Jamie Strandboge
-
simplify seccomp_
load_filters( ) and always die() on error instead of trying to
return non-zero sometimes
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jamie Strandboge (jdstrand) wrote : | # |
Thanks for the review! Yes, the error checking on the (f)close()s are not needed since they are read only, but I added them to help futureproof the codebase. Leaving those in.
I cleaned up seccomp_
Preview Diff
1 | === modified file 'debian/changelog' | |||
2 | --- debian/changelog 2016-03-30 12:46:25 +0000 | |||
3 | +++ debian/changelog 2016-03-31 19:41:16 +0000 | |||
4 | @@ -2,6 +2,9 @@ | |||
5 | 2 | 2 | ||
6 | 3 | * update README for recent commits | 3 | * update README for recent commits |
7 | 4 | * add README.syscalls | 4 | * add README.syscalls |
8 | 5 | * src/seccomp.c: various cleanups from security team audit (also add | ||
9 | 6 | additional tests) | ||
10 | 7 | * don't support obsoleted SNAP_APP_TMPDIR and SNAP_APP_USER_DATA_PATH | ||
11 | 5 | 8 | ||
12 | 6 | -- Jamie Strandboge <jamie@ubuntu.com> Tue, 29 Mar 2016 16:07:13 -0500 | 9 | -- Jamie Strandboge <jamie@ubuntu.com> Tue, 29 Mar 2016 16:07:13 -0500 |
13 | 7 | 10 | ||
14 | 8 | 11 | ||
15 | === modified file 'src/main.c' | |||
16 | --- src/main.c 2016-03-22 19:42:30 +0000 | |||
17 | +++ src/main.c 2016-03-31 19:41:16 +0000 | |||
18 | @@ -67,6 +67,9 @@ | |||
19 | 67 | 67 | ||
20 | 68 | int status = 0; | 68 | int status = 0; |
21 | 69 | pid_t pid = fork(); | 69 | pid_t pid = fork(); |
22 | 70 | if (pid < 0) { | ||
23 | 71 | die("could not fork"); | ||
24 | 72 | } | ||
25 | 70 | if (pid == 0) { | 73 | if (pid == 0) { |
26 | 71 | char buf[64]; | 74 | char buf[64]; |
27 | 72 | unsigned major = MAJOR(devnum); | 75 | unsigned major = MAJOR(devnum); |
28 | @@ -198,7 +201,8 @@ | |||
29 | 198 | if (fd < 0) | 201 | if (fd < 0) |
30 | 199 | return false; | 202 | return false; |
31 | 200 | int n = read(fd, content, sizeof(content)); | 203 | int n = read(fd, content, sizeof(content)); |
33 | 201 | close(fd); | 204 | if (close(fd) != 0) |
34 | 205 | die("could not close override"); | ||
35 | 202 | if (n < sizeof(content)) | 206 | if (n < sizeof(content)) |
36 | 203 | return false; | 207 | return false; |
37 | 204 | 208 | ||
38 | @@ -259,7 +263,7 @@ | |||
39 | 259 | die("unable to chown tmpdir"); | 263 | die("unable to chown tmpdir"); |
40 | 260 | } | 264 | } |
41 | 261 | // ensure we set the various TMPDIRs to our newly created tmpdir | 265 | // ensure we set the various TMPDIRs to our newly created tmpdir |
43 | 262 | const char *tmpd[] = { "TMPDIR", "TEMPDIR", "SNAP_APP_TMPDIR", NULL }; | 266 | const char *tmpd[] = { "TMPDIR", "TEMPDIR", NULL }; |
44 | 263 | int i; | 267 | int i; |
45 | 264 | for (i = 0; tmpd[i] != NULL; i++) { | 268 | for (i = 0; tmpd[i] != NULL; i++) { |
46 | 265 | if (setenv(tmpd[i], "/tmp", 1) != 0) { | 269 | if (setenv(tmpd[i], "/tmp", 1) != 0) { |
47 | @@ -395,7 +399,7 @@ | |||
48 | 395 | // Try to create the directory. It's okay if it already | 399 | // Try to create the directory. It's okay if it already |
49 | 396 | // existed, but any other error is fatal. | 400 | // existed, but any other error is fatal. |
50 | 397 | if (mkdirat(fd, path_segment, 0755) < 0 && errno != EEXIST) { | 401 | if (mkdirat(fd, path_segment, 0755) < 0 && errno != EEXIST) { |
52 | 398 | close(fd); | 402 | close(fd); // we die regardless of return code |
53 | 399 | free(path_copy); | 403 | free(path_copy); |
54 | 400 | die("failed to create user data directory"); | 404 | die("failed to create user data directory"); |
55 | 401 | } | 405 | } |
56 | @@ -403,7 +407,10 @@ | |||
57 | 403 | // previous one) so we can continue down the path. | 407 | // previous one) so we can continue down the path. |
58 | 404 | int previous_fd = fd; | 408 | int previous_fd = fd; |
59 | 405 | fd = openat(fd, path_segment, open_flags); | 409 | fd = openat(fd, path_segment, open_flags); |
61 | 406 | close(previous_fd); | 410 | if (close(previous_fd) != 0) { |
62 | 411 | free(path_copy); | ||
63 | 412 | die("could not close path segment"); | ||
64 | 413 | } | ||
65 | 407 | if (fd < 0) { | 414 | if (fd < 0) { |
66 | 408 | free(path_copy); | 415 | free(path_copy); |
67 | 409 | die("failed to create user data directory"); | 416 | die("failed to create user data directory"); |
68 | @@ -413,7 +420,10 @@ | |||
69 | 413 | } | 420 | } |
70 | 414 | 421 | ||
71 | 415 | // Close the descriptor for the final directory in the path. | 422 | // Close the descriptor for the final directory in the path. |
73 | 416 | close(fd); | 423 | if (close(fd) != 0) { |
74 | 424 | free(path_copy); | ||
75 | 425 | die("could not close final directory"); | ||
76 | 426 | } | ||
77 | 417 | 427 | ||
78 | 418 | free(path_copy); | 428 | free(path_copy); |
79 | 419 | } | 429 | } |
80 | @@ -422,16 +432,8 @@ | |||
81 | 422 | { | 432 | { |
82 | 423 | const char *user_data = getenv("SNAP_USER_DATA"); | 433 | const char *user_data = getenv("SNAP_USER_DATA"); |
83 | 424 | 434 | ||
94 | 425 | // If $SNAP_USER_DATA wasn't defined, check the deprecated | 435 | if (user_data == NULL) |
95 | 426 | // $SNAP_APP_USER_DATA_PATH. | 436 | return; |
86 | 427 | if (user_data == NULL) { | ||
87 | 428 | user_data = getenv("SNAP_APP_USER_DATA_PATH"); | ||
88 | 429 | // If it's still not defined, there's nothing to do. No need to | ||
89 | 430 | // die, there's simply no directory to create. | ||
90 | 431 | if (user_data == NULL) { | ||
91 | 432 | return; | ||
92 | 433 | } | ||
93 | 434 | } | ||
96 | 435 | // Only support absolute paths. | 437 | // Only support absolute paths. |
97 | 436 | if (user_data[0] != '/') { | 438 | if (user_data[0] != '/') { |
98 | 437 | die("user data directory must be an absolute path"); | 439 | die("user data directory must be an absolute path"); |
99 | @@ -449,15 +451,16 @@ | |||
100 | 449 | const char *appname = argv[1]; | 451 | const char *appname = argv[1]; |
101 | 450 | const char *aa_profile = argv[2]; | 452 | const char *aa_profile = argv[2]; |
102 | 451 | const char *binary = argv[3]; | 453 | const char *binary = argv[3]; |
105 | 452 | unsigned real_uid = getuid(); | 454 | uid_t real_uid = getuid(); |
106 | 453 | unsigned real_gid = getgid(); | 455 | gid_t real_gid = getgid(); |
107 | 454 | 456 | ||
108 | 455 | if (!verify_appname(appname)) | 457 | if (!verify_appname(appname)) |
109 | 456 | die("appname %s not allowed", appname); | 458 | die("appname %s not allowed", appname); |
110 | 457 | 459 | ||
111 | 458 | // this code always needs to run as root for the cgroup/udev setup, | 460 | // this code always needs to run as root for the cgroup/udev setup, |
112 | 459 | // however for the tests we allow it to run as non-root | 461 | // however for the tests we allow it to run as non-root |
114 | 460 | if (geteuid() != 0 && getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) { | 462 | if (geteuid() != 0 |
115 | 463 | && secure_getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) { | ||
116 | 461 | die("need to run as root or suid"); | 464 | die("need to run as root or suid"); |
117 | 462 | } | 465 | } |
118 | 463 | 466 | ||
119 | @@ -511,13 +514,11 @@ | |||
120 | 511 | // set apparmor rules | 514 | // set apparmor rules |
121 | 512 | rc = aa_change_onexec(aa_profile); | 515 | rc = aa_change_onexec(aa_profile); |
122 | 513 | if (rc != 0) { | 516 | if (rc != 0) { |
124 | 514 | if (getenv("SNAPPY_LAUNCHER_INSIDE_TESTS") == NULL) | 517 | if (secure_getenv("SNAPPY_LAUNCHER_INSIDE_TESTS") == NULL) |
125 | 515 | die("aa_change_onexec failed with %i", rc); | 518 | die("aa_change_onexec failed with %i", rc); |
126 | 516 | } | 519 | } |
131 | 517 | // set seccomp | 520 | // set seccomp (note: seccomp_load_filters die()s on all failures) |
132 | 518 | rc = seccomp_load_filters(aa_profile); | 521 | seccomp_load_filters(aa_profile); |
129 | 519 | if (rc != 0) | ||
130 | 520 | die("seccomp_load_filters failed with %i", rc); | ||
133 | 521 | 522 | ||
134 | 522 | // Permanently drop if not root | 523 | // Permanently drop if not root |
135 | 523 | if (geteuid() == 0) { | 524 | if (geteuid() == 0) { |
136 | @@ -534,8 +535,7 @@ | |||
137 | 534 | die("permanently dropping privs did not work"); | 535 | die("permanently dropping privs did not work"); |
138 | 535 | } | 536 | } |
139 | 536 | // and exec the new binary | 537 | // and exec the new binary |
142 | 537 | argv[NR_ARGS] = (char *)binary, | 538 | execv(binary, (char *const *)&argv[NR_ARGS]); |
141 | 538 | execv(binary, (char *const *)&argv[NR_ARGS]); | ||
143 | 539 | perror("execv failed"); | 539 | perror("execv failed"); |
144 | 540 | return 1; | 540 | return 1; |
145 | 541 | } | 541 | } |
146 | 542 | 542 | ||
147 | === modified file 'src/seccomp.c' | |||
148 | --- src/seccomp.c 2016-03-22 19:42:30 +0000 | |||
149 | +++ src/seccomp.c 2016-03-31 19:41:16 +0000 | |||
150 | @@ -14,6 +14,10 @@ | |||
151 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. |
152 | 15 | * | 15 | * |
153 | 16 | */ | 16 | */ |
154 | 17 | #ifndef _GNU_SOURCE | ||
155 | 18 | #define _GNU_SOURCE | ||
156 | 19 | #endif | ||
157 | 20 | |||
158 | 17 | #include <errno.h> | 21 | #include <errno.h> |
159 | 18 | #include <stdio.h> | 22 | #include <stdio.h> |
160 | 19 | #include <unistd.h> | 23 | #include <unistd.h> |
161 | @@ -37,7 +41,7 @@ | |||
162 | 37 | return slen; | 41 | return slen; |
163 | 38 | } | 42 | } |
164 | 39 | 43 | ||
166 | 40 | int seccomp_load_filters(const char *filter_profile) | 44 | void seccomp_load_filters(const char *filter_profile) |
167 | 41 | { | 45 | { |
168 | 42 | debug("seccomp_load_filters %s", filter_profile); | 46 | debug("seccomp_load_filters %s", filter_profile); |
169 | 43 | int rc = 0; | 47 | int rc = 0; |
170 | @@ -45,45 +49,51 @@ | |||
171 | 45 | scmp_filter_ctx ctx = NULL; | 49 | scmp_filter_ctx ctx = NULL; |
172 | 46 | FILE *f = NULL; | 50 | FILE *f = NULL; |
173 | 47 | size_t lineno = 0; | 51 | size_t lineno = 0; |
174 | 52 | uid_t real_uid, effective_uid, saved_uid; | ||
175 | 48 | 53 | ||
176 | 49 | ctx = seccomp_init(SCMP_ACT_KILL); | 54 | ctx = seccomp_init(SCMP_ACT_KILL); |
180 | 50 | if (ctx == NULL) | 55 | if (ctx == NULL) { |
181 | 51 | return ENOMEM; | 56 | errno = ENOMEM; |
182 | 52 | 57 | die("seccomp_init() failed"); | |
183 | 58 | } | ||
184 | 53 | // Disable NO_NEW_PRIVS because it interferes with exec transitions in | 59 | // Disable NO_NEW_PRIVS because it interferes with exec transitions in |
185 | 54 | // AppArmor. Unfortunately this means that security policies must be | 60 | // AppArmor. Unfortunately this means that security policies must be |
186 | 55 | // very careful to not allow the following otherwise apps can escape | 61 | // very careful to not allow the following otherwise apps can escape |
188 | 56 | // the snadbox: | 62 | // the sandbox: |
189 | 57 | // - seccomp syscall | 63 | // - seccomp syscall |
190 | 58 | // - prctl with PR_SET_SECCOMP | 64 | // - prctl with PR_SET_SECCOMP |
191 | 59 | // - ptrace (trace) in AppArmor | 65 | // - ptrace (trace) in AppArmor |
192 | 60 | // - capability sys_admin in AppArmor | 66 | // - capability sys_admin in AppArmor |
193 | 61 | // Note that with NO_NEW_PRIVS disabled, CAP_SYS_ADMIN is required to | 67 | // Note that with NO_NEW_PRIVS disabled, CAP_SYS_ADMIN is required to |
194 | 62 | // change the seccomp sandbox. | 68 | // change the seccomp sandbox. |
204 | 63 | if (getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) { | 69 | |
205 | 64 | rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_NNP, 0); | 70 | if (getresuid(&real_uid, &effective_uid, &saved_uid) != 0) |
206 | 65 | if (rc != 0) { | 71 | die("could not find user IDs"); |
207 | 66 | fprintf(stderr, "Cannot disable nnp\n"); | 72 | |
208 | 67 | return -1; | 73 | // If running privileged or capable of raising, disable nnp |
209 | 68 | } | 74 | if (real_uid == 0 || effective_uid == 0 || saved_uid == 0) |
210 | 69 | } | 75 | if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_NNP, 0) != 0) |
211 | 70 | 76 | die("Cannot disable nnp"); | |
212 | 71 | if (getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR") != NULL) | 77 | |
213 | 78 | // Note that secure_gettenv will always return NULL when suid, so | ||
214 | 79 | // SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR can't be (ab)used in that case. | ||
215 | 80 | if (secure_getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR") != NULL) | ||
216 | 72 | filter_profile_dir = | 81 | filter_profile_dir = |
218 | 73 | getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR"); | 82 | secure_getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR"); |
219 | 74 | 83 | ||
225 | 75 | char profile_path[128]; | 84 | char profile_path[512]; // arbitrary path name limit |
226 | 76 | if (snprintf | 85 | int snprintf_rc = snprintf(profile_path, sizeof(profile_path), "%s/%s", |
227 | 77 | (profile_path, sizeof(profile_path), "%s/%s", filter_profile_dir, | 86 | filter_profile_dir, filter_profile); |
228 | 78 | filter_profile) < 0) { | 87 | if (snprintf_rc < 0 || snprintf_rc >= 512) { |
229 | 79 | goto out; | 88 | errno = 0; |
230 | 89 | die("snprintf returned unexpected value"); | ||
231 | 80 | } | 90 | } |
232 | 81 | 91 | ||
233 | 82 | f = fopen(profile_path, "r"); | 92 | f = fopen(profile_path, "r"); |
234 | 83 | if (f == NULL) { | 93 | if (f == NULL) { |
235 | 84 | fprintf(stderr, "Can not open %s (%s)\n", profile_path, | 94 | fprintf(stderr, "Can not open %s (%s)\n", profile_path, |
236 | 85 | strerror(errno)); | 95 | strerror(errno)); |
238 | 86 | return -1; | 96 | die("aborting"); |
239 | 87 | } | 97 | } |
240 | 88 | // 80 characters + '\n' + '\0' | 98 | // 80 characters + '\n' + '\0' |
241 | 89 | char buf[82]; | 99 | char buf[82]; |
242 | @@ -104,16 +114,17 @@ | |||
243 | 104 | fprintf(stderr, | 114 | fprintf(stderr, |
244 | 105 | "seccomp filter line %zu was too long (%zu characters max)\n", | 115 | "seccomp filter line %zu was too long (%zu characters max)\n", |
245 | 106 | lineno, sizeof(buf) - 2); | 116 | lineno, sizeof(buf) - 2); |
248 | 107 | rc = -1; | 117 | errno = 0; |
249 | 108 | goto out; | 118 | die("aborting"); |
250 | 109 | } | 119 | } |
251 | 110 | // kill final newline | 120 | // kill final newline |
252 | 111 | len = trim_right(buf, len); | 121 | len = trim_right(buf, len); |
253 | 112 | if (len == 0) | 122 | if (len == 0) |
254 | 113 | continue; | 123 | continue; |
255 | 114 | 124 | ||
258 | 115 | // check for special "@unrestricted" command | 125 | // check for special "@unrestricted" rule which short-circuits |
259 | 116 | if (strncmp(buf, "@unrestricted", sizeof(buf)) == 0) | 126 | // seccomp sandbox |
260 | 127 | if (strcmp(buf, "@unrestricted") == 0) | ||
261 | 117 | goto out; | 128 | goto out; |
262 | 118 | 129 | ||
263 | 119 | // syscall not available on this arch/kernel | 130 | // syscall not available on this arch/kernel |
264 | @@ -132,13 +143,15 @@ | |||
265 | 132 | fprintf(stderr, | 143 | fprintf(stderr, |
266 | 133 | "seccomp_rule_add failed with %i for '%s'\n", | 144 | "seccomp_rule_add failed with %i for '%s'\n", |
267 | 134 | rc, buf); | 145 | rc, buf); |
269 | 135 | goto out; | 146 | errno = 0; |
270 | 147 | die("aborting"); | ||
271 | 136 | } | 148 | } |
272 | 137 | } | 149 | } |
273 | 138 | } | 150 | } |
274 | 139 | 151 | ||
277 | 140 | // raise privileges to load seccomp policy since we don't have nnp | 152 | // If not root but can raise, then raise privileges to load seccomp |
278 | 141 | if (getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) { | 153 | // policy since we don't have nnp |
279 | 154 | if (effective_uid != 0 && saved_uid == 0) { | ||
280 | 142 | if (seteuid(0) != 0) | 155 | if (seteuid(0) != 0) |
281 | 143 | die("seteuid failed"); | 156 | die("seteuid failed"); |
282 | 144 | if (geteuid() != 0) | 157 | if (geteuid() != 0) |
283 | @@ -149,10 +162,8 @@ | |||
284 | 149 | 162 | ||
285 | 150 | if (rc != 0) { | 163 | if (rc != 0) { |
286 | 151 | fprintf(stderr, "seccomp_load failed with %i\n", rc); | 164 | fprintf(stderr, "seccomp_load failed with %i\n", rc); |
288 | 152 | goto out; | 165 | die("aborting"); |
289 | 153 | } | 166 | } |
290 | 154 | |||
291 | 155 | out: | ||
292 | 156 | // drop privileges again | 167 | // drop privileges again |
293 | 157 | if (geteuid() == 0) { | 168 | if (geteuid() == 0) { |
294 | 158 | unsigned real_uid = getuid(); | 169 | unsigned real_uid = getuid(); |
295 | @@ -162,9 +173,11 @@ | |||
296 | 162 | die("dropping privs after seccomp_load did not work"); | 173 | die("dropping privs after seccomp_load did not work"); |
297 | 163 | } | 174 | } |
298 | 164 | 175 | ||
299 | 176 | out: | ||
300 | 165 | if (f != NULL) { | 177 | if (f != NULL) { |
302 | 166 | fclose(f); | 178 | if (fclose(f) != 0) |
303 | 179 | die("could not close seccomp file"); | ||
304 | 167 | } | 180 | } |
305 | 168 | seccomp_release(ctx); | 181 | seccomp_release(ctx); |
307 | 169 | return rc; | 182 | return; |
308 | 170 | } | 183 | } |
309 | 171 | 184 | ||
310 | === modified file 'src/utils.c' | |||
311 | --- src/utils.c 2016-03-22 12:17:03 +0000 | |||
312 | +++ src/utils.c 2016-03-31 19:41:16 +0000 | |||
313 | @@ -70,7 +70,8 @@ | |||
314 | 70 | die("fwrite failed"); | 70 | die("fwrite failed"); |
315 | 71 | if (fflush(f) != 0) | 71 | if (fflush(f) != 0) |
316 | 72 | die("fflush failed"); | 72 | die("fflush failed"); |
318 | 73 | fclose(f); | 73 | if (fclose(f) != 0) |
319 | 74 | die("fclose failed"); | ||
320 | 74 | } | 75 | } |
321 | 75 | 76 | ||
322 | 76 | int must_snprintf(char *str, size_t size, const char *format, ...) | 77 | int must_snprintf(char *str, size_t size, const char *format, ...) |
323 | 77 | 78 | ||
324 | === added file 'tests/test_bad_seccomp_filter_length' | |||
325 | --- tests/test_bad_seccomp_filter_length 1970-01-01 00:00:00 +0000 | |||
326 | +++ tests/test_bad_seccomp_filter_length 2016-03-31 19:41:16 +0000 | |||
327 | @@ -0,0 +1,20 @@ | |||
328 | 1 | #!/bin/sh | ||
329 | 2 | |||
330 | 3 | set -e | ||
331 | 4 | |||
332 | 5 | . $(pwd)/common.sh | ||
333 | 6 | |||
334 | 7 | printf "Test seccomp filter (bad - too long)" | ||
335 | 8 | |||
336 | 9 | cat >$TMP/aaprofile <<EOF | ||
337 | 10 | # some comment | ||
338 | 11 | baddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd | ||
339 | 12 | EOF | ||
340 | 13 | |||
341 | 14 | if $L appname aaprofile /bin/true 2>/dev/null; then | ||
342 | 15 | # true returned successfully, length test failed | ||
343 | 16 | FAIL | ||
344 | 17 | fi | ||
345 | 18 | |||
346 | 19 | # all good | ||
347 | 20 | PASS | ||
348 | 0 | 21 | ||
349 | === added file 'tests/test_bad_seccomp_filter_missing_trailing_newline' | |||
350 | --- tests/test_bad_seccomp_filter_missing_trailing_newline 1970-01-01 00:00:00 +0000 | |||
351 | +++ tests/test_bad_seccomp_filter_missing_trailing_newline 2016-03-31 19:41:16 +0000 | |||
352 | @@ -0,0 +1,17 @@ | |||
353 | 1 | #!/bin/sh | ||
354 | 2 | |||
355 | 3 | set -e | ||
356 | 4 | |||
357 | 5 | . $(pwd)/common.sh | ||
358 | 6 | |||
359 | 7 | printf "Test seccomp filter (bad - no trailing newline)" | ||
360 | 8 | |||
361 | 9 | echo -n "missingnewline" > $TMP/aaprofile | ||
362 | 10 | |||
363 | 11 | if $L appname aaprofile /bin/true 2>/dev/null; then | ||
364 | 12 | # true returned successfully, length test failed | ||
365 | 13 | FAIL | ||
366 | 14 | fi | ||
367 | 15 | |||
368 | 16 | # all good | ||
369 | 17 | PASS | ||
370 | 0 | 18 | ||
371 | === modified file 'tests/test_create_user_data' | |||
372 | --- tests/test_create_user_data 2016-03-19 02:32:43 +0000 | |||
373 | +++ tests/test_create_user_data 2016-03-31 19:41:16 +0000 | |||
374 | @@ -82,56 +82,28 @@ | |||
375 | 82 | return $pass | 82 | return $pass |
376 | 83 | } | 83 | } |
377 | 84 | 84 | ||
378 | 85 | # $1 = $SNAP_APP_USER_DATA_PATH definition | ||
379 | 86 | # $2 = Profile name | ||
380 | 87 | # $3 = True if success is expected, false otherwise | ||
381 | 88 | run_deprecated() { | ||
382 | 89 | export SNAP_APP_USER_DATA_PATH=$1 | ||
383 | 90 | run_launcher $1 $2 $3 | ||
384 | 91 | pass=$? | ||
385 | 92 | unset SNAP_APP_USER_DATA_PATH | ||
386 | 93 | |||
387 | 94 | return $pass | ||
388 | 95 | } | ||
389 | 96 | |||
390 | 97 | # $1 = User data path | ||
391 | 98 | # $2 = Profile name | ||
392 | 99 | # $3 = True if success is expected, false otherwise | ||
393 | 100 | run_both() { | ||
394 | 101 | run_current $1 $2 $3 | ||
395 | 102 | current_pass=$? | ||
396 | 103 | run_deprecated $1 $2 $3 | ||
397 | 104 | deprecated_pass=$? | ||
398 | 105 | |||
399 | 106 | if [ $current_pass -a $deprecated_pass ]; then | ||
400 | 107 | PASS | ||
401 | 108 | else | ||
402 | 109 | FAIL | ||
403 | 110 | fi | ||
404 | 111 | } | ||
405 | 112 | |||
406 | 113 | printf "Test that an unrestricted launcher creates user data" | 85 | printf "Test that an unrestricted launcher creates user data" |
408 | 114 | run_both $TMP/user_data unrestricted true | 86 | run_current $TMP/user_data unrestricted true |
409 | 115 | 87 | ||
410 | 116 | printf "Test that a restricted launcher creates user data" | 88 | printf "Test that a restricted launcher creates user data" |
412 | 117 | run_both $TMP/user_data restricted true | 89 | run_current $TMP/user_data restricted true |
413 | 118 | 90 | ||
414 | 119 | printf "Test that an unrestricted launcher creates user data with parent directory" | 91 | printf "Test that an unrestricted launcher creates user data with parent directory" |
416 | 120 | run_both $TMP/parent/user_data unrestricted true | 92 | run_current $TMP/parent/user_data unrestricted true |
417 | 121 | 93 | ||
418 | 122 | printf "Test that a restricted launcher creates user data with parent directory" | 94 | printf "Test that a restricted launcher creates user data with parent directory" |
420 | 123 | run_both $TMP/parent/user_data restricted true | 95 | run_current $TMP/parent/user_data restricted true |
421 | 124 | 96 | ||
422 | 125 | printf "Test that user data can contain multiple path separators" | 97 | printf "Test that user data can contain multiple path separators" |
424 | 126 | run_both $TMP//user_data unrestricted true | 98 | run_current $TMP//user_data unrestricted true |
425 | 127 | 99 | ||
426 | 128 | printf "Test that user data must be absolute" | 100 | printf "Test that user data must be absolute" |
428 | 129 | run_both "../foo" unrestricted false | 101 | run_current "../foo" unrestricted false |
429 | 130 | 102 | ||
430 | 131 | printf "Testing that user data cannot be contained within a symlink" | 103 | printf "Testing that user data cannot be contained within a symlink" |
431 | 132 | mkdir $TMP/nefarious_parent | 104 | mkdir $TMP/nefarious_parent |
432 | 133 | ln -s $TMP/nefarious_parent $TMP/parent | 105 | ln -s $TMP/nefarious_parent $TMP/parent |
434 | 134 | run_both $TMP/parent/user_data unrestricted, false | 106 | run_current $TMP/parent/user_data unrestricted, false |
435 | 135 | 107 | ||
436 | 136 | printf "Test that an unrestricted launcher works when user data exists (current)" | 108 | printf "Test that an unrestricted launcher works when user data exists (current)" |
437 | 137 | mkdir $TMP/user_data | 109 | mkdir $TMP/user_data |
438 | @@ -148,19 +120,3 @@ | |||
439 | 148 | else | 120 | else |
440 | 149 | FAIL | 121 | FAIL |
441 | 150 | fi | 122 | fi |
442 | 151 | |||
443 | 152 | printf "Test that an unrestricted launcher works when user data exists (deprecated)" | ||
444 | 153 | mkdir $TMP/user_data | ||
445 | 154 | if run_deprecated $TMP/user_data unrestricted true; then | ||
446 | 155 | PASS | ||
447 | 156 | else | ||
448 | 157 | FAIL | ||
449 | 158 | fi | ||
450 | 159 | |||
451 | 160 | printf "Test that an restricted launcher works when user data exists (deprecated)" | ||
452 | 161 | mkdir $TMP/user_data | ||
453 | 162 | if run_deprecated $TMP/user_data restricted true; then | ||
454 | 163 | PASS | ||
455 | 164 | else | ||
456 | 165 | FAIL | ||
457 | 166 | fi | ||
458 | 167 | 123 | ||
459 | === added file 'tests/test_unrestricted_missed' | |||
460 | --- tests/test_unrestricted_missed 1970-01-01 00:00:00 +0000 | |||
461 | +++ tests/test_unrestricted_missed 2016-03-31 19:41:16 +0000 | |||
462 | @@ -0,0 +1,30 @@ | |||
463 | 1 | #!/bin/sh | ||
464 | 2 | |||
465 | 3 | set -e | ||
466 | 4 | |||
467 | 5 | . $(pwd)/common.sh | ||
468 | 6 | |||
469 | 7 | cat >$TMP/myprofile <<EOF | ||
470 | 8 | # super strict filter | ||
471 | 9 | @unrestricte | ||
472 | 10 | @unrestrictes | ||
473 | 11 | @nrestricted | ||
474 | 12 | @UNRESTRICTED | ||
475 | 13 | unrestricted | ||
476 | 14 | EOF | ||
477 | 15 | |||
478 | 16 | # ensure that the command "true" can not run due to impossible | ||
479 | 17 | # filtering | ||
480 | 18 | |||
481 | 19 | printf "Test that near misses of unrestricted fail" | ||
482 | 20 | if $L appid myprofile /bin/true 2>/dev/null; then | ||
483 | 21 | # true returned successfully, our filtering is broken! | ||
484 | 22 | FAIL | ||
485 | 23 | else | ||
486 | 24 | # true returend a error code, check dmesg | ||
487 | 25 | if dmesg|tail -n1|grep -q "audit"; then | ||
488 | 26 | PASS | ||
489 | 27 | else | ||
490 | 28 | FAIL | ||
491 | 29 | fi | ||
492 | 30 | fi |
I've completed a partial review. I still haven't gotten through all of the seccomp and test related changes. I'll complete that later this afternoon.