Re: Patch for CVE-2018-7490 in uwsgi

[Abhijith PA <abhijith@disroot.org>]

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