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

Enable using 'require' command from within 'config.ld' #308

Closed
wants to merge 1 commit into from

Conversation

AntumDeluge
Copy link
Contributor

@AntumDeluge AntumDeluge commented Jul 27, 2018

Adds ldoc.require to be used in config.ld:

-- Enable `require` command within `config.ld`
ldoc.require = require

See #304.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to contribute!

I think this makes sense for some use cases, but would you be able to add documentation for it? It seems like this project should probably document all it's options ;-)

@alerque
Copy link
Member

alerque commented Sep 29, 2020

Doesn't this also largely defeat the purpose of having the config file sanboxed off so tightly? This should be made optional in some way that the person executing it has to explicitly allow this action.

@tst2005
Copy link

tst2005 commented Sep 30, 2020

Is this minor change not a huge security hole ?!
Do you really need to allow active (and potential dangerous) lua code inside a config file ?
Why do you need to use require ?
require allow to load file system support, socket support, crypto module (if there are available that is often the case).

  • Do you want a PoC of malicious config.ld file that is able to upload your private key on a remote server, setup a backdoor, encrypt your file or just destroy everything ?

IMHU allowing require in config.ld is a very bad idea, but I really wonder why you need it.

Regards,

@alerque
Copy link
Member

alerque commented Sep 30, 2020

@tst2005 As I commented above, yes this does defeat the purpose. What I didn't realize before is that the list of allowed commands wasn't just a whitelist allowing some things through from the Lua VM, it is a complete sandbox with all available commands re-implemented. That does change my view of how this should be handled a bit.

Note that we're not just talking about the config file here, were talking about the code in templates as well which has the same sandboxing.

I still thing the feature makes sense to have, but doing it just for this one command and not for all commands is silly and would be easy for people to blow off the security ramifications. Even if in their mind they are just using it for an include(), fundamentally they are changing the security model of the whole system.

Rather than this PR as implemented, for the use cases where this really does make sense I think we could accept a kind of --UNSAFE-NO-SANDBOX flag that just eschewed the whole system of builtins and let you use any Lua code you want in templates/configs/whatever. That makes more sense to me that piece mealing one command at a time starting with the most unsafe one possible in the name of something people may assume is iniquitous.

@alerque alerque closed this Sep 30, 2020
@Elv13
Copy link

Elv13 commented Oct 6, 2020

Sorry for the delay. I just want to say we (AwesomeWM) also want to have an option to disable the sandbox. I had to implement really nasty hacks to poke a hole and get rid of it by abusing a reference to penlight accessible from the template, then reverse-forward it back to config.ld.

I needed to add code to config.ld to modify the items tree to bolt-on property typing, signals, permissions and theme variables used by AwesomeWM.

https://awesomewm.org/apidoc/core_components/client.html

The resulting code is horrifying: https://github.com/awesomeWM/awesome/blob/master/docs/config.ld . Having no sandbox, being able to use penlight and having a proper/official way to access the AST would allow me to cleanup this mess.

I would also like to further go down that rabbit hole and have custom post-processor on the AST to be able to:

  • "find all instance of this things" and auto generate a "consumer" table (this would be used for theme variables).
  • "get all place where @see points to here to make sure they are symetric"
  • "when type 'whatever' is used, always add this '@\see' implicitly"
  • "if the API has a capital letter and no explicit '@\iamsosorry' tag, assert and quit".

@tst2005
Copy link

tst2005 commented Oct 6, 2020

About LDOC the need I see is to load or enable one or more feature (for now the only one i see is string.split).
The state I want to avoid is to expose everything.

Why not create a feature directory to put lua module file.
Make a require-like function that is ONLY try to load module inside this directory.

Sample:

Inside feature/string/split.lua

return require "string.split"

Inside config.ld:

local split = feature("string.split")

By this way anyone can expose any lua module but not all of them.

@alerque
Copy link
Member

alerque commented Oct 6, 2020

I'm AFK right now, but thanks for the input. Can someone open an issue to track adding a way to surface modules and/or completely disable the sandbox. Re-opening this PR doesn't seem like the right move but this does need to be followed up on.

@Elv13
Copy link

Elv13 commented Oct 12, 2020

Can someone open an issue to track adding a way to surface modules and/or completely disable the sandbox.

Done.

I split into 5 smaller issues required to implement a proper module system to extend ldoc. As you probably know, the current codebase is not the most maintainable and readable code in the universe. Beside the inconsistent indentation and large spaghetti modules, the way it work itself isn't "internally extendable" anymore. It works, but adding more features should probably be done using modules rather than bolting more stuff into the core and killing the project (again).

Not much as to actually be done to support modules. Most of the work needed is to undo things like the sandbox and document some pseudo-private-but-used-by-the-template-so-they-are-in-fact-public APIs.

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

Successfully merging this pull request may close these issues.

4 participants