View Issue Details

IDProjectCategoryView StatusLast Update
0000828LDMud 3.5Efunspublic2018-01-30 04:59
Reporterzesstra Assigned ToGnomi  
PrioritynormalSeveritymajorReproducibilityN/A
Status resolvedResolutionfixed 
Target Version3.5.0Fixed in Version3.5.0 
Summary0000828: Secure or remove export_uid().
Descriptionexport_uid() can be used to change an objects UID to the calling objects euid if the target object has no euid.

seteuid() usually calls valid_seteuid(), BUT: it does NOT if an object clears its euid.

The result is that there is no nice way to prevent that objects change UIDs and it can be a major loop hole in UID based access rights.
(I don't consider an sefun export_uid() "nice".)

Suggestions:
1) call valid_seteuid() in the master also for clearing the euid.
2) raise a privilege violation for export_uid().
3) remove export_uid() altogether.

Concerning 3) I quote the source of export_uid:
 * TODO: seteuid() goes through the mudlib, why not this one, too?
 * TODO:: Actually, this efun is redundant, archaic and should
 * TODO:: vanish altogether.

Actually my suggestion is 3) for 3.5.0 and also 1) for the sake of consistency of seteuid().
TagsNo tags attached.

Activities

fufu

2013-09-28 15:03

manager   ~0002209

I wouldn't miss this function. So 3) for 3.5.0 sounds good.

I disagree with 1): conceptually, set_euid(0) only drops privileges, and I see no reason to ever forbid that. The presence of export_uid() does not change that (yes, objects with euid 0 may experience sudden increases of power, but that's the fault of whoever exported their uid to them). It's just very very hard to use export_uid() safely.

The purpose of export_uid() is delegation. It comes with a hefty contract, something like

=== export_uid(X) (called by Y) contract ===
I, the undersigned, hereby grant all my rights and privileges to X. X may act on my behalf to the full extend that LPC code permits. I take full responsibility for X' actions.

Y <unreadable>
===

Calls to export_uid() should be audited, which points towards 2) as the proper solution.

Because of its contract, there is little use for export_uid() inside a mudlib. I believe it was designed for a very narrow purpose, namely for wizard tools developed in various home directories. When a wizard developed a tool, often other wizards would want to try it out. To use the tool, those wizards would have to copy it to their own home directory; (e)uids would cause trouble otherwise. export_uid() offers an alternative to copying: The wizard could grant her power to the tool (the initial euid would be 0 already on the muds I've seen) and then use it as her own.

fufu

2013-09-28 19:19

manager   ~0002210

On second thought, why does export_uid change the uid of an object and not its euid? That's strange, and indeed dangerous. Changing the euid should be enough for delegation.

zesstra

2013-09-29 18:13

administrator   ~0002211

I agree Fuchur.
And for changing the euid we already have a function - although in that case privileges are not pushed, but requested...

Concerning the problem with the wizard tools: we use the 'backbone uid mechanism', i.e. in specific directories (where objects would get the backbone uid) every object created gets the (e)UID of the object creating the new object. That UID is meant to be permanent and the mudlib master decides. That solves it sufficiently for us.

Concerning dropping the euid: yes, there seems to be little reason to forbid it. But if we raise a privilege violation upon every attempt of changing an euid it is the decision of the mud(lib) and that seems to be more 'consistent' to me. But I can live without it since we would not limit changing the euid to 0.

Gnomi

2013-09-30 12:38

manager   ~0002214

I agreee with Fuchur, that export_uid should be renamed to export_euid and change the euid. We have to keep in mind that mudlibs must be adapted accordingly, this can not be simulated, because uids become constant this way.

I'm a bit hesitant about removing this functin altogether, because it's not easy to simulate it safely. (We have the backbone uid mechanism only for mudlib objects. For wizard tools that other wizards provide there is a trust mechanism, that exports the uid only if the tool hasn't modified since the permission was given.)

Maybe extend seteuid(string euid) to seteuid(string euid, object ob)? That way export_euid can be implemented and has master oversight. (But this is a dangerous change as valid_seteuid must now also check previous_object(), maybe only allow it from master and simul-efun...)

zesstra

2013-10-01 00:07

administrator   ~0002215

We also use the BB mechanism only for mudlib objects and wizard tools located in a directory only admins have write-access to.

I think, I prefer to have a seteuid() like you described to a separate export_euid(). It is probably OK since mud admins anyway have to read the migration documents for 3.5...

(Just my personal opinion FTR: I think, immutable UIDs are a sane thing given that we have eUIDs.)

zesstra

2016-12-08 23:35

administrator   ~0002279

I guess, seteuid() with the ability to set the euid of other objects would be the most appealing approach for me. However, I don't have much preference for seteuid() or export_euid()...
But why not give valid_seteuid() both the changed object and (as new 3rd argument) the changing object as arguments?

Gnomi

2016-12-09 00:47

manager   ~0002280

On existing implementations of valid_seteuid() the new third parameter would be silently ignored (if some mud admin overlooked that change and didn't adapt the mudlib accordingly). I'm not sure about the security implications of that. Other objects would then be able to do euid changes that only the object could do to itself.

That's why I'm thinking about entirely new interface for that, something like configure_object(ob, OC_EUID) with its privilege checks.

zesstra

2016-12-30 18:59

administrator   ~0002281

Ah, yes. This is clear - my confusion was the the requirement to check previous_object() instead of an explicit argument.

I believe, during the update to 3.5.x mud admins anyway have to check for a bunch of stuff. However, a new interface is a safer way, I agree. And operators could create a simul_efun facilitating configure_object(ob, OC_EUID) if desired.

From my viewpoint as admin in Morgengrauen: I can live with both approaches, we use export_uid and allow it anyway only for ROOT objects.

zesstra

2018-01-28 22:31

administrator   ~0002293

Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 1142 for details). Thank you for reporting!

Gnomi

2018-01-29 19:59

manager   ~0002300

Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 1206 for details). Thank you for reporting!

Gnomi

2018-01-29 20:34

manager   ~0002346

Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 2407 for details). Thank you for reporting!

Gnomi

2018-01-29 22:57

manager   ~0002351

Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 2529 for details). Thank you for reporting!

Gnomi

2018-01-30 04:59

manager   ~0002402

Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 2406 for details). Thank you for reporting!

Issue History

Date Modified Username Field Change
2013-09-27 23:02 zesstra New Issue
2013-09-28 15:03 fufu Note Added: 0002209
2013-09-28 19:19 fufu Note Added: 0002210
2013-09-29 18:13 zesstra Note Added: 0002211
2013-09-30 12:38 Gnomi Note Added: 0002214
2013-10-01 00:07 zesstra Note Added: 0002215
2016-12-08 23:35 zesstra Note Added: 0002279
2016-12-09 00:47 Gnomi Note Added: 0002280
2016-12-30 18:59 zesstra Note Added: 0002281
2017-04-26 23:22 Gnomi Assigned To => Gnomi
2017-04-26 23:22 Gnomi Status new => assigned
2017-09-30 18:03 Gnomi Status assigned => resolved
2017-09-30 18:03 Gnomi Resolution open => fixed
2017-09-30 18:03 Gnomi Fixed in Version => 3.5.0
2018-01-28 22:31 Source_changeset_attached => ldmud.git master cf06dff2
2018-01-28 22:31 zesstra Note Added: 0002293
2018-01-29 19:59 Gnomi Source_changeset_attached => ldmud.git master cf06dff2
2018-01-29 19:59 Gnomi Note Added: 0002300
2018-01-29 20:34 Gnomi Source_changeset_attached => ldmud.git master cf06dff2
2018-01-29 20:34 Gnomi Note Added: 0002346
2018-01-29 22:57 Gnomi Source_changeset_attached => ldmud.git master cf06dff2
2018-01-29 22:57 Gnomi Note Added: 0002351
2018-01-30 04:59 Gnomi Source_changeset_attached => ldmud.git master cf06dff2
2018-01-30 04:59 Gnomi Note Added: 0002402