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

[CSM] Improve security #5100

Merged
merged 1 commit into from Jan 28, 2017

Conversation

red-001
Copy link
Contributor

@red-001 red-001 commented Jan 22, 2017

No description provided.

@nerzhul
Copy link
Member

nerzhul commented Jan 23, 2017

@sapier is this correct for you ?

"date",
"difftime",
"time",
"setlocale",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just remove it here instead of setting it to nil in init.lua

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used by bultin.

"setlocale",
};
static const char *debug_whitelist[] = {
"getinfo",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combined with the other changed this effectively disables the debug library, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ask @sapier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually debug code provides a lot of information about internal things.
e.g. some of the debug functions provide access to local variables of other functions.
As debug usually ain't used by anything productive disabling it completely is the savest way to get rid of those issues. Of course you could check each debug fct on it's own but I assume if you remove all potentially unsave ones debug wouldn't be usefull anyway ;-)

"math",
};
static const char *io_whitelist[] = {
"close",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client side lua should not be able to read the filesystem at all, apart dofile

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact, reading FS okay, but only in selected places. Client side mode can have logs, or local data (for example imagine pure client side notes, like a journal)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logs

Should use an api feature for this

local data

Should use a virtual filesystem similar to HTML5's webstorage/localstorage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to rubenwardy. Deciding which path to access is quite hard if you consider the corner cases. If you limit to a single mod specific folder you're basically imitating a virtual filesystem. If not you risk missing some critical parts. e.g. /tmp usually is fine for local applications but there's a lot of data stored in /tmp you don't want someone from outside your machine to access.

SECURE_API(g, dofile);
SECURE_API(g, loadstring);
SECURE_API(g, require);
lua_pop(L, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if these save functions are save enough for clientside

SECURE_API(io, input);
SECURE_API(io, output);
SECURE_API(io, lines);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if these save functions are save enough for clientside same her especially for open

@ShadowNinja
Copy link
Member

The jit part should check USE_LUAJIT, but more importantly I think the duplication between the security initialization functions should be substantially reduced.

@nerzhul nerzhul force-pushed the client_side_modding branch 2 times, most recently from b4aaca4 to 635507c Compare January 26, 2017 21:47
@nerzhul
Copy link
Member

nerzhul commented Jan 27, 2017

can you rebase @red-001 ?

@nerzhul nerzhul added the Rebase needed The PR needs to be rebased by its author. label Jan 27, 2017
@Zeno- Zeno- removed the Rebase needed The PR needs to be rebased by its author. label Jan 28, 2017
@nerzhul nerzhul merged commit d696b0d into minetest:client_side_modding Jan 28, 2017
nerzhul pushed a commit to nerzhul/minetest that referenced this pull request Feb 7, 2017
nerzhul pushed a commit that referenced this pull request Feb 8, 2017
nerzhul pushed a commit to nerzhul/minetest that referenced this pull request Mar 6, 2017
nerzhul pushed a commit that referenced this pull request Mar 13, 2017
@red-001 red-001 deleted the improve_security branch March 19, 2017 10:38
@paramat
Copy link
Contributor

paramat commented Jan 17, 2018

Lacking description.

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

Successfully merging this pull request may close these issues.

None yet

8 participants