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

Lua sanitation of game_config can be bypassed #4302

Open
jostephd opened this issue Sep 1, 2019 · 4 comments

Comments

@jostephd
Copy link
Member

commented Sep 1, 2019

wml.parse("game_config.cfg") works and returns the contents of game_config.cfg.

This is unexpected, since the Lua API wesnoth.game_config is sanitized:

int lua_kernel_base::impl_game_config_get(lua_State* L)
{
char const *m = luaL_checkstring(L, 2);
return_int_attrib("base_income", game_config::base_income);
return_int_attrib("village_income", game_config::village_income);
return_int_attrib("village_support", game_config::village_support);
return_int_attrib("poison_amount", game_config::poison_amount);
return_int_attrib("rest_heal_amount", game_config::rest_heal_amount);
return_int_attrib("recall_cost", game_config::recall_cost);
return_int_attrib("kill_experience", game_config::kill_experience);
return_int_attrib("combat_experience", game_config::combat_experience);
return_string_attrib("version", game_config::wesnoth_version.str());
return_bool_attrib("debug", game_config::debug);
return_bool_attrib("debug_lua", game_config::debug_lua);
return_bool_attrib("mp_debug", game_config::mp_debug);
return 0;
}

lua_getglobal(L, "wesnoth");
lua_newuserdata(L, 0);
lua_createtable(L, 0, 3);
lua_pushcfunction(L, &dispatch<&lua_kernel_base::impl_game_config_get>);
lua_setfield(L, -2, "__index");
lua_pushcfunction(L, &dispatch<&lua_kernel_base::impl_game_config_set>);
lua_setfield(L, -2, "__newindex");
lua_pushstring(L, "game config");
lua_setfield(L, -2, "__metatable");
lua_setmetatable(L, -2);
lua_setfield(L, -2, "game_config");
lua_pop(L, 1);

What's more, game_config.cfg may contain addresses of private MP servers, or presentation layer UI customizations (custom audio files / colors) - Lua addons don't need to be able to access this sort of info.

I suppose we should split game_config to separate engine-related information (such as MP server addresses) from gameplay-related information (such as recall_cost)?

@jyrkive

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

I suppose we should split game_config to separate engine-related information (such as MP server addresses) from gameplay-related information (such as recall_cost)?

I think it would be better to just block game_config.cfg from being read directly. Add-ons can use wesnoth.game_config to read the parts they need.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

Why is this marked low-priority? If it's a potential privacy issue, shouldn't it be high-priority?

Also, game_config.cfg is included by the toplevel _main.cfg, so you'd need to block that (as well as the empty path, which probably loads the toplevel _main.cfg). Maybe it's simplest just to block any toplevel file?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Why is this marked low-priority? If it's a potential privacy issue, shouldn't it be high-priority?

I had asked about this before creating the issue and was advised to treat it as a public, low-priority issue. I hadn't been sure either, and given your comment, I'll remove the label (for now, anyway).

Maybe it's simplest just to block any toplevel file?

Shouldn't we be doing a whitelist rather than a blacklist? Addons shouldn't have any reason to load any file not in the add-ons directory, right?

@jostephd jostephd removed the Low-priority label Sep 11, 2019

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Well, yeah, we could just go ahead and blacklist the entire main data directory, sure. I imagine some people could come up with niche reasons for loading files there, but...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.