TIC-80 icon indicating copy to clipboard operation
TIC-80 copied to clipboard

Lua: `require` function should be disabled

Open soxfox42 opened this issue 2 years ago • 12 comments

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.

soxfox42 avatar Nov 10 '23 08:11 soxfox42

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

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

borbware avatar Nov 12 '23 15:11 borbware

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 avatar Nov 12 '23 19:11 MegadronA03

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

MegadronA03 avatar Nov 13 '23 04:11 MegadronA03

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.

soxfox42 avatar Nov 20 '23 21:11 soxfox42

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.

MegadronA03 avatar Nov 21 '23 13:11 MegadronA03

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.

soxfox42 avatar Nov 22 '23 06:11 soxfox42

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

sogaiu avatar Nov 22 '23 09:11 sogaiu

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.

soxfox42 avatar Nov 22 '23 09:11 soxfox42

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.

sogaiu avatar Nov 22 '23 10:11 sogaiu

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)

atesin avatar Dec 29 '23 03:12 atesin

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.

andreyorst avatar Jun 03 '24 20:06 andreyorst

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.

soxfox42 avatar Jun 03 '24 23:06 soxfox42