Project

General

Profile

Bug #15424

Use fixed UID and GID for debian-tor

Added by segfault over 1 year ago. Updated about 1 year ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
03/16/2018
Due date:
% Done:

100%

Feature Branch:
bug/15424-check-if-tor-uid-changed
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Related issues

Related to Tails - Bug #15407: Prevent system user uid:s and gid:s from changing between releases Resolved 06/28/2018
Related to Tails - Bug #15419: Detect earlier in the dev process if we're breaking automatic upgrades Resolved 06/28/2018
Related to Tails - Bug #15695: Avoid breaking automatic upgrades to Tails 3.9 Resolved 06/30/2018

Associated revisions

Revision c1bd7ccb (diff)
Added by intrigeri over 1 year ago

Make code more consistent (refs: #15424).

Always use getent and quote variables.

Revision 649eeafd (diff)
Added by intrigeri over 1 year ago

Quote one more variable (refs: #15424)

Revision 7d8f7feb (diff)
Added by intrigeri over 1 year ago

Add comments to explain why we're doing that (refs: #15424)

History

#1 Updated by segfault over 1 year ago

  • Related to Bug #15407: Prevent system user uid:s and gid:s from changing between releases added

#2 Updated by segfault over 1 year ago

  • Related to Bug #15419: Detect earlier in the dev process if we're breaking automatic upgrades added

#3 Updated by segfault over 1 year ago

See branch bug/15424-check-if-tor-uid-changed in my repository.

#4 Updated by segfault over 1 year ago

  • Blocked by Bug #15422: 04-change-gids-and-uids doesn't change UIDs of existing files added

#5 Updated by intrigeri over 1 year ago

  • Status changed from Confirmed to In Progress
  • Target version set to Tails_3.6.1
  • Feature Branch set to segfault:bug/15424-check-if-tor-uid-changed

#6 Updated by intrigeri over 1 year ago

  • Assignee changed from segfault to intrigeri
  • Target version changed from Tails_3.6.1 to Tails_3.7
  • % Done changed from 0 to 20
  • QA Check set to Ready for QA
  • Feature Branch changed from segfault:bug/15424-check-if-tor-uid-changed to bug/15424-check-if-tor-uid-changed

Improved a bit, segfault says my commit looks good. But actually it's a no-op because on 3.6.1 debian-tor will get the same UID/GID as in 3.6, and we've decided to support that automatic upgrade patch => I'll test and merge into stable but only once 3.6.1 is out.

#7 Updated by intrigeri over 1 year ago

  • % Done changed from 20 to 30

Merged current stable, added two improvements on top: 649eeafd34ed99b808ee7e4aa641a2a355711910, 7d8f7feb612368d008d420606dd2473f44d416ab. I'll now try to test this:

  • in a scenario where the added code should be a no-op: currently it's the case on stable so building the topic branch is enough
  • in a scenario where the code should do something: temporarily tweak our packages list so that debian-tor's UID and GID are not the ones we want

#8 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to segfault
  • QA Check changed from Ready for QA to Dev Needed

intrigeri wrote:

I'll now try to test this:

  • in a scenario where the added code should be a no-op: currently it's the case on stable so building the topic branch is enough

This works fine.

  • in a scenario where the code should do something: temporarily tweak our packages list so that debian-tor's UID and GID are not the ones we want

This fails:

08:49:50 Change GIDs and UIDs
08:49:50 + echo Change GIDs and UIDs
08:49:50 + getent group 122
08:49:50 + awk -F : {print $1}
08:49:50 + TPS_GROUP_STEALER=Debian-gdm
08:49:50 + [ -n Debian-gdm ]
08:49:50 + groupmod --gid 150 Debian-gdm
08:49:50 + find / -wholename /proc -prune -o ( ! -type l -a -gid 122 -exec chgrp 150 {} ; )
08:49:51 + + awk -F : {print $1}
08:49:51 getent passwd 115
08:49:51 + TPS_USER_STEALER=
08:49:51 + [ -n  ]
08:49:51 + TOR_NEW_GID=114
08:49:51 + getent passwd debian-tor
08:49:51 + awk -F : {print $4}
08:49:51 Changing debian-tor GID from 115 to 114 
08:49:51 + TOR_OLD_GID=115
08:49:51 + [ 114 != 115 ]
08:49:51 + echo Changing debian-tor GID from 115 to 114 
08:49:51 + + awk -F : {print $1}
08:49:51 getent group 114
08:49:51 + TOR_GROUP_STEALER=monkeysphere
08:49:51 + [ -n monkeysphere ]
08:49:51 + echo debian-tor GID is occupied by monkeysphere
08:49:51 + groupmod --gid 151 monkeysphere
08:49:51 debian-tor GID is occupied by monkeysphere
08:49:51 + find / -wholename /proc -prune -o ( ! -type l -a -gid 114 -exec chgrp 151 {} ; )
08:49:51 + groupmod --gid 114 debian-tor
08:49:51 + find / -wholename /proc -prune -o ( ! -type l -a -gid 115 -exec chgrp 114 {} ; )
08:49:51 + TOR_NEW_UID=107
08:49:51 + + awk -Fgetent passwd debian-tor
08:49:51  : {print $3}
08:49:51 Changing debian-tor UID from 108 to 107
08:49:51 + TOR_OLD_UID=108
08:49:51 + [ 107 != 108 ]
08:49:51 + echo Changing debian-tor UID from 108 to 107
08:49:51 + awk -F+  : {print $1}
08:49:51 getent passwd 107
08:49:51 debian-tor UID is occupied by monkeysphere
08:49:51 + TOR_USER_STEALER=monkeysphere
08:49:51 + [ -n monkeysphere ]
08:49:51 + echo debian-tor UID is occupied by monkeysphere
08:49:51 + usermod --uid 151 monkeysphere
08:49:51 usermod: user monkeysphere is currently used by process 12939
08:49:51 E: config/chroot_local-hooks/04-change-gids-and-uids failed (exit non-zero). You should check for errors.

Here's the patch I use locally to test this:

diff --git a/config/chroot_local-hooks/04-change-gids-and-uids b/config/chroot_local-hooks/04-change-gids-and-uids
index 32719e2ea3..5ec9f22cdb 100755
--- a/config/chroot_local-hooks/04-change-gids-and-uids
+++ b/config/chroot_local-hooks/04-change-gids-and-uids
@@ -1,6 +1,7 @@
 #!/bin/sh

 set -e
+set -x

 echo "Change GIDs and UIDs" 

diff --git a/config/chroot_local-packageslists/tails-common.list b/config/chroot_local-packageslists/tails-common.list
index 5aa0f54a28..756f1bed80 100644
--- a/config/chroot_local-packageslists/tails-common.list
+++ b/config/chroot_local-packageslists/tails-common.list
@@ -425,3 +425,6 @@ python3-pydbus

 # For our python "shell" scripting support
 python3-sh
+
+# XXX: test!
+exim4

#9 Updated by intrigeri over 1 year ago

  • Blocked by deleted (Bug #15422: 04-change-gids-and-uids doesn't change UIDs of existing files)

#10 Updated by segfault over 1 year ago

  • Assignee changed from segfault to intrigeri
  • QA Check changed from Dev Needed to Info Needed

This fails:
[...]

So the issue is that the user to be changed still has processes running. We could try to kill all processes running by that user, and hope that it doesn't break things, but that seems error-prone to me.

We should consider other approaches. One idea I had is to put the passwd file in config/chroot_local-includes, check in a late local hook whether it was changed, and abort the build in that case. This way we would detect when ids changed and would have to manually update the passwd file.

Do you see a better solution?

#11 Updated by segfault over 1 year ago

[...] passwd file [...]

s/passwd file/ passwd and group files/

#12 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to segfault

So the issue is that the user to be changed still has processes running. We could try to kill all processes running by that user, and hope that it doesn't break things, but that seems error-prone to me.

Agreed: the risk of problematic consequences is a bit too high IMO (it can affect stuff outside of the live-build chroot) and since signals are asynchronous it's not trivial to get it right and non-racy.

We should consider other approaches. One idea I had is to put the passwd file in config/chroot_local-includes, check in a late local hook whether it was changed, and abort the build in that case. This way we would detect when ids changed and would have to manually update the passwd file.

It was useful to look for a cheap ad-hoc solution to the specific problem this ticket is about, but I think we can now conclude that it's not that cheap ⇒ I propose we reject this ticket and switch to #15407 that's about fixing the more general problem at hand wrt. changing UID/GID:s. And then if we want, or have, to implement a generic solution to #15407 and #15419, then I think what you're proposing makes sense. See the description of #15407 for a discussion about whether it's worth doing that work or not. See you there!

#13 Updated by bertagaz over 1 year ago

  • Target version changed from Tails_3.7 to Tails_3.8

#14 Updated by intrigeri about 1 year ago

  • Target version changed from Tails_3.8 to Tails_3.9

#15 Updated by intrigeri about 1 year ago

  • Status changed from In Progress to Rejected
  • Assignee deleted (segfault)
  • % Done changed from 30 to 100

I've updated #15407 and #15419. Conclusion: let's reject this but I'll reuse some of the code (the UID/GID change detection part) so it was useful!

#16 Updated by intrigeri about 1 year ago

  • Related to Bug #15695: Avoid breaking automatic upgrades to Tails 3.9 added

Also available in: Atom PDF