-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
daemon: overlay2: remove world writable permission from the lower file #47498
Conversation
In de2447c, the creation of the 'lower' file was changed from using os.Create to using ioutils.AtomicWriteFile, which ignores the system's umask. This means that even though the requested permission in the source code was always 0666, it was 0644 on systems with default umask of 0022 prior to de2447c, so the move to AtomicFile potentially increased the file's permissions. This is not a security issue because the parent directory does not allow writes into the file, but it can confuse security scanners on Linux-based systems into giving false positives. Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
Hm.. nice find. We should probably consider updating the GoDoc for those functions and call this out explicitly (either that, or make it align with |
Looks like #46471 was part of v25.0, so I added a cherry-pick label for consideration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me; it does appear that the intended semantics of the ioutils
methods are a literal permissions mask not accounting for umask.
Would you mind updating the docstrings in the pkg/ioutils
package to make clear how they handle the permissions bits?
I think it's fine to do that in a separate PR, as it's not directly related, and wouldn't be needed to backport, so separate concern. |
Would a v24 backport also be possible? That's the version this was reported to me against and what we use (SLES). I can open the PRs for both backports if needed. |
There won't be a 24.0 release, unless a maintainer is interested in sponsoring that work (currently Mirantis/Microsoft are maintaining 23.0 and everyone else is on 25.0). If SLES is packaging 24.0, I think you would be better off asking them to take a patch (or an untagged commit) in their packaging/fork. |
Thanks, that's enough for me (I will have that commit submitted, but we have an upstream-first policy and as such if upstream were to ever release another v24, I'd need to get the patch merged there before submitting it to SLES). |
Ah! I missed that that patch was backported, so indeed it looks like 23.0 and 24.0 also would have this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Created a quick tracking ticket for the GoDoc changes in case someone wants to pick up that work; |
Unlike its stdlib counterparts, AtomicFileWriter does not take into consideration umask due to its use of chmod. Failure to recognize this may cause subtle problems like the one described in moby#47498. Therefore the documentation has been updated to let users know that umask is not taken into consideration when using AtomicFileWriter. Closes moby#47516.
Unlike its stdlib counterparts, AtomicFileWriter does not take into consideration umask due to its use of chmod. Failure to recognize this may cause subtle problems like the one described in moby#47498. Therefore the documentation has been updated to let users know that umask is not taken into consideration when using AtomicFileWriter. Closes moby#47516. Signed-off-by: Antonio Aguilar <antonio@zoftko.com>
Unlike its stdlib counterparts, AtomicFileWriter does not take into consideration umask due to its use of chmod. Failure to recognize this may cause subtle problems like the one described in moby#47498. Therefore the documentation has been updated to let users know that umask is not taken into consideration when using AtomicFileWriter. Closes moby#47516. Signed-off-by: Antonio Aguilar <antonio@zoftko.com>
In de2447c, the creation of the 'lower' file was changed from using os.Create to using ioutils.AtomicWriteFile, which ignores the system's umask. This means that even though the requested permission in the source code was always 0666, it was 0644 on systems with default umask of 0022 prior to de2447c, so the move to AtomicFile potentially increased the file's permissions.
This is not a security issue because the parent directory does not allow writes into the file, but it can confuse security scanners on Linux-based systems into giving false positives.
Reproduction steps:
With this patch added to the same docker version (24.0.7_ce):
Output from the same system with docker-24.0.5_ce-150000.185.1 (before de2447c was added) installed shows the same behavior as 27.0.7_ce with this patch:
The patch from this PR restores the behavior from 24.0.5_ce where the lower files were not world writable.