On 2017-04-20 08:08:50, Brian May wrote: > Antoine Beaupré <[email protected]> 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.
