Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snap-confine: validate SNAP_NAME against security tag #3476
Conversation
All checks have failed
| - int status = regexec(&re, security_tag, 0, NULL, 0); | ||
| + // first capture is the entire string, second is the name we care about | ||
| + regmatch_t matches[2]; | ||
| + int status = regexec(&re, security_tag, sizeof(matches)/sizeof(regmatch_t), matches, 0); |
| regfree(&re); | ||
| - return (status == 0); | ||
| + if (status != 0 || matches[1].rm_so < 0) { |
zyga
Jun 13, 2017
Contributor
Can you please document that this checks the sub-match associated for snap name was actually found.
All checks have failed
| - g_assert_false(verify_security_tag("snap..name.app")); | ||
| - g_assert_false(verify_security_tag("snap.name..app")); | ||
| - g_assert_false(verify_security_tag("snap.name.app..")); | ||
| + g_assert_false(verify_security_tag |
zyga
Jun 13, 2017
Contributor
Can you move this test to the end of the function and add a comment to explaining that it snap name and security tag are not matching (though are separately valid).
| @@ -27,25 +27,34 @@ | ||
| #include "string-utils.h" | ||
| #include "cleanup-funcs.h" | ||
| -bool verify_security_tag(const char *security_tag) | ||
| +bool verify_security_tag(const char *security_tag, const char *snap_name) | ||
| { | ||
| // The executable name is of form: |
zyga
Jun 13, 2017
Contributor
Can you move this to the documentation of the function in the header file please
All checks have failed
zyga
requested a review
from
jdstrand
Jun 13, 2017
Some checks were not successful
codecov-io
commented
Jun 14, 2017
•
Edited 1 time
-
codecov-io
Jun 19, 2017
-
Jun 19, 2017
Codecov Report
@@ Coverage Diff @@
## master #3476 +/- ##
==========================================
- Coverage 77.16% 77.14% -0.02%
==========================================
Files 373 373
Lines 25793 25793
==========================================
- Hits 19902 19899 -3
- Misses 4134 4136 +2
- Partials 1757 1758 +1
Continue to review full report at Codecov.
|
jdstrand
approved these changes
Jun 14, 2017
Thanks for this. Approving (though please make the requested comment changes before merging).
| die("can not compile regex %s", whitelist_re); | ||
| - int status = regexec(&re, security_tag, 0, NULL, 0); | ||
| + // first capture is the entire string, second is the name we care about |
jdstrand
Jun 14, 2017
Contributor
This comment isn't quite right. We actually care about both because we want to make sure the whole string matches the regex and that the snap name matches a specific substring. I suggest updating this comment to be:
// first capture is for verifying the full security tag, second capture
// for verifying the snap_name is correct for this security tag
| regfree(&re); | ||
| - return (status == 0); | ||
| + // make sure that snap name was captured by 2nd match group |
jdstrand
Jun 14, 2017
Contributor
Please adjust the comment to be:
// Fail if no match or if snap name wasn't captured in the 2nd match group
| + * - <hookname must start with a lowercase letter, then may | ||
| + * contain lowercase letters and '-' | ||
| + **/ | ||
| +bool verify_security_tag(const char *security_tag, const char *snap_name); |
jdstrand
Jun 14, 2017
Contributor
This change is fine. While it breaks API, this is in a private library and all callers of verify_security_tag() have either been removed or updated for the new arg.
| - if (snap_name == NULL) { | ||
| - die("SNAP_NAME is not set"); | ||
| - } | ||
| - sc_snap_name_validate(snap_name, NULL); | ||
| sc_snap_name_validate(base_snap_name, NULL); | ||
| debug("security tag: %s", security_tag); |
jdstrand
Jun 14, 2017
Contributor
The changes to snap-confine.c look fine-- it only moves the check and setting of snap_name up above verify_security_tag() and passes snap_name to verify_security_tag().
| @@ -157,9 +153,6 @@ void setup_devices_cgroup(const char *security_tag, struct snappy_udev *udev_s) | ||
| NULL, | ||
| }; | ||
| - // extra paranoia | ||
| - if (!verify_security_tag(security_tag)) | ||
| - die("security tag %s not allowed", security_tag); | ||
| if (udev_s == NULL) | ||
| die("snappy_udev is NULL"); | ||
| if (udev_s->udev == NULL) |
jdstrand
Jun 14, 2017
Contributor
The changes to udev-support.c look fine-- snappy_udev_init() and setup_devices_cgroup() are only used in snap-confine.c both called after verify_security_tag()
stolowski
added some commits
Jun 19, 2017
All checks have passed
mvo5
merged commit cb3b286
into
snapcore:master
Jun 19, 2017
4 of 7 checks passed
|
@jdstrand Thanks for the review, I've fixed the comments per your suggestions. |
stolowski commentedJun 13, 2017
Make sure SNAP_NAME env variable matches the name from security tag.