Missing check for is_dir when listing apps. #17400

Closed
nougad opened this Issue Jul 5, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@nougad

nougad commented Jul 5, 2015

Some distros include non directories in app dir (like arch: https://www.archlinux.org/packages/community/any/owncloud/):

/usr/share/webapps/owncloud/apps/issue_template.md 
/usr/share/webapps/owncloud/apps/MAINTAINERS.md 
/usr/share/webapps/owncloud/apps/build.xml 
/usr/share/webapps/owncloud/apps/CONTRIBUTING.md 
/usr/share/webapps/owncloud/apps/README 

In file/line: https://github.com/owncloud/core/blob/master/lib/private/app.php#L782

a check for is_dir is missing:

-if ($file[0] != '.' and is_file($apps_dir['path'] . '/' . $file . '/appinfo/info.xml')) {
+if ($file[0] != '.' and is_dir($apps_dir['path'] . '/' . $file) and is_file($apps_dir['path'] . '/' . $file . '/appinfo/app.php')) {

this leads to error messages if open_basedir is used in php.ini:

Jul 05 16:17:28 owncloud ownCloud[12709]: {PHP} is_file(): open_basedir restriction in effect. File(/usr/share/webapps/owncloud/apps/issue_template.md/appinfo/app.php) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/:/etc/webapps/:/srv/owncloud/data) at /usr/share/webapps/owncloud/lib/private/app.php#776 
Jul 05 16:17:28 owncloud ownCloud[12709]: {PHP} is_file(): open_basedir restriction in effect. File(/usr/share/webapps/owncloud/apps/MAINTAINERS.md/appinfo/app.php) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/:/etc/webapps/:/srv/owncloud/data) at /usr/share/webapps/owncloud/lib/private/app.php#776 
Jul 05 16:17:28 owncloud ownCloud[12709]: {PHP} is_file(): open_basedir restriction in effect. File(/usr/share/webapps/owncloud/apps/build.xml/appinfo/app.php) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/:/etc/webapps/:/srv/owncloud/data) at /usr/share/webapps/owncloud/lib/private/app.php#776 
Jul 05 16:17:28 owncloud ownCloud[12709]: {PHP} is_file(): open_basedir restriction in effect. File(/usr/share/webapps/owncloud/apps/CONTRIBUTING.md/appinfo/app.php) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/:/etc/webapps/:/srv/owncloud/data) at /usr/share/webapps/owncloud/lib/private/app.php#776 
Jul 05 16:17:28 owncloud ownCloud[12709]: {PHP} is_file(): open_basedir restriction in effect. File(/usr/share/webapps/owncloud/apps/README/appinfo/app.php) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/:/etc/webapps/:/srv/owncloud/data) at /usr/share/webapps/owncloud/lib/private/app.php#776 

Easiest way to reproduce is calling /usr/share/webapps/owncloud/occ app:list

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Jul 5, 2015

Member

This seems to be a packaging problem on the distribution side. This files are not part of the official ownCloud zip and tar files. It seems that they somehow put other git repositories directly into the apps folder.
Sorry but this is not how it works. The official tar and zip files should be used to guarantee the optimal quality.
Closing because of not an ownCloud bug.

Member

karlitschek commented Jul 5, 2015

This seems to be a packaging problem on the distribution side. This files are not part of the official ownCloud zip and tar files. It seems that they somehow put other git repositories directly into the apps folder.
Sorry but this is not how it works. The official tar and zip files should be used to guarantee the optimal quality.
Closing because of not an ownCloud bug.

@karlitschek karlitschek closed this Jul 5, 2015

@nougad

This comment has been minimized.

Show comment
Hide comment
@nougad

nougad Jul 5, 2015

Yes, you're right. But don't you think adding this safety guard is a question of code quality?

nougad commented Jul 5, 2015

Yes, you're right. But don't you think adding this safety guard is a question of code quality?

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Jul 5, 2015

Member

Wow, that is a weird PHP 'feature'. is_file() will cause an open_basedir warning if the path doesn't exist but part of the path is a path to an existing file. E.g. /etc/hosts/foo will cause the warning, while /etc/foo will not (assuming /etc/foo doesn't exist but /etc/hosts does).

@karlitschek I can also see why some distributions might put extraneous files in the apps directory. The fix seems clean enough and makes sense. @nougad please submit a PR 😄

Member

Xenopathic commented Jul 5, 2015

Wow, that is a weird PHP 'feature'. is_file() will cause an open_basedir warning if the path doesn't exist but part of the path is a path to an existing file. E.g. /etc/hosts/foo will cause the warning, while /etc/foo will not (assuming /etc/foo doesn't exist but /etc/hosts does).

@karlitschek I can also see why some distributions might put extraneous files in the apps directory. The fix seems clean enough and makes sense. @nougad please submit a PR 😄

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Jul 5, 2015

Member

Sure. Everyone is welcome to work on that obviously. I personally think that we should first fix bug that exist in the real ownCloud release before we fix frankenstein unofficial ownClouds.
But this is just me of course.

Member

karlitschek commented Jul 5, 2015

Sure. Everyone is welcome to work on that obviously. I personally think that we should first fix bug that exist in the real ownCloud release before we fix frankenstein unofficial ownClouds.
But this is just me of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment