[pacman-dev] [PATCH] libalpm: Check cachedir access before re-creating

[Eli Schwartz]

On 2/4/19 8:30 AM, Andrew Gregory wrote:
> On 01/23/19 at 10:52pm, David Phillips wrote:
>> If users have mounted a filesystem such as sshfs or NFS that doesn't
>> allow root access by default for security reasons, _alpm_filecache_setup
>> will blindly try and re-create the components of the cachedir path,
>> eating EACCES (to be liek mkdir -p). Then, it will return this path as
>> if it is valid when it may not be readable by root, causing
>> alpm_check_downloadspace to trigger a FPE since it assumes the cachedir
>> is R/W and uses fsinfo which may be zero.
>>
>> This patch adds a check before trying to re-create the cachedir in order
>> to determine if creating it will result in EACCES, and outputs a warning
>> if so. It also discards the current cachedir from being considered. This
>> causes a more meaningful and graceful failure than a FPE.
>> ---
>>  lib/libalpm/util.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
>> index d33eef2a..d4375593 100644
>> --- a/lib/libalpm/util.c
>> +++ b/lib/libalpm/util.c
>> @@ -849,12 +849,16 @@ const char *_alpm_filecache_setup(alpm_handle_t *handle)
>>  	for(i = handle->cachedirs; i; i = i->next) {
>>  		cachedir = i->data;
>>  		if(stat(cachedir, &buf) != 0) {
>> -			/* cache directory does not exist.... try creating it */
>> -			_alpm_log(handle, ALPM_LOG_WARNING, _("no %s cache exists, creating...\n"),
>> -					cachedir);
>> -			if(_alpm_makepath(cachedir) == 0) {
>> -				_alpm_log(handle, ALPM_LOG_DEBUG, "using cachedir: %s\n", cachedir);
>> -				return cachedir;
>> +			if (errno == EACCES) {
>> +				_alpm_log(handle, ALPM_LOG_WARNING, _("failed to stat %s: %s\n"), cachedir, strerror(errno));
>> +			} else {
>> +				/* cache directory does not exist.... try creating it */
>> +				_alpm_log(handle, ALPM_LOG_WARNING, _("no %s cache exists, creating...\n"),
>> +						cachedir);
>> +				if(_alpm_makepath(cachedir) == 0) {
>> +					_alpm_log(handle, ALPM_LOG_DEBUG, "using cachedir: %s\n", cachedir);
>> +					return cachedir;
>> +				}
> 
> It's been quite a while since I used sshfs or NFS, so maybe I'm
> missing something, but this doesn't make a lot of sense to me.  Does
> makepath really succeed if we don't have access rights?  I would have
> thought that would already fail, causing the next directory to be
> tried.  Second, why single out EACCESS as the exception?  Of all the
> reasons stat can fail, ENOENT is really the only one that makes sense
> for us to try to create the directory.  Where is the FPE actually
> occurring?  That should be fixed directly, because it could still be
> triggered if our access rights change between selecting the cachedir
> and actually using it.

lib/libalpm/diskspace.c:403 would be a divide-by-zero, right after the
comment:

/* there's no need to check for a R/O mounted filesystem here, as
* _alpm_filecache_setup will never give us a non-writable directory */

-- 
Eli Schwartz
Bug Wrangler and Trusted User

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1601 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20190204/c9ed3542/attachment.asc>