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

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(-)

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.

apg

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 */