Merge ~azzar1/ubiquity:fix_regain_privileges_save into ubiquity:master

Proposed by Andrea Azzarone
Status: Merged
Approved by: Iain Lane
Approved revision: 92eccf445c54d05dbfc781849fed2c344daf2159
Merged at revision: 87e14f5399dbc3616889cf0ea6b34563c3793068
Proposed branch: ~azzar1/ubiquity:fix_regain_privileges_save
Merge into: ubiquity:master
Diff against target: 20 lines (+6/-0)
1 file modified
ubiquity/misc.py (+6/-0)
Reviewer Review Type Date Requested Status
Iain Lane Approve
Daniel van Vugt (community) Approve
Didier Roche (community) Approve
Review via email: mp+345056@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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

Revision history for this message
Didier Roche (didrocks) wrote :

@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
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

@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.

Revision history for this message
Andrea Azzarone (azzar1) wrote :

> @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)."

Revision history for this message
Didier Roche (didrocks) wrote :

Thanks for additional comment! LGTM.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Tested and verified bug fixed :)

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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"

Revision history for this message
Iain Lane (laney) wrote :

Alright, thanks, let's do this.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/ubiquity/misc.py b/ubiquity/misc.py
2index 0698555..5448981 100644
3--- a/ubiquity/misc.py
4+++ b/ubiquity/misc.py
5@@ -116,9 +116,15 @@ def drop_privileges_save():
6 def regain_privileges_save():
7 """Recover our real UID/GID after calling drop_privileges_save."""
8 assert _dropped_privileges is not None and _dropped_privileges > 0
9+ # We need to call os.setresuid and os.setresgid twice to avoid
10+ # permission issues when calling os.setgroups (see LP: #646827).
11+ _, euid, _ = os.getresuid()
12+ _, egid, _ = os.getresgid()
13 os.setresuid(0, 0, 0)
14 os.setresgid(0, 0, 0)
15 os.setgroups([])
16+ os.setresgid(-1, egid, -1)
17+ os.setresuid(-1, euid, -1)
18
19
20 @contextlib.contextmanager

Subscribers

People subscribed via source and target branches