Re: Patch for CVE-2018-7490 in uwsgi
Hi Abhijith,
On Sun, Mar 18, 2018 at 05:13:53PM +0530, Abhijith PA wrote:
> Thanks for the patch :)
Thanks for looking at it :-)
> It look like ( and also you mentioned) you have added following lines
> from master branch. But I don't see the point of doing these other
> than that the upstream patch applies perfectly now. Can you provide
> little more information.
>
> + // 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.
Kind regards,
Gero
Reply to: