Improve path traversal issue detection with proper nginx conf parser

[alexAubin]

Follow-up of https://github.com/YunoHost/package_linter/pull/61 ... this is a significant improvement in the detection of path traversal issue in nginx confs (the previous method was quite dirty and unreliable)

To do this, I used the small nginxparser lib from https://github.com/fatiherikli/nginxparser which in fact was used / reworked / fixed by the certbot project.

So far the package_linter was quite simple and did not require dependencies but for this to work, we actually need to pip install pyparsing (maybe there's an apt package, idk). Sooo to be seen how we actually integrate this dependency thing in package_check or for user running the linter manually

[maniackcrudelis]

https://github.com/YunoHost/package_check/blob/master/sub_scripts/testing_process.sh#L382-L385

[alexAubin]

Hmmm that looks cool but :

• Nextcloud seems to be vulnerable to path traversal issue and I don't see the message in last CI check ? Is it understood ?
• This doesn't seem to cover trickier case where the alias doesn't point to /var/www/$app/ and yet could still be an issue if secrets are store in $final_path/..

[maniackcrudelis]

Nextcloud isn't vulnerable, because it doesn't use a simple location. phpsysinfo_ynh is vulnerable to it. By the time this issue appeared, we had detect many vulnerable apps, but I retried with both nextcloud and phpsysinfo and couldn't get a file elsewhere than $final_path. I think nginx has been fixed with the version used in stretch. Even though I can't find anything about it on https://nginx.org/en/security_advisories.html

Anyway, before popping new errors on a lot of apps, we should be sure this is still relevant.

[alexAubin]

Well indeed I have a hard time reproducing the issue on some apps which appeared to be vulnerable ... yet the app i'm testing almost has a configuration identical to phpsysinfo's with which i am able to reproduce the issue (so I don't think nginx has been fixed upstream)

Will investigate further

[alexAubin]

Hmmm so an important ingredient seems to be that, in order for the path traversal to work, the alias folder must not end with a / :

e.g.

alias /var/www/foo;

Since Nextcloud's alias does end with a / it is indeed not vulnerable : https://github.com/YunoHost-Apps/nextcloud_ynh/blob/master/conf/nginx.conf#L9

Edit : uh wait no, that's the opposite ... the path traversal issue can happen only if the alias path does end with / ?

[maniackcrudelis]

And so, how did you reproduce ?

[alexAubin]

And so, how did you reproduce ?

(c.f. discussion on nextcloud_ynh ;)

I pushed a few more commits to improve the detection ...

• I chose to consider that the path used after alias is "bad" if it does not contains __FINALPATH__ as it probably means that the user ain't using the nginx helper ... (c.f. stuff like https://github.com/YunoHost-Apps/rainloop_ynh/blob/80cce268248d4d34b2ad1671ee31d3403f9262c6/scripts/install#L139 where ALIASTOCHANGE is replaced with something ending with /)
• Since the detection should be more reliable now, I propose to promote this detection to an error instead of just a warning because .. well it's scary :| ... and app passing level 5 should be at least "secure" against that kind scary of stuff (c.f. other things like the use of rm -rf)

[maniackcrudelis]

I really don't think we should rely to a parsing of the nginx conf to consider that there's indeed a alias traversal issue. I'm working on package check to fix the detection, and I'd rather prefer to actually try to use that security issue to prove that the package is vulnerable instead of suppose it by parsing the nginx conf.

[maniackcrudelis]

Alias traversal detection is now fixed by https://github.com/YunoHost/package_check/commit/a2dde3bd434fe0faa4aaf2914b4e966126a3554e, and use its own html file, so the title won't change...

This detection isn't related to any level, it can be changed though.

[alexAubin]

Well it took me a whole day to work on this but okay :sweat_smile:

So I guess yup, we can keep it as a warning only, but what I'd really like is to have the path traversal test to be part of level 5 ... I guess it's somewhat doable to integrate the CI check in the level definition :stuck_out_tongue_winking_eye: ?

More generally on the long term, I wish we could integrate anything that could be a security / integrity concern. The goal being to increase the minimum level requirement that we apply during yunohost app install to 5 such that :

• we can really say to people that "if there's no warning, the app is kinda safe to install" (and oppositely if it's not level 5, it's not safe ...)
• this in turn encouraging people to at least have no error in package linter and respect at least the basic good practices if they want people to actually use their app

(But first I guess we gotta see how all the recent change on the apps level impact the levels already)

[maniackcrudelis]

I know you're working hard on package linter since a while now. But I have to admit that I still don't trust it. Considering a minimum level, I'm even in favor of showing by default only apps that are at least level 7. (With the new definition of levels)

Sorry for the time you spent of that very issue, but I really think we should put our trust in nginx to see if it handles that issue correctly instead of a parsing of the conf file. Without really knowing how nginx will interpret it. That's why I'm more in favor of a real test of the issue to know whether or not we can grab a file out of the scope of the app.

In package check, this test fills a variable, which can be used to validate any level we want. So it can be level 5, with the linter.

[alexAubin]

Superseded by other PR