Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

webserver: make VFS handler more secure #1480

Merged
merged 3 commits into from Oct 4, 2012

Conversation

Projects
None yet
3 participants
Owner

Montellese commented Sep 26, 2012

These commits are meant to make the VFS handler of xbmc's webserver more secure. Currently it allows to access any file which xbmc can reach through its VFS including files like /etc/passwd etc because the only check that is made is whether the file exists or not.

With these commits every requested VFS path will be checked against the list of user-specified sources. If the path does not point to a file within a (sub-)directory of one of those sources a HTTP 401 (Unauthorized) error is returned. For this to work I had to write a method which would take care of ".." etc in paths because otherwise it would be possible to do something like

/path/to/source/../../../etc/passwd

and still access any file. I don't think the implementation is optimal but I couldn't find an existing method that does the same so I wrote URIUtils::GetRealPath() (and I also wrote a unit test for it).

IMO this is the best way to make the VFS handler more secure (short of ripping it out which I plan to do once there is a better way to access media files through the webserver). If someone adds the root path as a source it's his own fault. And if someone adds a symlink to the root directory (or somewhere else) in one of his source directories it's his own problem as well.

Member

elupus commented Sep 26, 2012

What about rars/zips? They have url encoded source in the host name field
and can be recursive.
On Sep 26, 2012 8:40 AM, "Sascha Montellese" notifications@github.com
wrote:

These commits are meant to make the VFS handler of xbmc's webserver more
secure. Currently it allows to access any file which xbmc can reach through
its VFS including files like /etc/passwd etc because the only check that is
made is whether the file exists or not.

With these commits every requested VFS path will be checked against the
list of user-specified sources. If the path does not point to a file within
a (sub-)directory of one of those sources a HTTP 401 (Unauthorized) error
is returned. For this to work I had to write a method which would take care
of ".." etc in paths because otherwise it would be possible to do something
like

/path/to/source/../../../etc/passwd

and still access any file. I don't think the implementation is optimal but
I couldn't find an existing method that does the same so I wrote
URIUtils::GetRealPath() (and I also wrote a unit test for it).

IMO this is the best way to make the VFS handler more secure (short of
ripping it out which I plan to do once there is a better way to access
media files through the webserver). If someone adds the root path as a
source it's his own fault. And if someone adds a symlink to the root
directory (or somewhere else) in one of his source directories it's his own

problem as well.

You can merge this Pull Request by running:

git pull https://github.com/Montellese/xbmc check_path_in_path

Or view, comment on, or merge it at:

#1480
Commit Summary

  • URIUtils: add GetRealPath() to handle . and .. in paths
  • [test] TestURIUtils: add test for GetRealPath()
  • webserver: only allow access to files which are in a (sub-)directory

File Changes

  • M xbmc/network/httprequesthandler/HTTPVfsHandler.cpp (39)
  • M xbmc/utils/URIUtils.cpp (41)
  • M xbmc/utils/URIUtils.h (2)
  • M xbmc/utils/test/TestURIUtils.cpp (41)

Patch Links

Owner

Montellese commented Sep 26, 2012

I don't have any archives so I don't really know how these work. Do you have an example. The same problem probably applies to stacked items but in that case the stacked files could just be accessed separately?

Member

elupus commented Sep 26, 2012

Rar://[url encoded fill path to rar file] /sub/dir/in/archive

Just grab any zip or rar navigate into it in xbmc and check log.

Owner

Montellese commented Sep 26, 2012

OK I pushed another commit which will handle rar:// and zip:// paths by using GetHostName() on the original path in the VFS handler. Furthermore I have extended URIUtils::GetRealPath() to also process the hostname and not only the file path.

I haven't tested this yet but will do later.

Owner

Montellese commented Sep 26, 2012

Confirmed working for zip and rar files.

Owner

Montellese commented Sep 27, 2012

@jmarshallnz I've taken care of all your comments i.e. added some doxy for URIUtils::GetRealPath(), removed those stupid whitespaces from the empty lines and added recursive behaviour in case of rar/zip paths within a rar/zip path (and also added a test for that in the unit test).

@ghost ghost assigned Montellese Oct 1, 2012

Owner

Montellese commented Oct 1, 2012

@jmarshallnz I've moved the CURL::Decode logic and IsInArchive check into GetRealPath as it only applies to rar/zip paths within a rar/zip path and therefore only needs to be done for filenames and not for hostnames.

Member

jmarshallnz commented Oct 1, 2012

Still not quite right - I think there's a misunderstanding on how rar/zip work (it may be me that's misunderstanding). I thought that a rar within a zip would look like:

rar://<url_encoded_path_to_rar_within_zip>/path/inside/rar

i.e.

rar://encode(zip://encode(/path/to/zip.zip)/path/in/zip/to/rar.rar)/path/inside/rar

That is, the protocol is the inner-most rar/zip. The hostname is the encoded path to the innermost rar/zip, and as you recurse in, you're going back "up" the hierarchy.

Thus, you never have to decode/encode the filename portion of the URL - only the hostname. So something like:

CURL url(path)
if (IsInArchive(path))
{
// we need to check the hostname as that's the "parent" rar/zip file
std::string archive = CURL::Decode(url.GetHostName());
url.SetHostName(CURL::Encode(GetRealPath(archive));
}
// finally process the filename
url.SetFileName(GetRealPath(url.GetFileName()));
return url.Get();

Member

elupus commented Oct 1, 2012

@jmarshallnz code seem almost right. But if i remember correctly you don't need to decode the return from GetHostName(), it's already decoded properly. But you do need to recurse it.

while(IsInArchive(path))
path = CURL(path).GetHostName();

should be enough to get to the root archive file which is what should decide access.

Owner

Montellese commented Oct 1, 2012

I thought it would be rar://< encode(/path/to/rar.rar) >/< encode(zip://< encode(/path/to/rared/zip.zip) >) > (which is why I applied it to the filename/path) but I don't really know either. I'm sure @cptspiff can shed some light on this.

Owner

Montellese commented Oct 1, 2012

@elupus The recursing happens by calling GetRealPath recursively. I first tried without the calls to Decode and Encode and then it didn't work so I added them. The problem is that you get a double-encoding for an "embedded" rar/zip URL: one to encode the whole "rar://" path and one encoding for the actual path to the rar so. GetHostName() takes care of the first encoding but not of the second.

Member

elupus commented Oct 1, 2012

Then something is very wrong. There should be no double encoding.

Member

elupus commented Oct 1, 2012

Example url that i just tested: rar://smb%3a%2f%2fmisto%2fVideos%2fmovies%2fGhost.Dog.The.Way.of.the.Samurai.1999.DVDRip.XviD.iNT-PFa%2fghostdogcd1-pfa.rar/ghostdogcd1-pfa.avi

There is no double encoding going on.

Member

jmarshallnz commented Oct 1, 2012

Sascha: my code should work without the CURL::Decode() (as GetHostName()
handles that for us - not sure if SetHostName() does as well, if so, the
Encode can be dropped as well). Once you recurse, you have a new URL, thus
the GetHostName() decodes the doubly-encoded portions of the URL.

On Tue, Oct 2, 2012 at 10:08 AM, Joakim Plate notifications@github.comwrote:

Then something is very wrong. There should be no double encoding.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1480#issuecomment-9050123.

Member

elupus commented Oct 1, 2012

Oh, this is due to how stuff is served up by the webserver perhaps?

Owner

Montellese commented Oct 1, 2012

@elupus Ah I thought a rar file within a rar file would be represented by an embedded "rar://" URL as well. Seems like this is not the case.

And no libmicrohttpd url-decodes everything that comes in so we already get the URL-decoded path.

I'll use jmarshalls code without the decoding/encoding and adjust the unit tests.

Owner

Montellese commented Oct 1, 2012

OK I hope I got it right now. I checked in XBMC with a test zip inside a test rar which results in a path like this:

zip://rar%3A%2F%2F%252Fpath%252Fto%252Frar%2Fpath%2Fto%2Fzip/subpath/to/file

so no extra URL decoding/encoding necessary.

Owner

Montellese commented Oct 2, 2012

Updated the VFS handler to handle archive paths recursively to get to the path to the actual archive file (thanks @elupus).

Member

jmarshallnz commented Oct 2, 2012

Looks good to me

Member

elupus commented Oct 2, 2012

Two comments.
Firstly i find it pointless to do the is archive check in getrealpath.
Second, shouldn't you use get parent folder uri function to go up one
level, instead of re implementing?

Owner

Montellese commented Oct 2, 2012

Yup that IsInArchive() check could be dropped.
Concerning using get parent folder(): I have to split the path to be able to find the ".." and "." in the first place so I went with a "vector" based implementation which would re-assemble the path at the end of all processing. I could just append every processed directory to the resulting string and then to a call to GetParentPath() when I find a "..". Not sure if it's really re-implementing as GetParentPath() does a lot more (and includes a lot more checks and conditions) which are not needed in this case and it also has to search in the string again and remove something from the end. That approach contains a lot more string manipulations than the one I went for which takes the string apart once, then operates on a vector and then puts the string back together.

Owner

Montellese commented Oct 2, 2012

OK I removed the unnecessary check for IsInArchive() on the path's hostname in GetRealPath().

Member

jmarshallnz commented Oct 2, 2012

Looks fine to me.

Montellese added a commit that referenced this pull request Oct 4, 2012

Merge pull request #1480 from Montellese/check_path_in_path
webserver: make VFS handler more secure

@Montellese Montellese merged commit 8b9f91d into xbmc:master Oct 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment