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

Create the SSCSM scripting #15818

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Conversation

Desour
Copy link
Member

@Desour Desour commented Feb 22, 2025

Based on #15568. But I decided to not update that, as it received no reviews. => Not waiting.

What this PR includes:

  • Everything from Add a SSCSM controller and environment skeleton #15568. (Might be worth reading PR description there.)
  • A minimal scripting env.
  • It is meant to be secure (i.e. includes overwritten functions, like a less precise os.clock()). (See also doc/sscsm_security.md.)
  • Loading of client builtin.
  • A non-binary setting enable_sscsm. This is not a bool, but an enum that says when (actually server-sent) sscsm is on. (Client builtin is always on.) The idea is that, for example, if it's localhost, sscsm is on in singleplayer, and when joining a server on localhost, but not lan or other servers. (See also doc/sscsm_security.md.)
  • Documentation.

Possible follow-up PRs:

  • Fill the scripting env with functions.
  • Move hardcoded stuff (e.g. from game.cpp) to client builtin, by adding events and functions.
  • Client builtin anticheat check (by hashing files). (I think we could use cmake for this.)
  • Sending mods to the client (only in singleplayer (then we can even easily change the network packets without breaking anything)).

To do

This PR is Ready to be Reviewed.

How to test

  • Start the game.
  • Set enable_sscsm to singleplayer. Start the game again. => It prints out nodes (not node names, but content ids).

@Desour Desour added Feature ✨ PRs that add or enhance a feature Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) SSCSM labels Feb 22, 2025
Copy link

@wmikita wmikita left a comment

Choose a reason for hiding this comment

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

the amount of effort you're putting into bringing SSCSM is admirable! I really appreciate you working on this :3
it would be nice to have a demo/test SSCSM usage in the codebase too, i.e. clientmods/preview(_sscsm?) & games/devtest/mods/sscsm(?)

Desour and others added 15 commits March 24, 2025 11:47
Co-authored-by: Jude Melton-Houghton <[email protected]>
tostring({}) and string.format("%s", {}) give you pointers.
(see lj_strfmt_obj)
this is not very critical, but attacks could be made harder if we change this.
the effort of overwriting is not worth it I think right now
20 us was the value, firefox used as first response to the spectre attacks.
now it's 100 us or 5 us, depending on whether it's "cross-origin isolated".
we only have one origin, so choosing 20 us is probably fine, I guess
see also:
https://www.mozilla.org/en-US/security/advisories/mfsa2018-01/
https://developer.mozilla.org/en-US/docs/Web/API/Performance/now#security_requirements

other clocks:
* os.time() and os.date() only have seconds precision, AFAIK.
* dtime is only given once per step, so it's not useful
* there might be other ways to build clocks (if we get async envs for sscsm,
  with a busy loop, for example)
rationale:
* it's just boilerplate, as these just fill out the structs. can also be done at call site
* they are usually only called at one place
* it would lead to many includes (or at least forward defs) in sscsm_controller.h and
  sscsm_environment.h
@Desour Desour force-pushed the sscsm_script_begin branch from d7bb5f9 to 51bd206 Compare March 24, 2025 11:39
@Desour
Copy link
Member Author

Desour commented Mar 24, 2025

it would be nice to have a demo/test SSCSM usage in the codebase too, i.e. clientmods/preview(_sscsm?) & games/devtest/mods/sscsm(?)

Loading mods comes in a later PR.
But there's hardcoded preview code in C++ for now.

@Desour Desour force-pushed the sscsm_script_begin branch from b8d4d9a to 7e6c796 Compare March 24, 2025 12:21
@Desour
Copy link
Member Author

Desour commented Mar 24, 2025

All that's left here is (I think) to find out if we can/want to allow string.dump, os.date, debug.traceback, and debug.getinfo. I'd appreciate help!

Also, potential reviewers, do you want a separate #15568?

@appgurueu
Copy link
Contributor

appgurueu commented Mar 25, 2025

string.dump should be excluded. There is no point in it since we don't allow loading the dumped bytecode representation. (And on top of that, it could potentially be used to leak some info from other mods which ought to be opaque.)

os.date should be included: It is safe to assume that it won't leak time with absurd sub-millisecond precision.

debug.traceback should be safe (in principle the debug library need not be safe; but this one can most probably be considered an exception) and useful. Include.

debug.getinfo should be excluded, especially f, which lets you get arbitrary functions off the stack. A more detailed assessment can be made later when/if it becomes necessary.

Note: For SSCSM debugging it should be possible to turn off this sandbox in some capacity eventually, but that can come later.

I don't have hard feelings either way for whether this should be separate or not.

@Desour
Copy link
Member Author

Desour commented Mar 25, 2025

Thank you, that helps deciding! <3


os.date should be included: It is safe to assume that it won't leak time with absurd sub-millisecond precision.

Yes, it uses localtime.
My worries come more from http://lua-users.org/wiki/SandBoxes saying:

os.date - UNSAFE - This can crash on some platforms (undocumented). For example, os.date'%v'. It is reported that this will be fixed in 5.2 or 5.1.3.

The issue is basically that strftime is platform dependent.
It seems to also exist in luajit: LuaJIT/LuaJIT#851

I guess I will better disallow it. (It's also not that useful for modding anyways.)

@y5nw
Copy link
Contributor

y5nw commented Mar 25, 2025

For os.date we could consider a simple sanitization feature along the lines of (not tested)

local _date = os.date
function os.date(fmt, ...)
  if not fmt then
    return _date()
  end
  assert(type(fmt) == "string", "Format specifier must be a string")
  for perc, spec in fmt:gmatch("(%%+)(.?)") do
    if perc:len() % 2 ~= 0 then
      assert(spec ~= "", "Conversion specifier extends beyond end of string")
      assert(("AaBbcdFHIjMmpSUWwXxYyZ"):find(spec, 1, true), "Invalid conversion specifier")
    end
  end
  return _date(fmt, ...)
end

(Assuming we want to expose os.date.)

@Desour Desour removed the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Mar 25, 2025
@Desour
Copy link
Member Author

Desour commented Mar 25, 2025

I don't have hard feelings either way for whether this should be separate or not.

I'll go for not separate then. 🤷

For os.date we could consider a simple sanitization feature along the lines of (not tested)

Yes, we could. But I consider having os.date() to not be a high priority, so I won't (in this PR at least).


PR should be good now. Self-review is done.

@Desour Desour marked this pull request as ready for review March 25, 2025 15:49
@FatalistError
Copy link

looks like a good start.

I think that at least in somewhere in proceeding added APIs there should be the ability to opt-in to use server SSCSMs. Testing on real servers is very important. And additionally security vulnerabilities (and other bugs) need to be discovered to be worked out, even if unsafe (hence why it'd be opt-in, preferably with a warning).

and one other thing is the example in the markdown about "sscsm flow" is kind of... hard to understand? Maybe a better explanation is in order.

either way I think that this PR is a great leap forward even if there's no real API yet. If we can create a proper API, it will absolutely change Luanti for the better and give the game what it's needed for years. I'm very excited to see that, and as a modder I've been waiting for it for a very long time.

@Desour
Copy link
Member Author

Desour commented Mar 30, 2025

I think that at least in somewhere in proceeding added APIs there should be the ability to opt-in to use server SSCSMs. Testing on real servers is very important. And additionally security vulnerabilities (and other bugs) need to be discovered to be worked out, even if unsafe (hence why it'd be opt-in, preferably with a warning).

I do not want to discuss here whether or not users should already be able to activate SSCSM anywhere. It's not there yet anyways.

and one other thing is the example in the markdown about "sscsm flow" is kind of... hard to understand? Maybe a better explanation is in order.

sscsm_security.md is not aimed at modders, but at developers. The example you're referring to is derived from an actual issue we had in server modding. We can still update it to something else later. 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature SSCSM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants