The rules-save file gets wrong permissions 066: # ls -la /var/lib/nftables/rules-save ----rw-rw- 1 root root 7539 Aug 2 14:39 /var/lib/nftables/rules-save Steps to reproduce: rm /var/lib/nftables/rules-save && /etc/init.d/nftables save I guess this is due to the umask set in the case statement in /usr/libexec/nftables/nftables.sh. The one I have here is equal to https://gitweb.gentoo.org/repo/gentoo.git/tree/net-firewall/nftables/files/libexec/nftables-mk.sh A non-root user invoking "nft list ruleset" is not allowed to see the applied rules. Having the rule-save file set to 066 bypasses this, as every non-root user can see applied rules.
(In reply to thomasb from comment #0) The same here. > I guess this is due to the umask set in the case statement in > /usr/libexec/nftables/nftables.sh. The one I have here is equal to > https://gitweb.gentoo.org/repo/gentoo.git/tree/net-firewall/nftables/files/ > libexec/nftables-mk.sh I change the "/usr/libexec/nftables/nftables.sh": main (){ "store") local tmp_save="${NFTABLES_SAVE}.tmp" ==> #umask 600; ==> umask 027; ( printf '#!/sbin/nft -f\nflush ruleset\n' nft ${SAVE_OPTIONS} list ruleset ) > "$tmp_save" && mv ${tmp_save} ${NFTABLES_SAVE} ;; } Now the file gets usr: rw, grp: r, other: none (chmod 640) > A non-root user invoking "nft list ruleset" is not allowed to see the > applied rules. Having the rule-save file set to 066 bypasses this, as every > non-root user can see applied rules. I don't understand the Philosophy of that. On my system only root can read or change the firewall. So in Consequence umask should be 666?
(In reply to Lloyd from comment #1) > (In reply to thomasb from comment #0)> > > I guess this is due to the umask set in the case statement in > > /usr/libexec/nftables/nftables.sh. The one I have here is equal to > > https://gitweb.gentoo.org/repo/gentoo.git/tree/net-firewall/nftables/files/ > > libexec/nftables-mk.sh > > I change the "/usr/libexec/nftables/nftables.sh": > [...] > Now the file gets usr: rw, grp: r, other: none (chmod 640) Makes sense, yes. > > A non-root user invoking "nft list ruleset" is not allowed to see the > > applied rules. Having the rule-save file set to 066 bypasses this, as every > > non-root user can see applied rules. > > I don't understand the Philosophy of that. On my system only root can read > or change the firewall. A non-root user invoking "nft list ruleset" gets an empty output, which is intentionally implemented that way to prevent unprivileged users from seeing the ruleset. With the nftables ebuild an unprivileged user can read /var/lib/nftables/rules-save and is able to see the ruleset. That's a contradiction: Upstream developers prevent unprivileged users from seeing the ruleset. The nftables ebuild sort of bypasses that as it makes the rule-save file world readable. > So in Consequence umask should be 666? From my point of view 027 or 077 would both be fine.
umask should have been 177 indeed. Seems I messed up when I wrote that code. Sorry! I'll get a patch over the weekend and get the issue fixed with a new revbump.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=73598a5e25d6583dde4f08a34df5073817c5a391 commit 73598a5e25d6583dde4f08a34df5073817c5a391 Author: Francisco Blas (klondike) Izquierdo Riera <klondike@gentoo.org> AuthorDate: 2019-09-07 20:38:38 +0000 Commit: Matthew Thode <prometheanfire@gentoo.org> CommitDate: 2019-09-07 21:24:11 +0000 net-firewall/nftables: Fix permissions for rules.save Due to a bug, the rules.save file was created with the wrong permissions which allowed all users to read the file with the system rules although root privileges are usually required to do so. To fix this issue, the following measures have been taken: * The umask on nftables-mk.sh is now correctly set to 177 * nftables.sh now also sets the umask before saving the rules * The ebuilds will warn on post installation if the rules.save has insecure permissions * The ebuilds have been bumped to ensure these changes are applied Bug: https://bugs.gentoo.org/691326 Signed-off-by: Francisco Blas Izquierdo Riera (klondike) <klondike@gentoo.org> Package-Manager: Portage-2.3.69, Repoman-2.3.11 Signed-off-by: Matthew Thode <prometheanfire@gentoo.org> net-firewall/nftables/files/libexec/nftables-mk.sh | 2 +- net-firewall/nftables/files/libexec/nftables.sh | 1 + net-firewall/nftables/nftables-0.9.0-r5.ebuild | 103 +++++++++++++++++++++ ...ables-0.9.1.ebuild => nftables-0.9.1-r1.ebuild} | 10 +- ...ables-0.9.2.ebuild => nftables-0.9.2-r1.ebuild} | 10 +- 5 files changed, 121 insertions(+), 5 deletions(-)
Created attachment 589380 [details] GLSA proposal
This issue allows local attackers to edit the ruleset that may be used on reboot (or in the best case create a race-condition instead between the script overwritting the ruleset on shutdown and the attacker). As the attacker already needs a foothold on the system and has to be authenticated (but not privileged) I have rated this as medium. @security, can you please check the attached GLSA propossal (it's my first time so it may be full of issues) and issue it. I'm unsure though if you need to wait for stabilization of 0.9.0-r5 or not.
Stabilization done, @security if you need a template for a GLSA handling this issue see the attached one, if you think a GLSA is not necessary just close this bug.
(In reply to Francisco Blas Izquierdo Riera from comment #7) > Stabilization done, @security if you need a template for a GLSA handling > this issue see the attached one, if you think a GLSA is not necessary just > close this bug. Thanks, Francisco. I am rating this a B3 as even modifying the ruleset would only allow remote access from a user, but still requires authentication. Please drop the vulnerable versions.
(In reply to comment #4) > https://gitweb.gentoo.org/repo/gentoo.git/commit/ > ?id=73598a5e25d6583dde4f08a34df5073817c5a391 > > commit 73598a5e25d6583dde4f08a34df5073817c5a391 > Author: Francisco Blas (klondike) Izquierdo Riera <klondike@gentoo.org> > AuthorDate: 2019-09-07 20:38:38 +0000 > Commit: Matthew Thode <prometheanfire@gentoo.org> > CommitDate: 2019-09-07 21:24:11 +0000 > > net-firewall/nftables: Fix permissions for rules.save > > Due to a bug, the rules.save file was created with the wrong > permissions which allowed all users to read the file with the > system rules although root privileges are usually required to > do so. > > To fix this issue, the following measures have been taken: > * The umask on nftables-mk.sh is now correctly set to 177 > * nftables.sh now also sets the umask before saving the rules > * The ebuilds will warn on post installation if the rules.save > has insecure permissions Changes in pkg_postinst() are not fully correct. When net-firewall/nftables is installed the first time, /var/lib/nftables/rules-save does not exist at the beginning of pkg_postinst(), and the following code is run: if [[ ! -f "${save_file}" ]]; then touch "${save_file}" This creates /var/lib/nftables/rules-save with standard permissions 0644. When rebuilding net-firewall/nftables, the following code is run: elif [[ $(( "$( stat --printf '%05a' "${save_file}" )" & 07177 )) -ne 0 ]]; then ewarn "Your system has dangerous permissions for ${save_file}" ewarn "It is probably affected by bug #691326." ewarn "You may need to fix the permissions of the file. To do so," ewarn "you can run the command in the line below as root." ewarn " 'chmod 600 \"${save_file}\"'" Please fix ebuilds to not print warnings in situation when it is not needed.
Regarding this code: if [[ ! -f "${save_file}" ]]; then touch "${save_file}" I would suggest just writing it as: ( umask 0177; touch -a "${save_file}" ) Doing so would address the issue of the mode being incorrect in the case that touch is charged with creating the file. It also does away with the explicit race condition. It's still not atomic but it's as close as one can reasonably get in this situation. Naturally, it could be annoying for the mtime of any existing file to be updated every time that pkg_postinst is executed, but just updating the access time with -a should be acceptable. Further, nothing prevents you from then proceeding to check the mode anyway. I have to wonder why the check even exists though. Portage already exhibits a very high signal-to-noise ratio in general, and such messages do stand a good chance of being overlooked in practice. Further, the nftables runscript always coerces a mode of 0600 in its save function anyway. So, unless the user is expected never to use the save function to write out rules-save - which seems rather unlikely - it's a little odd to pretend that they have a choice that needs to be deferred to, isn't it? I can think of a few options that might make more sense … The simplest - and most aggressive - would be to just double down on enforcing a mode of 0600. For instance, don't just fix it in as part of the stop path in the runscript, but in start_pre and wherever else. In that case, it would more sense to fix it unconditionally in pkg_postint - if at all - rather than engaging in digital hand-wringing. Another option might be to only take action if the "all users" bits are open in some way. For example: if (( $(stat --printf %05a "${save_file}") & 00007 != 0 )); then chmod o-rwx "${save_file}" # or perhaps just whine about it fi # Otherwise, just assume the current mode is what the user wants and leave # things alone. The assigned group will be root in most cases. If not, this # rare user might intend for a particular group to have read rights. I think this would strike a reasonable balance between taking corrective action and respecting the user's wishes. Yet another option would be to allow the user to define the desired mode in the conf.d file, which otherwise defaults to 0600, and to rigorously enforce it. In any case, whatever corrective action the ebuild may take, I reckon that it's important that it be consistent with the policy of the runscript. If the runscript is to continue enforcing 0600 then it should probably do so more aggressively. Or perhaps the constraint could be relaxed as mentioned above. Either way, let's try to make sure that they are both on the same page.
Hi Arfrever, This one went under my radar. Sorry. Next time it would be easier if you opened a new bug instead as this one is assigned to security@ Hi Kerin, Although starting a subshell is ugly the umask solution is the way to go as it prevents race conditions (which could be problematic if somebody ran the scripts with broken umasks for some reason). We don't want to make the mtime change in most circumstances so the check will remain there. Any other solutions are problematic. Config file protection doesn't work for permission changes and user may want to keep permissions different for whichever their reason is. In such a case I'm expecting them to be smart enough to handle setting their desired umask (and probably owner and group for the file) by modifying the scripts. Keep also in mind that administrators may use only the load/unload features and not the table saving ones (which they may defer somewhere else). I think this addresses your concerns. Thanks a lot for taking the time to share them :) I'll make a pull request and get it up later today.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=e9d9a46d5115e5c75085f335bded4badbce05673 commit e9d9a46d5115e5c75085f335bded4badbce05673 Author: Francisco Blas (klondike) Izquierdo Riera <klondike@gentoo.org> AuthorDate: 2019-12-02 18:27:29 +0000 Commit: Matthew Thode <prometheanfire@gentoo.org> CommitDate: 2019-12-02 18:51:45 +0000 net-firewall/nftables: Touch rules-save with right umask The nftables ebuild contains code to ensure the rules-save file is created so the service will start on systemd based systems. The current code creates the file with default permissions 644 which triggers the code for detecting misconfigured system added to address bug #691326 Instead of just using touch, start a subshell so we can call umask beforehand and address the issue. Bug: https://bugs.gentoo.org/691326 Signed-off-by: Francisco Blas Izquierdo Riera (klondike) <klondike@gentoo.org> Package-Manager: Portage-2.3.76, Repoman-2.3.11 Signed-off-by: Matthew Thode <prometheanfire@gentoo.org> net-firewall/nftables/nftables-0.9.0-r5.ebuild | 2 +- net-firewall/nftables/nftables-0.9.1-r1.ebuild | 2 +- net-firewall/nftables/nftables-0.9.2-r1.ebuild | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=c462d2e51ff55918629a7df082d8d1310a83b7ca commit c462d2e51ff55918629a7df082d8d1310a83b7ca Author: Francisco Blas (klondike) Izquierdo Riera <klondike@gentoo.org> AuthorDate: 2019-12-02 18:24:50 +0000 Commit: Matthew Thode <prometheanfire@gentoo.org> CommitDate: 2019-12-02 18:51:44 +0000 net-firewall/nftables: Drop vulnerable ebuild Drop the nftables-0.9.0-r4 ebuild which is affected by the permission handling bug as all stable arches can now use -r5 instead. Bug: https://bugs.gentoo.org/691326 Signed-off-by: Francisco Blas Izquierdo Riera (klondike) <klondike@gentoo.org> Package-Manager: Portage-2.3.76, Repoman-2.3.11 Signed-off-by: Matthew Thode <prometheanfire@gentoo.org> net-firewall/nftables/nftables-0.9.0-r4.ebuild | 97 -------------------------- 1 file changed, 97 deletions(-)
A fix for the unnecessary warning has been deployed to the portage tree. As the last vulnerable version of the ebuild has also been removed I'm closing this bug. Please open a new one if you find other issues.
(In reply to Francisco Blas Izquierdo Riera from comment #11) > Hi Kerin, > > Although starting a subshell is ugly the umask solution is the way to go as > it prevents race conditions (which could be problematic if somebody ran the > scripts with broken umasks for some reason). We don't want to make the mtime > change in most circumstances so the check will remain there. > Hi Francisco. Well, you can't prevent this kind of race condition in bash without employing some kind of mutually agreed-upon locking scheme (even set -C isn't good enough). It's the elimination of the scripted file test that goes the furthest in terms of making it less racy. Spawning a subshell is merely a sensible and effective way of scoping a umask change. Aesthetic sensibilities aside, there's nothing ugly about it. As for the mtime, perhaps it was lost in translation but the option that I mentioned guarantees that both the mtime and ctime remain unmodified. GNU touch provides two ways, in fact:- touch -a touch --time=atime In short, the code addresses half of Arfrever's complaint and is otherwise more reliable. Checking the mode need not be contingent upon an earlier file-existence test either. You could just check it anyway, and reasonably assume that the file doesn't exist if stat raises an error. if mode=$(stat --printf %05a "${save_file}" 2>/dev/null) && (( mode & ... != ... )); then ewarn "You'd better read this ..." fi Again, it does not eliminate the possibility of a race but it reduces the likelihood thereof. Besides, you _need_ for stat to succeed for the arithmetic expression not to raise an error, whether it be at the behest of my coding style or yours. Otherwise, there's always the marginal risk of stat printing nothing to STDOUT, leading to errors such as the following. # (( $(:) & 00007 != 0 )) -bash: ((: & 00007 != 0 : syntax error: operand expected (error token is "& 00007 != 0 ") # [[ $(( "$(:)" & 07177 )) -ne 0 ]] -bash: & 07177 : syntax error: operand expected (error token is "& 07177 ") > Any other solutions are problematic. Config file protection doesn't work for > permission changes and user may want to keep permissions different for > whichever their reason is. In such a case I'm expecting them to be smart > enough to handle setting their desired umask (and probably owner and group > for the file) by modifying the scripts. Keep also in mind that > administrators may use only the load/unload features and not the table > saving ones (which they may defer somewhere else). > > I think this addresses your concerns. Thanks a lot for taking the time to > share them :) Not entirely, but I appreciate the response, especially as I was not expecting one. So, as nit-picky as I may seem, thank you for that!
Hi Kerin! The funny thing with this bug is that when I saw it I just said something like "It's missing the umask before the touch, I'll handle the PR another day since today is too late" Sadly I forgot about it and had you not raised your point I would have completely forgotten about it. So thanks for your comment or I'd have forgotten about this. I know that -a just modifies the access time, but I'm not sure that behavior will work as expected on other POSIX systems, hence why we were using plain touch until now. It will modify the modification time but this is fine if a race condition arises there. Let's see how the new code works from a race condition perspective: if [[ ! -f "${save_file}" ]]; then ( umask 177; touch "${save_file}" ) elif [[ $(( "$( stat --printf '%05a' "${save_file}" )" & 07177 )) -ne 0 ]]; then ewarns ... fi You could say (with good reason) that the file might be created between the check and the umask+touch. This will have the effect of changing the modification time which shouldn't be significantly off from the original one (in the vast majority of cases as the window is fairly small). OTOH this solves the permission problem (using umask) and prevents permissions to be accidentally changed if something happens. There is also the chance that the first test passes and the file is then removed before running the stat but that should be fine as it will then fail (and most likely the ebuild would then fail or the if wouldn't be executed). I could do a double check in the second if but that would make the code significantly more complicated. It wouldn't change anything eitherway as your system would be probably botched up by this (at least if you use systemd) :( If we are concerned about stat printing nothing we could prefix a 0 to the left side of the string, but I think this will be a bit overkilling as that code is already complex enough (which may hint at the fact that an eclass to do this kind of permission checks may not be a bad idea). For now though I will leave the fix as is and see if somebody hits either of the race conditions. As I said. Thanks a lot for taking the time to share your concerns and sharing your input. If there is anything Gentoo needs are helpful users like you.
