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

Add plugin permissions and IO API #5231

Merged
merged 37 commits into from
Mar 9, 2024
Merged

Add plugin permissions and IO API #5231

merged 37 commits into from
Mar 9, 2024

Conversation

Mm2PL
Copy link
Collaborator

@Mm2PL Mm2PL commented Mar 3, 2024

This PR adds:

  • Wrappers for Lua's native io library. If the proper permissions are set, they are mostly transparent to the user and behave like Lua's functions.
  • Support for declaring permissions in info.json
  • Two permission types: FilesystemRead and FilesystemWrite
    Open questions:
  • How should we go about globs in permissions? We don't, per-path globs aren't a thing.
  • Should we ensure that plugins don't overwrite their code? Restrict everything to its data directory.
  • How should we ensure that plugins suddenly can't overwrite their info.json to add permissions? Same as above.

Closes #4620

@iProdigy
Copy link
Contributor

iProdigy commented Mar 3, 2024

could we solve some of the open questions by restricting the path to something like ./plugins/plugin_name/ instead of allowing arbitrary paths to be defined in the json? (i'm apprehensive to allow more general file access unless there's a compelling use case)

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Mar 3, 2024

Could we solve some of the open questions by restricting the path to something like ./plugins/plugin_name/ instead of allowing arbitrary paths to be defined in the json? (i'm apprehensive to allow more general file access unless there's a compelling use case)

I took the arbitrary paths as it might be useful for IPC (think FIFO), those usually are placed in temporary folders. I decided to not be overly restrictive with this. I do agree that it is a little scary. If we restricted it to plugin directory only, we could do away with per-path permissions entirely and just have an access filesystem permission. That still would not relieve us of problems 2 and 3: overwriting code and info.json.

@iProdigy
Copy link
Contributor

iProdigy commented Mar 3, 2024

to clarify, i mean a separate sub-directory that does not contain any code or the info.json file

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Mar 3, 2024

That would solve basically all permission issues. Only problems I could imagine is malicious plugins filling up the user's drive and that should not be a major issue. However I am still not sure about restricting the io module more than absolutely necessary.

src/controllers/plugins/LuaUtilities.hpp Show resolved Hide resolved
src/controllers/plugins/Plugin.cpp Outdated Show resolved Hide resolved
src/controllers/plugins/Plugin.cpp Outdated Show resolved Hide resolved
src/controllers/plugins/Plugin.cpp Outdated Show resolved Hide resolved
src/controllers/plugins/api/IOWrapper.cpp Outdated Show resolved Hide resolved
src/controllers/plugins/api/IOWrapper.cpp Show resolved Hide resolved
src/controllers/plugins/api/IOWrapper.cpp Show resolved Hide resolved
src/controllers/plugins/api/IOWrapper.cpp Outdated Show resolved Hide resolved
src/controllers/plugins/api/IOWrapper.cpp Show resolved Hide resolved
src/controllers/plugins/api/IOWrapper.cpp Show resolved Hide resolved
@Mm2PL Mm2PL requested a review from pajlada March 5, 2024 18:21
user data.
Prevents possible out of bounds reads. To see this even exist you would
have needed a userdatum smaller than chatterino::lua::api::UserData in
which case our program would die a horrible death by segmentation fault.
docs/wip-plugins.md Outdated Show resolved Hide resolved
@Mm2PL Mm2PL enabled auto-merge (squash) March 9, 2024 18:41
@Mm2PL Mm2PL disabled auto-merge March 9, 2024 18:43
@Mm2PL Mm2PL enabled auto-merge (squash) March 9, 2024 18:51
Copy link
Contributor

@iProdigy iProdigy 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 bearing with my security nits

@Mm2PL Mm2PL merged commit 658fced into master Mar 9, 2024
17 checks passed
@Mm2PL Mm2PL deleted the feature/plugin-permissions branch March 9, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a plugin permission system
4 participants