Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be careful with using Path.Combine() #32

Open
MarcusWichelmann opened this issue Aug 10, 2018 · 2 comments
Open

Be careful with using Path.Combine() #32

MarcusWichelmann opened this issue Aug 10, 2018 · 2 comments

Comments

@MarcusWichelmann
Copy link
Contributor

MarcusWichelmann commented Aug 10, 2018

The code is using the C# Helper method Path.Combine() in multiple places. For example: https://github.com/FubarDevelopment/WebDavServer/blob/master/src/FubarDev.WebDavServer.FileSystem.DotNet/DotNetDirectory.cs#L101

Do you know that using this method might result in some unwanted behaviours and could cause serious security issues?
Like explained in the remarks of the API documentation (https://msdn.microsoft.com/de-de/library/fyy7a5kt%28v=vs.110%29.aspx?f=255&MSPPError=-2147217396) the method call
Path.Combine(@"C:\MyWebDavDir", @"C:\Passwords\VerySecretFile.txt") will result in "C:\Passwords\VerySecretFile.txt".

Like in some of the methods in the DotNetFileSystem classes, the second argument of Path.Combine is often user-defined and therefore it might be possible to leave the WebDav root directory and read/write to any file on the system. At least on Windows.

I din't check the full code to make sure this is actually possible, but I'd recommend to replace every call to Path.Combine with one to an own more secure implementation.

And by the way: Many thanks for this awesome library!
Kind regards, Marcus Wichelmann

@fubar-coder
Copy link
Collaborator

fubar-coder commented Aug 10, 2018

That would require one to try to create a file name like C:\Whatever\stuff.txt, which might be possible, because \ isn't recognized as path separator on the ASP.NET Core side.

I have to think about this problem.

EDIT: It seems to me that only Windows systems are affected.

@MarcusWichelmann
Copy link
Contributor Author

Yes, you're right, only Windows should be affected.
I just noticed, that new Uri(Uri baseUri, string relativeUri) is also affected. So this might need to be checked, too.

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

No branches or pull requests

2 participants