While experimenting with user patch support, I discovered that portage can silently fail to apply patches that are contained in a path that cannot be traversed by portage's uid. Consider the following:- # pwd /etc/portage/patches/sys-kernel # readlink vanilla-sources /usr/src/git/linux-patches/4.9 # stat -c%a /usr/src 700 In this instance, I initially overlooked that the mode of the /usr/src directory had previously been tightened. Obviously, with /usr/src being owned by root and a mode of 700, the path could not be traversed by the portage user. My expectation would be that portage throws an exception because any attempt to descend into the sys-kernel/vanilla-sources path would have resulted in EACCES. Instead, it proceeds as if nothing were wrong and (obviously) never ends up applying any of the patches.
Just to add, this is with portage 2.3.8. # emerge --version Portage 2.3.8 (python 3.4.5-final-0, hardened/linux/amd64, gcc-5.4.0, glibc-2.23-r4, 4.9.56 x86_64)
Thanks, Zac. The attached patch changes epatch_user() so that it bails out on any EPATCH_SOURCE path that is either an unreadable directory or an unreadable symlink, as well as improving the legibility of the code (IMO). As such, it addresses my test case. Despite my better instinct, I've maintained the unsafe variable expansions so as to adhere to the existing coding style and to keep the patch focused on the issue at hand. Eclasses appear to have a lot of bad code but I'll save it for another time. The error message is meaningful but doesn't come across very clearly in the extremely verbose exception and stack trace. I'll leave it to someone else to make it more user-friendly, if possible.
Created attachment 498816 [details, diff] gentoo-bug-634396.patch Have epatch_user() die where a prospective EPATCH_SOURCE path cannot be read, provided it is either a directory or a symlink.
Created attachment 498820 [details, diff] gentoo-bug-634396-r1.patch Same as previous patch, except the three expansions containing $EPATCH_SOURCE are quoted, making it as safe as the original code (assignment was used originally, thus relaxing the quoting requirements).
This is still a potential issue. Any comments?
I think the best that you can do here is to prove that some file or directory exists which you should have access to but do not. I would start by defining a function that's designed to search for this type of case in a particular location, possibly using the find command.
(In reply to Zac Medico from comment #6) > I think the best that you can do here is to prove that some file or > directory exists which you should have access to but do not. I would start > by defining a function that's designed to search for this type of case in a > particular location, possibly using the find command. This constitutes neither a meaningful review/critique of my patch, nor does it explain how find(1) would, in any way, be a relevant tool in the course of addressing the issue. As such, I don't see how I'm supposed to respond other than to stand my ground. The loop touched by the patch is responsible for iterating over a set of pathnames which: 1) may or may not be directories 2) may or may not contain patches Said loop is directly responsible for the first consideration, whereas the epatch routine is responsible for the second. So, how is the first consideration attended to? By carrying out a solitary -d test, and therein lies the problem. For a -d test to be false does not prove that the given pathname isn't a directory. Therefore, it is insufficient as a means of silently eliminating a pathname as a potential patch-containing directory. The patch addresses this issue in the simplest and most direct manner possible, which is by augmenting the existing test with an additional one. To whit, if the given pathname is provably a directory OR a symlink, AND if it cannot be proven to have the 'r' bit set in the file mode, throw an exception. This comprises two cases in which an exception may be thrown: -d $path && ! -r $path: provably a directory but not (readable or traversible) -L $path && ! -r $path: provably a symlink but not ((symlink traversible) or (target readable)) It should be apparent to anyone that understands how unix permissions work that both both of these cases are ones in which an arbitrary pathname may NOT be safely eliminated as a potential patch-containing directory. Therefore, to raise an error is the appropriate thing to do.
I should add that these tests were implemented in such a way that takes into consideration the fact that we cannot discern the native errno in bash, where a syscall fails. Fortunately, it is not a significant impediment, given the scope of the problem.
(In reply to Kerin Millar from comment #7) > The patch addresses this issue in the simplest and most direct manner > possible, which is by augmenting the existing test with an additional one. > To whit, if the given pathname is provably a directory OR a symlink, AND if > it cannot be proven to have the 'r' bit set in the file mode, throw an > exception. This comprises two cases in which an exception may be thrown: > > -d $path && ! -r $path: provably a directory but not (readable or > traversible) > -L $path && ! -r $path: provably a symlink but not ((symlink traversible) or > (target readable)) > > It should be apparent to anyone that understands how unix permissions work > that both both of these cases are ones in which an arbitrary pathname may > NOT be safely eliminated as a potential patch-containing directory. > Therefore, to raise an error is the appropriate thing to do. If you can translate this into a reusable function, then we can re-use it elsewhere, and the documentation can include an explanation of these implementation details. It would also be interesting to have a unit test function, if possible (which may require root privileges + privilege dropping).
(In reply to Zac Medico from comment #9) > If you can translate this into a reusable function, then we can re-use it > elsewhere, and the documentation can include an explanation of these > implementation details. It would also be interesting to have a unit test > function, if possible (which may require root privileges + privilege > dropping). Fair enough. I'll take a look at doing that.
Does this apply to Portage's internal implementation of eapply_user (not epatch_user), or is it exclusive to the now-banned epatch eclass? (Wondering if I should close or reassign to Portage.)
(In reply to Sam James from comment #11) > Does this apply to Portage's internal implementation of eapply_user (not > epatch_user), or is it exclusive to the now-banned epatch eclass? Thanks for commenting. That's a good question. I'll try out some scenarios again soon.
