On Sunday 18 March 2018 06:40 PM, Gero Treuner wrote:
[..]
>> + // fix docroot
>> + if (uphp.docroot) {
>> + char *orig_docroot = uphp.docroot;
>> + uphp.docroot = uwsgi_expand_path(uphp.docroot,
>> strlen(uphp.docroot), NULL);
>> + if (!uphp.docroot) {
>> + uwsgi_log("unable to set php docroot to %s\n",
>> orig_docroot);
>> + exit(1);
>> + }
>> + uwsgi_log("PHP document root set to %s\n", uphp.docroot);
>> + uphp.docroot_len = strlen(uphp.docroot);
>> + }
>> +
>
> It looks very much like a sanity check about whether the path really
> exists. The effect of uwsgi_expand_path(...) is to call realpath()
> from libc .
>
> After closer checks I found this:
>
> (1)
> The section was introduced by
>
> https://github.com/unbit/uwsgi/commit/17c8fff794e8f0c1822ab27f7232ce7a93c9703a#diff-dcbc6e6f5e54a35fc03537a0a660d01b
> This is monster patch... Together with the parent commit I don't see a
> direct relation to a functional change -> still looks like a sanity
> check.
>
> (2)
> realpath() is also called later on in uwsgi_php_request(), so indicated
> by name based on requests (not initialization).
>
> Concluding this it changes uwsgi in a way that invalid paths are
> reported earlier at initialization time. Could be too much change for a
> security patch ;-)
>
> As alternative by not touching uphp.docroot the section should be this:
>
> if (uphp.docroot) {
> uphp.docroot_len = strlen(uphp.docroot);
> }
>
> The other code in php_plugin.c looks like safely accepting this in case
> of not perfectly valid paths. But I'd feel better with the early check.
>
>
But I guess similar can be done by `php-docroot = /some/path` in config
file or I can be completely wrong.
I will test your patch today itself.
Thanks,
--abhijith
