Skip to content

Fix E2 Permission Escalation Using Code Upload - #2161

Merged
thegrb93 merged 2 commits into
wiremod:masterfrom
Derpius:e2-upload-perms-fix
Apr 25, 2021
Merged

Fix E2 Permission Escalation Using Code Upload#2161
thegrb93 merged 2 commits into
wiremod:masterfrom
Derpius:e2-upload-perms-fix

Conversation

@Derpius

@Derpius Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

Updates a chip's owner when uploading into it if the current chip owner does not match the uploading player.
Fixes #929 (thanks to @adosikas for mentioning this here #2157 (comment))

Solution is only a few lines, only difficulty was updating the E2 chip's overlay which uses a completely undocumented function ENT:GetPlayerName(), which is set using another undocumented function ENT:SetPlayer().

@TomyLobo

Copy link
Copy Markdown
Contributor

How about simply blocking the upload?

You know, like I suggested 5 years ago:
#929 (comment)

@TomyLobo

Copy link
Copy Markdown
Contributor

I actually like @thegrb93's suggestion in the next commit, but you need to persist the "do not run" flag through dupes, otherwise you'll just delay the issue.
How does your fix address this, btw?

@Nebual

Nebual commented Apr 23, 2021

Copy link
Copy Markdown
Member

reviews 6 year old comments on the original issue

I think the main downside of this is:

  • newbie makes a chip, its not working, asks for help
  • Neb (as an admin, or PP friend) takes a look, fixes the bug, hits Upload and Exit
  • the chip is now Neb's, so newbie can't access it without Neb granting him PP access

(I figure this is the main positive use case for a second person to edit a chip)

Your #2157 PR does at least mean in that last case, the newbie can at least request to read the updated version of previously-their-chip, but its a bit inconvenient if they need to then rewire the chip to actually use it... hmm...

I don't think thats a blocker, and the security benefit you're proposing probably outweighs that con, but how do you feel about:

When you update someone else's chip, for E2 purposes the owner changes, but for PP purposes the owner remains the original. ie, the code's owner changes, but the gmod_entity's owner (who created the physical Entity into the world, and welded it to their ship, and wired it to their screen) remains. I think that'd just mean removing the SetOwner/CPPISetOwner lines, and adding a comment explaining that this distinction is intentional.

Then, if the ownership of the code isn't critical to the function of the chip (eg. it doesn't hover above owner()'s head), this workflow makes intutive sense and everything "just works" like it did before. If the ownership does matter, then (as opposed to prior to this PR), there's simply a 4th step of the newbie resaving the chip, which seems like something users would figure out.

... I also kinda like the gameplay implications of (in an RP sense, assuming you're PP buddies and all) "hacking" a chip by resaving it, without needing to remover/recreate/rewire it.

@TomyLobo

TomyLobo commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

That'd be confusing as hell to have different PP and E2 owners. Let's not do that.

Btw, this issue you're fixing here is only really surprising if you gave someone the right to right-click your chip but not to left-click it, and thus, ostensibly, only read your chip.
I wonder what change made that distinction possible.

@Nebual

Nebual commented Apr 23, 2021

Copy link
Copy Markdown
Member

Naw, all of this is still pertinent without #2157's changes, since PP can often be one-directional: an admin can do anything (eg. fix a chip) to a newbie, a newbie can't write or (without #2157) read an admin's chip, so if all chips an admin writes becomes the admin's, its kinda hard to collaborate.

Its slightly confusing, but I don't think it'd actually come up much. Its not like E2 programatically exposes the PP owner, so for all E2/code purposes "the owner changed" (which is different behaviour than we've had for the last decade, and thus by default I dislike the backwards compatibility change, but we've acknowledged the security hole it leaves for 6 years now, I think its a good fix), the PP staying is an implementation detail nobody'll notice - unless you can think of a semi-common use case this would affect/break. I think the PP changing would break more flows, like the original owner wanting their chip back.

@TomyLobo

TomyLobo commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

Ok let me summarize @thegrb93's suggestion, together with my improvement and a suggestion on how to implement it:

  1. Uploading to a chip that is not your own adds "@disabled\n" at the top
  2. If a chip starts with "@disabled\n", it is not run at all. Basically all further processing steps will receive an empty string
  3. If the user wants to re-enable the chip, they simply remove line 1 and click Upload & Exit
  4. While the chip is disabled we can also highlight it in a special way to make it more obvious

Implementing 1-3 should be pretty simple, but 4 is a bit trickier.
Whether you implement 4 or not, this way every step is transparent to the user

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo

How about simply blocking the upload?

Actually joked about this on Discord but you did in fact find a reason why this extremely simple fix wasn't adequate.

@Nebual not setting the CPPI and SENT owner is fine (only set them towards the end of testing this so I know not setting them works), seems like all of the internals just rely on chip.player anyway (and that undocumented func), I'll push those changes now.

@Nebual

Nebual commented Apr 23, 2021

Copy link
Copy Markdown
Member

Tomylobo's solution also seems acceptable, if a bit less streamlined for collaborating.
I'd tolerate either but I prefer 100PX's with CPPI left original - perhaps we can try that, and if the community hates it or finds it buggy/conflicting, we can pivot to the safer but less-streamlined "@disabled" solution.

@TomyLobo

TomyLobo commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

If you want a more streamlined solution, have it as a separate flag, but make damn sure it never ever gets discarded.
EDIT: You could also have an "unlock" button in the editor that removes the "@disabled" line in 1 click.

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo why use a flag when you can literally prevent a user from impersonating another user at all?

@TomyLobo

Copy link
Copy Markdown
Contributor

@TomyLobo why use a flag when you can literally prevent a user from impersonating another user at all?

How certain are you that your change actually does that?

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo because I tested using concmd mainly, possibly the most harmful function to have run on another player

@TomyLobo

TomyLobo commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

oh so you just lack imagination, got it.
I'm not even talking about what you can do with it.
I'm talking about what mixed-owner contraptions mean, security-wise. Have you even considered that?
That's, afaik, completely untested because it it just isn't part of the threat model so far.

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo well given you can literally run concmd as another user without this PR, have you considered the security implications of that?

@TomyLobo

Copy link
Copy Markdown
Contributor

First of all, you can run concmd as someone else only if they give you PP and that was, so far, always the end of what we cared about, security-wise, anyway.
Second, we're not talking your solution vs. nothing, we're talking your solution vs. mine.

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo then implement your solution and either PR, upload, or merge so we can compare the differences in what they allow.

@Divran

Divran commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

so why not use the suggestion by myself and @thegrb93 as seen here
#929 (comment)
and here
#929 (comment)

edit: the main downside of that idea would be if you paste someone else's contraption downloaded from the internet or whatever, the e2s wouldn't run - unless we implement a window that appears to ask the user to allow it or something, as was also brought up in that older discussion.

edit 2: anyway I'm fine with the change in this PR. it might be a little bit confusing sometimes but I don't see anything wrong with it.

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo

I'm talking about what mixed-owner contraptions mean, security-wise. Have you even considered that?

You realise that mixed owner contraptions are already possible with wire, and function no differently given each chip is individual?

@TomyLobo

Copy link
Copy Markdown
Contributor

edit 2: anyway I'm fine with the change in this PR. it might be a little bit confusing sometimes but I don't see anything wrong with it.

I'm not fine with it, since it allows you to change ownership of an entity, which is not something wire currently does.
We shouldn't merge a potentially dangerous stop-gap if there's a simple solution that doesn't share this issue, waiting to be implemented

You realise that mixed owner contraptions are already possible with wire, and function no differently given each chip is individual?

Now make up your mind, do you trust people you shared PP with or not?
If you do, then your other PR makes no sense.
If you don't, then this one doesn't.

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

@TomyLobo ok wow, a few things to dismantle here.

  1. Just because wire doesn't do something doesn't affect what it can do, otherwise it'd literally never get updated with anything.
  2. "Potentially dangerous", how, dear god man how is updating a chip's owner so you can't run code as them dangerous.
  3. "If there's a simple solution", doesn't get much simpler than 8 lines (or 5 LoC).
  4. I'm not sure how prop protection relates to my rebuttal of your spiel on how this doesn't address multi owner contraptions, given those are completely unrelated, and given this PR has absolutely nothing to do with view requests (doesn't even affect the same part of code), then I'm not sure why you're mentioning it regarding multi user contraptions.
  5. Additionally, I trust prop protection for what it's designed for, prop protection (if I didn't then I wouldn't have added interop with it to view requests), just not as adequate at code protection mainly due to usually only wanting to share a single chip (which not all pp provide methods for, and FPP with per entity whitelisting would allow someone to delete your chip when you just want to give them code, unless they have such fine control that you can specifically allow tool right click only on a single entity), and that everyone down to moderators usually bypasses it (for good reason, they need to be able to clean up users' props frequently, doesn't mean they all need to take code though).

@TomyLobo

Copy link
Copy Markdown
Contributor
3. "If there's a simple solution", doesn't get much simpler than 8 lines (or 5 LoC).

Challenge accepted aaaand met:
#2162
boom, merge, test and ship

@Derpius

Derpius commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

#2162
I can feel my braincells slowly dying after this.

@thegrb93

Copy link
Copy Markdown
Contributor

I don't see anything wrong with this implementation. Probably just needs a bit of testing.

@thegrb93

Copy link
Copy Markdown
Contributor

I'm fine with merging as long as any issues found are fixed by @100PXSquared . Otherwise we can just revert the changes.

@thegrb93

Copy link
Copy Markdown
Contributor

I think a solution like this is better thegrb93/StarfallEx@592a3c2

@TomyLobo

Copy link
Copy Markdown
Contributor

I think a solution like this is better thegrb93/StarfallEx@592a3c2

What happens if you dupe a contraption that contains this chip?

@thegrb93

Copy link
Copy Markdown
Contributor

I think a solution like this is better thegrb93/StarfallEx@592a3c2

What happens if you dupe a contraption that contains this chip?

All works fine

@TomyLobo

Copy link
Copy Markdown
Contributor

I think a solution like this is better thegrb93/StarfallEx@592a3c2

What happens if you dupe a contraption that contains this chip?

All works fine

That's literally the least specific you could have been :)
How did you try to break it and in what way does it "work fine"?

@thegrb93 thegrb93 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Fasteroid

Fasteroid commented Apr 24, 2021

Copy link
Copy Markdown
Contributor

I think he means nothing breaks. Unless you specify what should happen when a contraption is duped, his implementation is okay.

[EDIT] starfalls are also rarely duped on my server

[EDIT 2] Apparently sparky does dupe them

@thegrb93

Copy link
Copy Markdown
Contributor

I think he means nothing breaks. Unless you specify what should happen when a contraption is duped, his implementation is okay.

[EDIT] starfalls are also rarely duped

I make lots of dupes with starfall

@TomyLobo

TomyLobo commented Apr 24, 2021

Copy link
Copy Markdown
Contributor

Jesus, you're the worst support customers...
Can you please define "okay"?
I'm sure it won't crash or anything, so at that very basic level it's probably okay, I figured that much before asking.
I asked for what you do to determine it's "okay" and what result constitutes "okay".
Specifically if you upload code to my chip, I dupe it and then your code runs, is that "okay" for you (because that wouldn't be okay for me)

@thegrb93

thegrb93 commented Apr 24, 2021

Copy link
Copy Markdown
Contributor

Yes, and that is ok.

You would see your chip errored so duping it is up to you.

@TomyLobo

Copy link
Copy Markdown
Contributor

how visible is this when the chip is buried in your contraption somewhere?

@Derpius

Derpius commented Apr 24, 2021

Copy link
Copy Markdown
Contributor Author

Well @TomyLobo if I also error the chip like sparky does in his commit to Starfall, then you'll see the errored chip due to the red highlight.

@thegrb93

thegrb93 commented Apr 24, 2021

Copy link
Copy Markdown
Contributor

how visible is this when the chip is buried in your contraption somewhere?

If you're worried about this, you should also be worried about someone putting their own e2 in your dupe. I recall an 'e2 virus' using this mechanism.

@Derpius

Derpius commented Apr 24, 2021

Copy link
Copy Markdown
Contributor Author

Although @thegrb93 I would rather not halt the chip at all, updating the owner could theoretically cause some confusion, although I doubt that much, halting the chip however makes collaboration significantly harder (I also think removing updating the owner from Starfall in favour of halting the chip is worse for the same reason, and regarding confusion, I literally never found anyone confused with SF owners updating, hell I didn't even really notice them changing automatically myself, just felt natural).

@TomyLobo

Copy link
Copy Markdown
Contributor

how visible is this when the chip is buried in your contraption somewhere?

If you're worried about this, you should also be worried about someone putting their own e2 in your dupe. I recall an 'e2 virus' using this mechanism.

hmmmm, true, if they have PP, they can weld stuff to your contraption.
I'm actually not sure though, does advdupe/advdupe2 duplicate entities that are not your own?

@Derpius

Derpius commented Apr 24, 2021

Copy link
Copy Markdown
Contributor Author

how visible is this when the chip is buried in your contraption somewhere?

If you're worried about this, you should also be worried about someone putting their own e2 in your dupe. I recall an 'e2 virus' using this mechanism.

hmmmm, true, if they have PP, they can weld stuff to your contraption.
I'm actually not sure though, does advdupe/advdupe2 duplicate entities that are not your own?

@TomyLobo just tested with Adv Dupe 2, they copy (and swap owner to the duping player as expected)

@TomyLobo

Copy link
Copy Markdown
Contributor

that leaves advdupe1 (and probably garry's duplicator, too)

@thegrb93

Copy link
Copy Markdown
Contributor

Although @thegrb93 I would rather not halt the chip at all, updating the owner could theoretically cause some confusion, although I doubt that much, halting the chip however makes collaboration significantly harder (I also think removing updating the owner from Starfall in favour of halting the chip is worse for the same reason, and regarding confusion, I literally never found anyone confused with SF owners updating, hell I didn't even really notice them changing automatically myself, just felt natural).

There's some cases where issues could happen. I don't think collaboration will have any negative impact

@Fasteroid

Copy link
Copy Markdown
Contributor

Although @thegrb93 I would rather not halt the chip at all, updating the owner could theoretically cause some confusion, although I doubt that much, halting the chip however makes collaboration significantly harder (I also think removing updating the owner from Starfall in favour of halting the chip is worse for the same reason, and regarding confusion, I literally never found anyone confused with SF owners updating, hell I didn't even really notice them changing automatically myself, just felt natural).

There's some cases where issues could happen. I don't think collaboration will have any negative impact

Changing the owner also felt completely natural to me (not to mention nobody on my server has ever complained about it working that way)
Could you describe in specific where "issues could happen"? Aside from the niche case the owner HAS to stay the same (in which case you could just hardcode your chip to find the correct owner), I see no issues.

@thegrb93

Copy link
Copy Markdown
Contributor

Pretty much 'niche case the owner HAS to stay the same'

@thegrb93
thegrb93 merged commit 9c9cf2d into wiremod:master Apr 25, 2021
@Derpius
Derpius deleted the e2-upload-perms-fix branch April 25, 2021 22:33
TomyLobo added a commit that referenced this pull request Apr 25, 2021
@Derpius
Derpius restored the e2-upload-perms-fix branch April 26, 2021 00:37
Fasteroid added a commit to Fasteroid/wire that referenced this pull request Apr 26, 2021
Fasteroid added a commit to Fasteroid/wire that referenced this pull request May 28, 2021
@Derpius

Derpius commented Sep 10, 2022

Copy link
Copy Markdown
Contributor Author

This was reverted and replaced with @disabled by @TomyLobo last year, however despite the vote that was taken in the Discord server being marginally in favour of @disabled, multiple people dislike it as it makes collaboration needlessly annoying.

I would suggest undoing the decision to revert this and add @disabled instead, as the only downside with my implementation is possibly unexpected behaviour if you're used to the way E2 currently handles the owner. Although I think most people will find the owner being the person who ran the chip more intuitive.

(from #off-topic today)
image
image

@CheezusChrust

CheezusChrust commented Sep 10, 2022

Copy link
Copy Markdown
Contributor

Seconded. Having @disabled added to the chip any time I'm collaborating with somebody is incredibly annoying.

@Fasteroid

Copy link
Copy Markdown
Contributor

Third "I agree", @disabled is stupid and was forced against popular opinion.

@thegrb93

Copy link
Copy Markdown
Contributor

use starfall lul

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.

Updating somone elses E2 doesn't change owner

7 participants