process icon indicating copy to clipboard operation
process copied to clipboard

Reconsider #44 (change user and group)

Open blackgnezdo opened this issue 3 years ago • 2 comments

The capability to change user and group was added 6 years ago in #45. There a few reasons why I believe this feature may not be pulling its weight:

  • doing setuid dance correctly in a cross-platform way is hard (consider how much effort openssh-portable goes through)
  • there's a TODO acknowledging the questionable correctness of the implementation
  • posix_spawn API doesn't support this capability so this feature requires keeping both code paths and runtime fall-back which is unlikely to be a well-tested scenario.
  • no tests cover this functionality (because it is really inconvenient to test)

@jprider63 do you still use this feature that you added? Can somebody think of a way to survey the ecosystem to figure out how much usage the feature received?

In summary, I believe the feature is likely not secure, contains race conditions, complicates the codebase, and has no tests. This seems like a good candidate for removal.

blackgnezdo avatar Jan 12 '22 04:01 blackgnezdo

Hi @blackgnezdo. Yes, child_user and child_group are still used in internal code.

jprider63 avatar Jan 12 '22 13:01 jprider63

For what it's worth, I tend to agree that user and group changing isn't pulling its weight. This sort of functionality is extremely platform-dependent and ripe for nasty (potentially exploitable) bugs (e.g. #149).

bgamari avatar Jan 14 '22 17:01 bgamari