Re: April report
Antoine Beaupré <anarcat@orangeseeds.org> writes:
>> Case (a), URL prefixed with "/vfs", we return the result of
>> CreateFileDownloadResponse(). The first 5 characters are removed - so
>> if "/vfs/" prefixed it strips the entire prefix - but curously doesn't
>> check the 5th character. So /vfss would also get stripped. (what happens
>> if we pass only 4 characters??? "/vfs"?)
>
> Hmm... I am not sure, but it seems to me it takes the "right" part of
> the string, starting from the "fifth" character, so whatever comes after
> the "fourth" character, ie. after "/vfs". in my tests:
>
> http://localhost:8080/vfs/etc/passwd
>
> returns /etc/passwd... So just clean passthrough.
I don't think this can be correct. a URL looks like
http://192.168.122.47/vfs/special://masterprofile/Thumbnails/Video/f/auto-f4b8e6fd.tbn
Which on my system maps to:
/root/.xbmc/userdata/Thumbnails/Video/f/auto-f4b8e6fd.tbn
My believe is that the following code:
if (strURL.Left(4).Equals("/vfs"))
{
strURL = strURL.Right(strURL.length() - 5);
return CreateFileDownloadResponse(connection, strURL, methodType);
}
Will set strURL to:
"special://masterprofile/Thumbnails/Video/f/auto-f4b8e6fd.tbn"
As this starts with the format of protocol://file, CURL will decode that
as protocol=="special" and
file=="masterprofile/Thumbnails/Video/f/auto-f4b8e6fd.tbn".
Which will then parse it off to the SpecialProtocol handler.
(it is probably fine, but it always makes me nervous when I see the
protocol being passed like this - I guess it means every protocol could
have vulnerabilities that should be checked. Not sure if all protocols
should be accessible from the website either.)
As a result it may not be possible to pass an absolute path here. Not
checked the case of "special:///file" however.
I am guessing this CSpecialProtocol::TranslatePath function - which I
can't find right now - translates the root dir to
"/root/.xbmc/userdata/" on my system.
Just noticed these startup messages:
07:21:43 T:140043670366272 NOTICE: special://xbmc/ is mapped to: /usr/share/xbmc
07:21:43 T:140043670366272 NOTICE: special://xbmcbin/ is mapped to: /usr/lib/xbmc
07:21:43 T:140043670366272 NOTICE: special://masterprofile/ is mapped to: /home/brian/.xbmc/userdata
07:21:43 T:140043670366272 NOTICE: special://home/ is mapped to: /home/brian/.xbmc
07:21:43 T:140043670366272 NOTICE: special://temp/ is mapped to: /home/brian/.xbmc/temp
>> Case (b), any other static file (not ending with /), we fall down to
>> CreateFileDownloadResponse() also. In this case I believe the first
>> character of the URL is always '/'.
>
> Yeah, that's pretty much what I see as well, but I'm not sure how it
> works out in the end - those all give 404s:
>
> http://localhost:8080/temp/xbmc.log
> http://localhost:8080/.xbmc/temp/xbmc.log
> http://localhost:8080/etc/passwd
>
> so i'm not sure that's such a good vector.
I suspect these paths somehow map to
/usr/share/xbmc/addons/webinterface.default/ - not sure how this is done
however. For loading the *.css and *.js files for the website.
I tried to bring up xbmc to test this theory out, however it is not
obliging right now.
> I just love reading the XBMC source code... Next is a URL factory? :p
Yes, agreed.
>> The URL is parsed with CURL. xbmc/URL.cpp
>
> And here I was assuming this was cURL. silly me. would have given even
> amusing consequences though.
Same here, it confused me at first too.
> This is one nasty horrible bug, if you ask me.
Yes, agreed. I suspect it is going to require somebody who has a good
understanding of the code to fix it properly (as opposed to just fixing
some use cases).
I am now inclined the think that the correct fix might be the backend;
that is every backend. Maybe turn every filename into an absolute path
and check that it has the correct prefix (might be treaky for some
backends). If the checks fail have Open return False - looks like how
errors are handled (this code is self documented!). Not sure if this
might break other things.
I haven't checked recently if upstream have a fix (I doubt it) however I
am doubtful that they are going to be interested in fixing the old
version in Jessie or Wheezy.
--
Brian May <bam@debian.org>
Reply to: