View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0000828 | LDMud 3.5 | Efuns | public | 2013-09-27 21:02 | 2018-01-30 03:59 |
| Reporter | zesstra | Assigned To | Gnomi | ||
| Priority | normal | Severity | major | Reproducibility | N/A |
| Status | resolved | Resolution | fixed | ||
| Target Version | 3.5.0 | Fixed in Version | 3.5.0 | ||
| Summary | 0000828: Secure or remove export_uid(). | ||||
| Description | export_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(). | ||||
| Tags | No tags attached. | ||||
|
|
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. |
|
|
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. |
|
|
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. |
|
|
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...) |
|
|
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.) |
|
|
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? |
|
|
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. |
|
|
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. |
|
|
Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 1142 for details). Thank you for reporting! |
|
|
Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 1206 for details). Thank you for reporting! |
|
|
Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 2407 for details). Thank you for reporting! |
|
|
Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 2529 for details). Thank you for reporting! |
|
|
Fix committed in revision cf06dff217a9ec4bed455885400b1832ff798a26 to master branch (see changeset 2406 for details). Thank you for reporting! |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2013-09-27 21:02 | zesstra | New Issue | |
| 2013-09-28 13:03 | fufu | Note Added: 0002209 | |
| 2013-09-28 17:19 | fufu | Note Added: 0002210 | |
| 2013-09-29 16:13 | zesstra | Note Added: 0002211 | |
| 2013-09-30 10:38 | Gnomi | Note Added: 0002214 | |
| 2013-09-30 22:07 | zesstra | Note Added: 0002215 | |
| 2016-12-08 22:35 | zesstra | Note Added: 0002279 | |
| 2016-12-08 23:47 | Gnomi | Note Added: 0002280 | |
| 2016-12-30 17:59 | zesstra | Note Added: 0002281 | |
| 2017-04-26 21:22 | Gnomi | Assigned To | => Gnomi |
| 2017-04-26 21:22 | Gnomi | Status | new => assigned |
| 2017-09-30 16:03 | Gnomi | Status | assigned => resolved |
| 2017-09-30 16:03 | Gnomi | Resolution | open => fixed |
| 2017-09-30 16:03 | Gnomi | Fixed in Version | => 3.5.0 |
| 2018-01-28 21:31 | Source_changeset_attached | => ldmud.git master cf06dff2 | |
| 2018-01-28 21:31 | zesstra | Note Added: 0002293 | |
| 2018-01-29 18:59 | Gnomi | Source_changeset_attached | => ldmud.git master cf06dff2 |
| 2018-01-29 18:59 | Gnomi | Note Added: 0002300 | |
| 2018-01-29 19:34 | Gnomi | Source_changeset_attached | => ldmud.git master cf06dff2 |
| 2018-01-29 19:34 | Gnomi | Note Added: 0002346 | |
| 2018-01-29 21:57 | Gnomi | Source_changeset_attached | => ldmud.git master cf06dff2 |
| 2018-01-29 21:57 | Gnomi | Note Added: 0002351 | |
| 2018-01-30 03:59 | Gnomi | Source_changeset_attached | => ldmud.git master cf06dff2 |
| 2018-01-30 03:59 | Gnomi | Note Added: 0002402 |
