Re: April report
On 2017-04-20 08:08:50, Brian May wrote:
> Antoine Beaupré <anarcat@orangeseeds.org> writes:
>
>> On 2017-04-19 19:05:36, Brian May wrote:
>>
>> [...]
>>
>>> As I have run out of hours this month, if anybody else wants to take
>>> over either of these, please let me know and I will provide more
>>> details.
>>
>> I'd take a look at the XBMC thing...
>
> The webserver is in xbmc/network/WebServer.cpp, see AnswerToConnection()
>
> 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.
> 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 don't see any sanity checking of the URL, at any stage.
Clearly, that's something that never crossed the author's mind.
Even the configured username/password protection is bypassed here, which
just boggles my mind. At *least* that should work...
> CreateFileDownloadResponse is in the same file. It opens a CFile to the
> URL, reads it, and sends it.
boom. :)
> CFile stuff is in xbmc/filesystem/File.cpp. CFile in turn passes
> everything to CFileFactory:
>
> CFileFactory::CreateLoader(url), which is in
> xbmc/filesystem/FileFactory.cpp
I just love reading the XBMC source code... Next is a URL factory? :p
> 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.
> For case (a) protocol used in the exploit is "special" - this comes from
> the untrusted user supplied URL - now stripped of /vfs/ - and can be
> anything. As such we use CFileSpecialProtocol. (or any other possible
> protocol). For case (b) (starting with '/') protocol is empty and uses
> CFileHD (not to be confused with CFile which we already used).
>
> Case (a) CFileSpecialProtocol just reads the file, and same with case
> (b) CFileHD just reads the file.
>
> ./xbmc/filesystem/FileSpecialProtocol.cpp
> ./xbmc/filesystem/FileHD.cpp
>
>
> Somewhere for case (a) there must be decoding of the special characters
> in the URL. I am not sure where this decoding takes place. In my scan of
> the source code I think I missed it. Might need to attach a debugger and
> double check what I have said, plus see where the decoding happens.
Maybe this is in CUtil::ValidatePath(), called early in URL::Parse()?
> I am speculating CURL might be the best place to strip "../" sequences
> from the file name, however this really depends on where the above
> decoding takes place.
Problem is "../" doesn't matter at this point - the user can just send
arbitrary absolute paths and kodi just happily gobbles it up. First step
is to make paths relative...
The other problem is that we use that generic CURL interface which is
used internally to refer to media files and whatnot. It seems like
handling path sanitization there could break unrelated things. We need
to fix things upstream, before we pass paths into CURL, in the webserver
directly.
I would sanitize paths there. Maybe through a helper function in
Util.cpp, since there are already path sanitization functions there...
But then *how* do you sanitize this? In the original advisory, Kodi uses
this interface to load thumbnails, _using an absolute path_! If we make
this relative without fixing the caller, we just break thumbnails, and
therefore a significant feature of the web interface (you know,
images).
I may be wrong here, but it seems to me we not only need to fix those
paths to be relative and without path escapes (after entities parsing,
mind you), but we also need to fix *all* possible callers.
This is one nasty horrible bug, if you ask me.
For the record, I haven't *quite* figured out how to extract the data
from my own Kodi instance at home, running 16.1 from backports. The /vfs
trick doesn't work, nor the /image/image trick from the advisory - but
god knows what's possible at this point...
I'll rest my case for tonight and sleep on this one, hopefully, some
brilliant idea will come up tomorrow.
A.
--
Quidquid latine dictum sit, altum sonatur.
Whatever is said in Latin sounds profound.
Reply to: