nesbox / TIC-80

TIC-80 is a fantasy computer for making, playing and sharing tiny games.
https://tic80.com
MIT License
4.91k stars 474 forks source link

Lua: `require` function should be disabled #2352

Open soxfox42 opened 9 months ago

soxfox42 commented 9 months ago

In the Lua bindings, dofile and loadfile are explicitly disabled with custom error messages, but Lua's standard require function is available. This seems to be most likely unintentional, as the reasoning for blocking the other methods applies here as well.

In fact, I think having require enabled is actually worse, since it can load native Lua modules from various directories on the user's system. If a user has installed packages with LuaRocks for instance, this may provide a way for carts to break out of the sandboxed environment.

borbware commented 9 months ago

I totally understand the breaking-out-of-sandbox concern.

I gotta say, however, that disabling require would devastate my current TIC-80 workflow...

MegadronA03 commented 9 months ago

it may sound fine if "out of the sandbox" features worked only in develonment environment, like developers had dev tools/console/PC. Tho it still questionable, like downloading cart and using it in dev env, just because it intentionally made to work in dev env, like dev tools and games with large save data.

MegadronA03 commented 9 months ago

tho that could also be up to platform, where the tic carts are published. Just prevent uploading non standard carts that contains advance api calls for debugging and file IO

soxfox42 commented 9 months ago

Just prevent uploading non standard carts that contains advance api calls for debugging and file IO

It's not that easy. I don't think you can detect that a cart might use require - it's pretty easy to disguise such a call.

MegadronA03 commented 9 months ago

Just prevent uploading non standard carts that contains advance api calls for debugging and file IO

It's not that easy. I don't think you can detect that a cart might use require - it's pretty easy to disguise such a call.

I think its not possible to detect that reliably, because lua compiles on fly(cart start), but require could be disabled (require == nil) if cart were booted from the online platform. This will force user to get away from sandbox if he wish to use require, making overall process a little bit tideous.

soxfox42 commented 9 months ago

I think that's a fair solution. I was also thinking we could add an option in the config that needs to be enabled before require works - that would make sure that users were aware of the risks that come with require.

sogaiu commented 9 months ago

Any opinions on whether other languages (the ones that can anyway) should have a similar type of arrangement?

soxfox42 commented 9 months ago

I haven't looked into what other languages offer, but I would probably say there should be a similar treatment for other languages. My only reasons for that are: 1. consistency and 2. to ensure people are aware that carts using these features won't work once published.

sogaiu commented 9 months ago

I was motivated to ask because in some cases efforts might have already been made to "permanently" disable similar functionality -- in a manner similar to what you described in the first comment / original post.

I get that at dev-time, it can be a boon to one's workflow (depending on which language I suppose) so if it's to be a "thing" for one language, I would imagine one would want it elsewhere where it could be made to work.

atesin commented 7 months ago

i don't really see any imperative/urgent need to disable require function, a little rough and hurried decision, i don't find the "other implementations do" argument logic enough to do the same,.. in fact i think the require function could be very useful, especially on cart development stages, to split and sort code, and not being confused with large texts in tic80 code editor

what i however agree is to FIX security issues in tic80 require implementation, for example skipping the LUA_PATH and similar system variables, and hardcoding package.path and package.cpath lua system tables with "safe" values and make them read only (i argue to set this tables with current tic80 command console work directory as seen in prompt symbol at least... see #2426 for more details)

andreyorst commented 2 months ago

Note, require is useful with Lua's module system obtained via package.preload. I often include other single-file libraries in cartridge source code by using functions:

package.preload.bump = function ()
  -- bump.lua code goes here
end

I then use the normal local bump = require "bump" in my code and it works. Putting such libraries in a function inside the preload table helps preventing locals leaking out, confusing with ones I might use in my code.

So removing require would break carts that do that.

soxfox42 commented 2 months ago

Ah, that's a very good point. I guess the best solution to this one might be to remove the three default package.searchers entries that can access the entire filesystem, but leave the package.preload one.