E2 View Requests System - #2157
Conversation
|
Looks pretty clean. You're saying admins will no longer be able to open code? There's legitimate reasons for this like inspecting the code for malicious behavior. Will there be an alternative? |
|
Would it make sense to also extend this to overwriting code? It is the more harmful interaction IMO (See #929 for context) Also note that it is pretty easy to get the E2 Code from a dupe. |
|
@thegrb93 There is no good reason for admins or other people with elevated permissions to be able to view code, preventing malicious code should be done with correct library permissions and terminating/removing. @adosikas The system could be extended to overwriting too, however that doesn’t give someone access to the original code, but I have seen the issue of owner not updating first hand recently actually. A better solution for this however would be either updating the owner properly, or just preventing inserting code under another user allowing permission escalation. As for retrieving code from a dupe file, I am aware, however I can’t think of a clean way to allow sharing an entire contraption with multiple chips, unless people actually shared the dupe file itself but that’s cumbersome. |
|
Regarding chip overwriting actually, I’ll take a look at the code and see if changing the owner on a running chip would cause problems, and if so I’ll just spawn a 2nd chip, upload the code into that, set its transform to the original chip, then delete the original chip (assuming a valid |
What happens if the owner is disconnected from the server
If no admin can look at code, then exploits can't be found and fixed. |
|
@Divran if there’s no valid owner (ie after a disconnect), then the code is downloaded as there’s nothing to verify either a request or prop protection against, however I’m almost certainly going to just change the flow slightly and err on the side of caution by just not allowing chips with no valid owner to be opened (also fixing a couple of little niggles I noticed after submitting the PR with the same commit). Edit: just gonna use the existing method of a dummy trace into a hook call on |
|
And regarding no admin looking at the code, of all the times where admins take code without permission, the majority of them is just stealing the code, very few times is it finding an exploit (and if it’s a proper exploit not just crashing gmod, cause anything can do that, it will likely be found by someone else and brought up in an issue eventually). |
|
It's not wiremod's job or responsibility to try to fight against admin abuse. If a server gives admin permissions to those kinds of people, that's their fault, not ours. You aren't even slowing down the abusers anyway, only harming legitimate servers. Wiremod's job is to provide adequate tools for administrators to do their jobs, not to hinder them.
Also, if you allow anyone (including non-admins) to download disconnected players E2s, then you've given more power to exploiters than admins, since crashing specific people isn't hard (even without e2). You said you were gonna fix that though, so that's good. |
|
The only real way to prevent admin abuse is to only give admins the same permissions as everyone else, for every tool (not just downloading E2 code). In which case they aren't admins any more. Which means that isn't a viable solution. Which means, the only real way to prevent admin abuse is for the owner of the server to not promote them in the first place. |
|
It’s not fighting admin abuse, it’s merely improving the existing systems in place which also happens to solve the issue of people with elevated permissions stealing code. And again, if an exploit gets shared around enough to become a problem, then someone (likely multiple people actually) is going to report it in some capacity. Also in all my years of playing gmod, I have not seen a single case of admins or any other staff taking code in order to try and find an exploit, and given you can perform a number of tricks to hide code in plain sight anyway making it practically impossible to notice, giving a bypass to staff causes far more problems than it solves. |
well that's good then because this isn't going to prevent admin abuse at all, it's only going to harm legitimate admins. |
|
@Divran I'm reiterating here but, this doesn't prevent exploits from being found, and staff with enough technical knowledge to even understand an exploit will likely know to just dupe, save, and read from the file, or they can kick the person then use their |
|
From the server perspective:
From the client perspective:
|
|
@TomyLobo Wiremod isn't prop protection, but no prop protection is going to implement a streamlined chip by chip basis request system, so people have to give each other FULL prop protection perms in order to share a single chip (or use other methods like pastebin). What you upload to the server isn't under your control, yes, but your data folder is actually, especially under the new server operator rules that finally came out. |
|
wait, so FPP doesn't exist? |
|
@TomyLobo I rarely see falco's in use, do they allow sharing chips using the same or an equivalent method to view requests here? |
|
Just more evidence that you're not playing on very great servers. |
|
I do play on small to medium servers cause they're usually not filled with bloat |
|
The problems you cited were:
|
|
Other than possibly replacing the default derma elements and the likely occurrence of bugs in the future cause nothing works perfectly first try this is already finished, and in use on a couple servers actually so I'll just keep merging from here into my fork if this doesn't get merged (which I expected, and so did a few other people due to not implementing bypasses for admins). |
|
Just to clarify my stance on things First, I don't think this is a bad idea or anything. In fact, I made something similar in the past which was never finished (and I seem to have lost the code). My implementation was a general-use "ask a client a question" sort of thing, where a popup would appear off to the side of the screen and the user could interact with it by holding C to enable the cursor. It just needs a few minor things ironed out. Firstly, the ability to download disconnected players E2s must be fixed, that's non-negotiable. There's dozens of ways to crash specific people, which would allow any exploiter to steal anyone's code whenever they felt like it. (EDIT: from the commit messages it looks like this is already fixed, cool) Secondly, at the very least super admins should always have access to view users code. In general, the super admin rank is given only to the person actually paying for the box the server is running on, which means they must always have the final say. I'm probably going to have to deny this pull request unless this is addressed. I can definitely agree that whether or not regular admins should have this kind of access is less important. Although still important. Anyway, I have stated my views on this matter in previous comments so I'll not repeat myself there. We can definitely leave this open for some more discussion to see what other people think. |
|
@Divran giving superadmins the ability to take code still irks me (especially after what I've seen in my playtime), however at the end of the day the person owning the server box can still just manually take code with sv lua, so providing a more "legitimate" way to do that isn't really something I have any power over anyway. Given that, I could add a server console only concmd that can't be called from lua (assuming blocking concmds from lua is exposed to lua itself, if it isn't I'll just add a standard superadmin check like you said) which whitelists a single steam ID (the server owner basically), or a list of them, to bypass the view requests system securely, without the risk of people who potentially don't have sv lua, or anyone who finds a way to elevate their rank, being able to abuse the system. Also gonna look into fixing the permissions escalation with chip overwrite like @adosikas said using the method I described above, as another PR though as this one is already quite large. |
|
Console command method implemented, just need to test. |
|
So to chime back in, the reason that I personally am really not liking this idea, primarily, is that now wiremod is responsible for maintaining this protection. As said I'm still on the fence where I feel like additional hooks to support something like this would be OK, but the handling itself should be a separate addon. I'm not going to vote no because I'm not that much against it, but I still think it would be better to allow prop protectors or a separate addon to actually do the handling. With this in place, it's now two separate systems in play for protecting things. As another point to make as well: there's going to be people who bypass this protection and make people think their code is secure when it's not, and when their stuff gets stolen anyways, they're going to blame us. This just feels icky. I don't object to the idea of providing choices to players to protect their contraptions, but this ends up taking one problem and breaking it into several smaller ones. I just don't know how to feel about it. |
|
I can review again Monday |
|
I have not read through every message in this conversation. I just wanted to come back for a second to mention some things that might not have been mentioned before. The following are facts:
And my opinion:Using "admin abuse" is a weak excuse for wanting this change, and even if this PR was as strong as the addon I made back in 2015, in my opinion we still shouldn't add it to wiremod because it makes it more difficult, or outright prevents admins from doing their job. Something like this qualifies as either a PP addon or probably even an admin mod (like ULX). An addon that has the "feature" of allowing power hungry idiots to hold power while simultaneously not letting them use it in any meaningful way. Either way, it's far out of the scope of a content-only addon like wiremod. |
|
@Divran first of all I added the whitelist server side console command so if bad servers wanna let their abusive admins steal chips? They still can by adding them to the whitelist (hell they could even automatically add all admins and above to it), however more importantly it allows good server owners to only whitelist the people they absolutely trust and that have the ability to asses someone’s E2. Secondly, this pr is less to prevent code theft (although that is a reason), and more to not require giving full prop protection permissions to share and collaborate on code, which your linked repo doesn’t seem to address (working on the collaboration system I outlined above at the moment as sending a view request each time gets tiring). |
Again, this is a PP feature, not a wiremod feature. Several prop protections already have the ability to share individual entities without requiring you to use an "all or nothing" checkbox. I haven't looked into it but I believe FPP is one of them. And yes, my linked addon is far simpler than that, and doesn't have such granular control.
if they promoted someone to admin who they don't trust in the first place then they're not a good server owner |
Not just admins bypass prop protection on servers, usually down to a moderator role (for good reason cause they need to mainly remove laggy dupes or spam), even then, trusting someone to manage basic ULX, and handle kicks and bans is one thing, trusting them to not steal code is another. |
|
If E2 was able to be properly sandboxed to the point where it's essentially impossible to lag or crash servers with it, I'd most likely agree with you. But that's impossible so we need admins to be able to do their jobs. That the average admin of the gmod community may have a questionable IQ isn't our problem. I've now stated my opinion probably several times over so I'm gonna leave this conversation for now and check back later or whatever |
|
@Divran admins can still do their job by removing or halting an E2, and most lag/crash E2s can be detected just from the cpu time in remote updater, unless they abuse the source engine (usually with physics) meaning there’s not much you can do other than a cleanup. The only time viewing an E2 really helps with an admin’s job would be with (the usually hilariously ineffective) E2 “hacks”, as they are just gonna run passively without standing out in the remote updater or causing a noticeable effect to the server. |
|
Oh and regarding single entity prop protection permissions, lets say I’m working on a tracer which is mid way through a scene, and someone wants to take a look at the code, view requests lets me give them the code once without letting them move or remove the E2 ruining the trace. |
|
Removed the do not merge as the chat in discord yesterday satisfied my request to continue discussion. THIS IS NOT A MERGE APPROVAL. I am only removing the block I put in place. |
|
Will users be able to tell whether or not an admin is able to bypass the system? |
|
@thegrb93 Right now no, but going to add a print to their chat when someone takes a chip without needing a view request |
|
What about two default-on cvars that switch whether superadmin is able to bypass this and the other whether admin is able to bypass this, and a new e2 function like isAdminReadable so users can check that and self destruct or whatever if the bypass is enabled. |
|
@thegrb93 ok actually forgot to work on this as I was working on a DXR tracer in gmod, but could just make it a super admin bypass, with a toggle to make it an admin bypass, however, add a chat print to alert someone their chip was just taken via admin bypass. The cvar could just have a value of 0, 1, or 2, where 0 means no one can bypass it, 1 means superadmins can bypass it, and 2 means admins can bypass it. |
|
Ok |
|
Bump |
|
Oop, been working on GModDXR and kinda forgot about this, just need to implement Will add it today. |
|
@thegrb93 finalised, just need to test. Replaced the bypass list with a bypass mode convar which takes either 0, 1, or 2: As for allow always, just saves a table called |
|
Tested and fixes pushed |
|
I'll give it a look |
|
Looks fine to me presuming its tested and working. I can merge if there's no other objections. If any issues pop up then hopefully you would fix them. I might be able to fix but if its more trouble than its worth then we can easily revert. |
…2174) * Add try/catch statement to E2. Adds a try / catch statement to E2 that allows users to catch any errors that occur inside of the block, and if so, run the catch block, setting the variable name provided to a new local variable that contains the error string. * Add not enough memory & table overflow to uncatchable * Lot of combined changes * Made ``exit`` skip the try catch block as a whole. * Added ``@strict`` directive * Refactored the directives a bit to add ``@strict`` * Added a ``throw`` function to registerFunction that either throws an error or returns the default value of the function inside using the type defined. You can override this with the second argument. * Edited the preprocessor for this ^ * Edited the entire core/entity file to preview how this would look. * Some copytype calls still existed and were replaced with ``E2Lib.fixDefault`` * Some missing ``throw``s * self:throw and explicit returns Throwing is much more explicit now. You call self:throw(msg, var) and var is returned if the chip isn't strict. Got rid of the throw hack to try and get the default type. The only problem here now is trying to get some debug info about where the error happened. Right now it's not very useful and isn't able to point to the function calling self:throw for some reason. * Fix travis lint Hardcoded the starting point for this line. (#"trigger" + 2) * Can only catch e2 errors & thrown errors Native lua/glua error() will always error try and catch blocks for security reasons. The only errors that will be caught are created by E2Lib.catchableError. (self.throw and e2's error) Replaced context:throw with context.throw to be more memory efficient i guess. * Throw compat with a lot more libraries Added ``@strict`` compat to a lot more libraries. I think adding more would make this PR too big, it's already pretty huge. Additionally made some code a bit more clean and optimized & used E2Lib.newE2Table more. (Notably with bone fwd up right) * Fix compile errors * Have compile traces be sent to runtime * E2 instances/contexts now have a ``trace`` field, which contains the latest trace from EvaluateStatement & the ``seq`` instruction. Thrown errors will now show as something like ``Expression 2 (generic): Runtime error 'Invalid physics object!' at line 10, char 16``. * Fixed some compiler quirks (Some functions called from the Compiler global object instead of self, which wouldn't allow for multiple compilers iirc) * Also added Compiler:CallInstruction which does self["Instr"...] etc for you * Localized string.upper use * Fix udf errors * Make stringcalls able to be caught Forgot to re-implement this since we opted for a whitelisted error instead. * Fix some core error * Add strict test to unit tests, fix exit, assert * Add a ``strict.txt`` unit test to data/expression2/tests. * Also add a small try/catch test to ``parsing.txt`` * Fix assert() e2function not throwing a catchable error. It now provides a runtime src position like the rest of the errors * Fix exit() not exiting the chip. The ``Skip`` table was just making exit() exit the try / catch block rather than passing it to the error handler to exit the whole chip. * Fix #include, Runtime traces * Include was messed up because whoever implemented it didn't think about needing the scope after erroring. It didn't load the old scope back which messed up trying to use try/catch. * Fixed runtime traces because before I tried to make it only take from the seq instruction which would always make the error come from where the first instruction is. * Renamed E2Lib.catchableError to raiseException to be more clear. Also has a 4th param for whether it's catchable, that is default true. * Added E2Lib.unpackException(struct) which is a boilerplate bridge function for when you want backwards compatibility with error messages & error structs in pcalls. * Remove debug stuff * Squashed commit of the following: commit 02d5dfd Author: stepa2 <sssstepa452@yandex.ru> Date: Thu Jul 29 20:48:37 2021 +0300 Implemented do-while statement for E2 (issue #2191) (#2193) * Allow calling entity:setPos and entity:setAng on entities without collisions Also allow calling entity:propManipulate, but position and rotation is only updated * Update prop.lua Removed tailing whitespace, fixed luacheck warning * Implemented do-while statement for E2 (#2191) * Removed debug messages I fogot to remove, added parsing tests, disabled bugged optimization * Linting pass commit 73b4dbf Author: thegrb93 <grbrown93@sbcglobal.net> Date: Sun Jul 25 00:52:11 2021 -0400 Fix explodeRE (#2204) commit 6a5d4a2 Author: Derpius <49565664+Derpius@users.noreply.github.com> Date: Sat Jul 24 23:58:13 2021 +0100 Add 4th option to view request bypass (#2198) * Add 4th option to view request bypass * Update comment commit adca748 Author: thegrb93 <grbrown93@sbcglobal.net> Date: Sat Jul 24 18:57:53 2021 -0400 Fix light sprite size (#2201) * Fix light sprite size * Revert sizing code. Also simplified because you can't even see the smaller sprites * Made spritesize an option/input * Add back tube light sprites * Add missing clamp commit c4d333a Author: Divran <arviddivran@gmail.com> Date: Sat Jul 24 23:19:26 2021 +0200 fixed crash exploit commit 966b1e4 Author: AbigailBuccaneer <AbigailBuccaneer@users.noreply.github.com> Date: Sat Jul 17 15:09:26 2021 +0100 Restore expression2 ghost functionality Commit ec99005 accidentally prevent the E2 tool from showing ghosts. commit 01c36ae Merge: f85c234 1a1ddd3 Author: Divran <arviddivran@gmail.com> Date: Tue Jul 13 22:54:54 2021 +0200 Merge pull request #2197 from sammyt291/patch-1 Light sprite colors and size commit 1a1ddd3 Author: sammyt291 <samuel_canning@hotmail.co.uk> Date: Sun Jul 11 20:07:42 2021 +0100 Light sprite colors and size Changed the smaller 3 sprites on the tubular light to match the color of the main sprite on the light Allowed the sprite overlay to be sized according to the size slider already present, clamped to 128 to avoid changing existing contraptions with large-size glows enabled. commit f85c234 Author: stepa2 <sssstepa452@yandex.ru> Date: Sat Jul 3 03:35:04 2021 +0300 Allow calling entity:setPos and entity:setAng on entities without collisions (#2188) * Allow calling entity:setPos and entity:setAng on entities without collisions Also allow calling entity:propManipulate, but position and rotation is only updated * Update prop.lua Removed tailing whitespace, fixed luacheck warning commit 860604b Author: CoreyLee Hassell <Anticept@users.noreply.github.com> Date: Fri Jul 2 20:34:47 2021 -0400 Revert "Delete json funcs until Facepunch/garrysmod-issues#4976 is fixed (#2177)" (#2185) This reverts commit c6ea454. commit eb3841f Author: thegrb93 <grbrown93@sbcglobal.net> Date: Fri Jun 25 19:50:34 2021 -0400 Improve regex and fix pcall not returning (#2186) * Improve regex and fix pcall not returning * Don't use %b commit 1ef0f63 Author: thegrb93 <grbrown93@sbcglobal.net> Date: Thu Jun 24 02:25:28 2021 -0400 Add level setting, also play regardless of distance/location (#2183) commit ec99005 Author: 100PXSquared <100pxsquared@gmail.com> Date: Wed Jun 23 08:01:26 2021 +0100 E2 View Requests System (#2157) * Add E2 view requests system * Prevent prop protection stopping view requests * Perform CanTool check if chip owner is invalid * Refactor checks in TOOL:Think * Add SteamID whitelist convar to view requests * Fix request answer netmsg validation * Add EoF newline * Refactor E2 view requests * Replace bypass list with bypass mode convar * Implement allow always * Replace checks in remote updater code request * Fix copy paste induced bug commit f888eb1 Author: thegrb93 <grbrown93@sbcglobal.net> Date: Wed Jun 23 02:39:23 2021 -0400 Fix turret behavior if parented (#2184) commit f8335f9 Author: thegrb93 <grbrown93@sbcglobal.net> Date: Fri Jun 18 11:35:20 2021 -0400 Limit regex cpu consumption (#2181) * Limit regex cpu consumption. Fixes: #2063 Related (Facepunch/garrysmod-requests#1878) * Better error message commit d0a84b7 Author: Divran <arviddivran@gmail.com> Date: Fri Jun 18 00:44:56 2021 +0200 sound duration is very expensive commit f54ff0a Author: Divran <arviddivran@gmail.com> Date: Fri Jun 18 00:13:01 2021 +0200 fixed auto indenting auto un-indenting after typing } was broken (triggered even if the same line contained a {), and did not add itself to the undo table, so could not be undone. auto indenting after typing { was also broken if the same line contained a } adjusted digi screen bandwidth slowed down clientside digi screen rendering to prevent fps lag fixed hologram queues after last update broke it commit 4855366 Author: Divran <arviddivran@gmail.com> Date: Wed Jun 16 18:58:31 2021 +0200 digi screen max global bandwidth commit 7b5ac40 Author: Divran <arviddivran@gmail.com> Date: Wed Jun 16 02:18:54 2021 +0200 improved digi screen performance added cpu time based usage checks serverside and coroutine processing clientside hopefully this is (at least v1 of) the final strip of duct tape for this notorious entity commit d8cdc76 Author: Divran <arviddivran@gmail.com> Date: Tue Jun 15 22:37:39 2021 +0200 fixed multiple crash and lag exploits reset() no longer resets e2 quotas emergency ram shutdown now also clears gtable memory hologram scale/clip update rate limited to 0.1s and opcost increased to 30 and a few others commit c6ea454 Author: thegrb93 <grbrown93@sbcglobal.net> Date: Sat Jun 12 19:13:26 2021 -0400 Delete json funcs until Facepunch/garrysmod-issues#4976 is fixed (#2177) * Revert "Squashed commit of the following:" This reverts commit 9f0b2f0. * Return keyword * Fixed expressions being missing thanks to missing return keyword * Removed useless local variable in var operator * Remove unused test code Not a bug or anything but something I randomly came across..
10 participants
Overview
Replaces the need for giving entire prop protection perms to someone in order to share a chip, and removes the ability for admins and other elevated positions to take code without permission given they usually bypass prop protection requirements (for good reason).
Also provides code protection on servers without prop protection installed / set up.
Works with both toolgun right click and remote updater, duplicating chips is unchanged
How it Works
Instead of performing a
CanToolcheck when downloading from a chip, a request to view the chip is sent to the owner, assuming they're valid(if no valid owner then the chip is downloaded as there is no way to verify against either prop protection or the owner, this could however be modified in order to merely deny taking the chip as a safety measure)Not sure why I didn't do this before, but now performs aCanToolcheck if invalid using a dummy trace.This then logs the request serverside and sets an expiry time (set at 1 minute, but could be modified or turned into a convar), and sends the request to the chip owner.
The owner gets a message printed to their chat telling them the name of the chip being taken, and the player attempting to take the chip, as well as telling them to view the request in their context menu.
They would then either ignore the request and allow it to expire (which notifies the asking player), or open the
View Requestsmenu in their context menu, containing a list of all outstanding requests (currently the icon for this is the red gear on a black background, however anything representing E2 would suffice).The list displays the entity ID of the chip being taken, the player asking to take the chip, the name of the chip, and the number of seconds until expiry for every request.
The owner can then right click each request to either accept or reject them, and attempting to accept will display a confirmation box with a warning about stealing code to prevent accidentally accepting a request.
Both accepting and denying send a message to the asking player, and a request cannot be sent for a chip with an outstanding valid request
Additional Context
Not preventing duplication of chips
After a suggestion from one of the people I tested with, the duplication process remains unchanged, as you can check if your chip was duped and act accordingly from within E2, and would prevent people from easily sharing complex creations with multiple chips.
Replacing
TOOL:RightClickwithTOOL:ThinkCanToolpreventsTOOL:RightClickfrom being called, so manually handling right click was required in order to run separately from prop protection.Lack of tooltip or other indication to say that items in the view requests list should be right clicked
Out of the three people I tested this with 1 on 1, all three of them instinctively knew what to do here in order to interact with list elements, and given that the only possible ways to naturally interact with list elements other than selecting them is either double clicking or right clicking, adding and explanation for this to the chat prints would make them way too long (already fairly long as is), and if someone doesn't think to right click a list element then adding the info to the window title would also likely be missed.
And other methods like popups or first time view tutorials are intrusive and likely to be clicked past anyway.
BetterChatPrintSimple chat print implementation to use instead of the archaic
usermessagesystem.Not sure if wire already has an implementation of chat print using the
netlibrary, if so then replace this with that.