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

Allow paths with ".." in macro inclusions #1033

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@CelticMinstrel
Member

CelticMinstrel commented May 7, 2017

I have made this safe (I think) by resolving any ".." components manually while the path is still a relative path to the gamedata or userdata directory. If there are ".." components left over that can't be resolved (ie, at the beginning of the path), then it's an error. Otherwise, resolution then proceeds as normal, depending on whether it's a WML path (which is also used by Lua) or binary path.

I think this could use an audit to determine if it's truly safe. I did add some unit tests to make sure it would be safe, but I'm not sure if it covers every possible case.

With this it should be possible to make any addon not depend on the name of its folder.

CelticMinstrel added some commits May 6, 2017

Accept ".." in file paths everywhere
This accepts ".." without escaping the sandbox by first resolving it and
failing if the result begins in "..". Only then is the relative path
resolved to an absolute path.
@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf May 7, 2017

Contributor

i don't like these commits, now get_wml_location is sometimes called with absolute paths, sometimes with relertive paths, sometimes it makes removes a part of the path sometimes not. In conslusion it makes to much harder to veryfy that these functions are actually safe. Even if this commit wouldn't create securits issues directly it it makes the code harder to understand and thus easier to accudnetly create seciruily issues later.

the best fix would probably to make the preprocessor code internally always use relertive paths aswell and then behave like the lua functions did previously.

also note the following potentionl security issue:

a = load("return wesnoth.read_file('filename')", "@path/to/wesnoth/C:/system/someimportantfile.txt)()

will the function remove the leading 'path/to/wesnoth' so that only 'C:/system/someimportantfile.txt' remains?

Contributor

gfgtdf commented May 7, 2017

i don't like these commits, now get_wml_location is sometimes called with absolute paths, sometimes with relertive paths, sometimes it makes removes a part of the path sometimes not. In conslusion it makes to much harder to veryfy that these functions are actually safe. Even if this commit wouldn't create securits issues directly it it makes the code harder to understand and thus easier to accudnetly create seciruily issues later.

the best fix would probably to make the preprocessor code internally always use relertive paths aswell and then behave like the lua functions did previously.

also note the following potentionl security issue:

a = load("return wesnoth.read_file('filename')", "@path/to/wesnoth/C:/system/someimportantfile.txt)()

will the function remove the leading 'path/to/wesnoth' so that only 'C:/system/someimportantfile.txt' remains?

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel May 7, 2017

Member

In your example, I'm pretty sure it would interpret that "C:" as the name of a file under "path/to/wesnoth", but it might depend on Boost.Filesystem to some extent.

Make the preprocessor pass relative paths as the current directory? I suppose I could try that. In that case, I suppose it could interpret an absolute path as the gamedata directory? (And issue a warning, of course.)

Member

CelticMinstrel commented May 7, 2017

In your example, I'm pretty sure it would interpret that "C:" as the name of a file under "path/to/wesnoth", but it might depend on Boost.Filesystem to some extent.

Make the preprocessor pass relative paths as the current directory? I suppose I could try that. In that case, I suppose it could interpret an absolute path as the gamedata directory? (And issue a warning, of course.)

Instead of trying to trap absolute paths in get_wml_location,
make the preprocessor not pass them.
@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg May 9, 2017

Contributor

Do you track the internal '../' components to check for underflow?

For example "dir/../../somefile" is semantically identical to "../somefile"

Note that "dir" above does not need to exist (and be a directory) if you are handing the "../" component internally.

Contributor

GregoryLundberg commented May 9, 2017

Do you track the internal '../' components to check for underflow?

For example "dir/../../somefile" is semantically identical to "../somefile"

Note that "dir" above does not need to exist (and be a directory) if you are handing the "../" component internally.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel May 9, 2017

Member

Based on the logic I wrote, "dir/../../somefile" should indeed be resolved to "../somefile" by the resolve_filename function. The resolved name is then passed through is_legal_file though, which (as before) returns false because it contains a ".." component.

(If you look at the unit tests, you'll see one of them verifies that get_wml_location("core/../..") == "".)

Sudden thought: I wonder if I should manipulate the path components by iterating through a bfs::path instead of searching through the path as a simple string...

Member

CelticMinstrel commented May 9, 2017

Based on the logic I wrote, "dir/../../somefile" should indeed be resolved to "../somefile" by the resolve_filename function. The resolved name is then passed through is_legal_file though, which (as before) returns false because it contains a ".." component.

(If you look at the unit tests, you'll see one of them verifies that get_wml_location("core/../..") == "".)

Sudden thought: I wonder if I should manipulate the path components by iterating through a bfs::path instead of searching through the path as a simple string...

@CelticMinstrel CelticMinstrel added this to the 1.13.9 milestone May 12, 2017

@loonycyborg

This comment has been minimized.

Show comment
Hide comment
@loonycyborg

loonycyborg Jun 4, 2017

Member

If you want to reliably prevent tricks with embedded ".." you should consider this function: http://www.boost.org/doc/libs/1_64_0/libs/filesystem/doc/reference.html#canonical . It even expands symbolic links.

Member

loonycyborg commented Jun 4, 2017

If you want to reliably prevent tricks with embedded ".." you should consider this function: http://www.boost.org/doc/libs/1_64_0/libs/filesystem/doc/reference.html#canonical . It even expands symbolic links.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jun 4, 2017

Member

I think I looked at that function but had some reason for not using it... possibly because it only works on files that actually exist? Or maybe because of Wesnoth's weird interpretation of ~ in paths?

Member

CelticMinstrel commented Jun 4, 2017

I think I looked at that function but had some reason for not using it... possibly because it only works on files that actually exist? Or maybe because of Wesnoth's weird interpretation of ~ in paths?

@loonycyborg

This comment has been minimized.

Show comment
Hide comment
Member

loonycyborg commented Jun 5, 2017

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jun 5, 2017

Member

Hmm, okay. Maybe that could replace the custom function I wrote to resolve paths, then. I'll take a look at some point... maybe this weekend.

Member

CelticMinstrel commented Jun 5, 2017

Hmm, okay. Maybe that could replace the custom function I wrote to resolve paths, then. I'll take a look at some point... maybe this weekend.

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