After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 750982 - Better sandboxing
Better sandboxing
Status: RESOLVED FIXED
Product: grilo
Classification: Other
Component: lua
unspecified
Other All
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2015-06-15 09:38 UTC by Bastien Nocera
Modified: 2015-06-16 10:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
lua-factory: Reduce the number of libs we load (2.31 KB, patch)
2015-06-15 09:38 UTC, Bastien Nocera
committed Details | Review
lua-factory: Don't allow Lua sources to load external modules (2.90 KB, patch)
2015-06-15 09:38 UTC, Bastien Nocera
none Details | Review
lua-factory: Don't allow Lua sources to load external modules (3.55 KB, patch)
2015-06-16 10:44 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2015-06-15 09:38:10 UTC
.
Comment 1 Bastien Nocera 2015-06-15 09:38:14 UTC
Created attachment 305272 [details] [review]
lua-factory: Reduce the number of libs we load

We don't want to lua to be initialised with a number of base libraries
that can access the local filesystem, or load external packages.

This is the recommended way to implement sandboxes in Lua.
Comment 2 Bastien Nocera 2015-06-15 09:38:19 UTC
Created attachment 305273 [details] [review]
lua-factory: Don't allow Lua sources to load external modules

We want to be able to control the modules used by Lua sources. Right
now, we'll only support the "grl" builtin module.
Comment 3 Victor Toso 2015-06-15 11:28:04 UTC
Hey, I agree that:
- We need better sandboxing (mainly for security reasons);
- Allowing Lua libraries as dependencies should be dealt with care;

What do you think about having those disable by default on build time but keep a build option for them? --lua-factory-sandbox=disable for instance?

I can see people using this feature happily outside desktop environment.
Comment 4 Bastien Nocera 2015-06-15 11:32:41 UTC
> I can see people using this feature happily outside desktop environment.

?

I don't think that security should be optional. And we can add wrappers around specific functionality when requested, rather than leaving it open by default.
Comment 5 Victor Toso 2015-06-15 21:28:40 UTC
(In reply to Bastien Nocera from comment #4)
> > I can see people using this feature happily outside desktop environment.
> 
> ?

In a set-top box or media center applications that could use Grilo as its framework for plugins. Being able to use ready-to-use Lua libraries could be nice. AFAIK this is not a present need...

> 
> I don't think that security should be optional. 

Me neither, I totally agree with better sandboxing.

> And we can add wrappers around specific functionality when requested, rather
> than leaving it open by default.

I don't see any harm on wrapping this on #ifdef ENABLE_LUA_LIBRARIES and let it disabled by default...

... but as this doesn't seem necessary for any known system/application, feel free to ignore the suggestion.
Comment 6 Victor Toso 2015-06-15 21:33:15 UTC
Review of attachment 305272 [details] [review]:

Looks good!
Comment 7 Victor Toso 2015-06-15 21:35:10 UTC
Review of attachment 305273 [details] [review]:

Feel free to push after removing the other function.

::: src/lua-factory/grl-lua-factory.c
@@ -928,3 @@
-      lua_module = it->data;
-
-      if (lua_module_exists (lua_module) == FALSE) {

lua_module_exists is not used anywhere else and should be removed too!

grl-lua-factory.c:520:1: warning: 'lua_module_exists' defined but not used [-Wunused-function]
 lua_module_exists (const gchar *lua_module
Comment 8 Bastien Nocera 2015-06-16 10:44:10 UTC
Created attachment 305380 [details] [review]
lua-factory: Don't allow Lua sources to load external modules

We want to be able to control the modules used by Lua sources. Right
now, we'll only support the "grl" builtin module.
Comment 9 Bastien Nocera 2015-06-16 10:44:45 UTC
Attachment 305272 [details] pushed as 2e25300 - lua-factory: Reduce the number of libs we load
Attachment 305380 [details] pushed as 3da1df6 - lua-factory: Don't allow Lua sources to load external modules