Merge ~azzar1/ubiquity:fix_regain_privileges_save into ubiquity:master

[Andrea Azzarone]

misc.py: Restore the corrent euid in regain_privileges_save

Calling regain_privileges_save should restore the effective user-id to the one
before the call to drop_privileges_save. We need to call os.setresuid and
os.setresgid twice to avoid permission issues when calling os.setgroups.

Fixes LP: #1751252

drop_privileges_save works if and only if drop_privileges has already been called.

The intial situation is (ruid, euid, suid) = (0, 0, 0)
After drop_privileges we get (ruid, euid, suid) = (0, 999, 0)
After drop_privileges_save we get (ruid, euid, suid) = (999, 999, 0)

Atm if we call regain_privileges_save we get (0, 0, 0) but we should get (0, 999, 0). It used to be like that, than https://git.launchpad.net/ubiquity/commit/?id=815fbf12 introduced the regression.

[Daniel van Vugt (vanvugt)]

Please add a mention of LP: #1751252 in the commit message.

[Daniel van Vugt (vanvugt)]

Also, is git really the right place? Do we need to update the Vcs fields?

$ apt-get source ubiquity
Reading package lists... Done
NOTICE: 'ubiquity' packaging is maintained in the 'Bzr' version control system at:
http://bazaar.launchpad.net/~ubuntu-installer/ubiquity/trunk
Please use:
bzr branch http://bazaar.launchpad.net/~ubuntu-installer/ubiquity/trunk

[Didier Roche-Tolomelli (didrocks)]

@vanvugt: I think ubiquity just moved to git

From my comments on IRC:

I don't understand why we don't call directly os.setresuid(0, egid, 0) for instance. This is probably because os.setgroups([]) has an incidence, but I've never used that one

Preservering the euid and geuid is needed for sure and replacing with 0 for uid/guid was a mistake
but I don't understand why the calls in 2 steps. Do you mind expanding (and adding a comment?)

review: Needs Information

[Dimitri John Ledkov (xnox)]

@vanvugt

we did move to git, but there was not an upload yet pointing to the new location.

Imho, Laney should review this, as he just had to fix euid missmatch of expectations this release sprint.

[Andrea Azzarone (azzar1)]

> @vanvugt: I think ubiquity just moved to git
>
> From my comments on IRC:
>
> I don't understand why we don't call directly os.setresuid(0, egid, 0) for
> instance. This is probably because os.setgroups([]) has an incidence, but I've
> never used that one
>
> Preservering the euid and geuid is needed for sure and replacing with 0 for
> uid/guid was a mistake
> but I don't understand why the calls in 2 steps. Do you mind expanding (and
> adding a comment?)

From https://git.launchpad.net/ubiquity/commit/?id=815fbf12 : "Set the effective UID in regain_privileges_save so we don't try to setgroups([]) as a regular user (LP: #646827)."

[Didier Roche-Tolomelli (didrocks)]

Thanks for additional comment! LGTM.

review: Approve

[Daniel van Vugt (vanvugt)]

Tested and verified bug fixed :)

review: Approve

[Daniel van Vugt (vanvugt)]

Although you might want to extend the commit message to say something like

"... to avoid triggering cairo crash https://bugs.freedesktop.org/show_bug.cgi?id=98883"

[Iain Lane (laney)]

Alright, thanks, let's do this.

review: Approve

diff --git a/ubiquity/misc.py b/ubiquity/misc.py
index 0698555..5448981 100644
--- a/ubiquity/misc.py
+++ b/ubiquity/misc.py
@@ -116,9 +116,15 @@ def drop_privileges_save():
 def regain_privileges_save():
     """Recover our real UID/GID after calling drop_privileges_save."""
     assert _dropped_privileges is not None and _dropped_privileges > 0
+    # We need to call os.setresuid and os.setresgid twice to avoid
+    # permission issues when calling os.setgroups (see LP: #646827).
+    _, euid, _ = os.getresuid()
+    _, egid, _ = os.getresgid()
     os.setresuid(0, 0, 0)
     os.setresgid(0, 0, 0)
     os.setgroups([])
+    os.setresgid(-1, egid, -1)
+    os.setresuid(-1, euid, -1)
 
 
 @contextlib.contextmanager