Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Don't expose /etc/alternatives from the host #50
Conversation
|
Looks good code-wise. But did you talk to ogra about the idea to have a custom update-alternatives in the image buidl that avoids this completely? |
|
I'll talk to ogre about a long term solution but this is here to fix snaps that are affected by this issue in the short term. |
jdstrand
reviewed
Jun 24, 2016
| @@ -215,6 +215,22 @@ void setup_snappy_os_mounts() | ||
| die("cannot bind mount %s to %s", src, dst); | ||
| } | ||
| } | ||
| + // Don't expose /etc/alternatives from the host, ensure that |
jdstrand
Jun 24, 2016
•
Edited 1 time
-
jdstrand
Jun 24, 2016
Contributor
-
Jun 24, 2016
To help reading the code, please say something like "Since we mounted /etc from the host above, we need to put /etc/alternatives from the os snap back..." (or similar). In other words, mention /etc is exposed but /etc/alternatives should be unexposed again.
As an aside: incidentally, this is precisely the problem with using bind mounts as a security mechanism-- they are far too coarse-grained and lead to "bind mount this huge list of things" or "bind mount this big directory and then bind mount this huge list of things to unexpose what's in the bid directory". I understand that what we are doing here has nothing to do with confinement, but I thought it worth mentioning that bind mounts like this lead to twisty implementations that are hard to follow.
jdstrand
reviewed
Jun 24, 2016
| + must_snprintf(dst, sizeof dst, "%s%s", rootfs_dir, | ||
| + etc_alternatives); | ||
| + debug("bind mounting %s to %s", src, dst); | ||
| + if (mount(src, dst, NULL, MS_BIND | MS_SLAVE, NULL) != 0) { |
jdstrand
Jun 24, 2016
Contributor
LGTM but please add a note about MS_SLAVE here like we do for the source_mounts mount() call above so it is clear.
zyga commentedJun 24, 2016
No description provided.