Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[merged] random fixes #117

Closed
wants to merge 3 commits into from
Closed

[merged] random fixes #117

wants to merge 3 commits into from

Conversation

giuseppe
Copy link
Member

No description provided.

@@ -518,7 +518,7 @@ switch_to_user_with_privs (void)
if (prctl (PR_SET_KEEPCAPS, 1, 0, 0, 0) < 0)
die_with_error ("prctl(PR_SET_KEEPCAPS) failed");

if (setuid (real_uid) < 0)
if (setuid (opt_sandbox_uid) < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, we're not currently validating that opt_sandbox_uid being set requires --unshare-user AFAICS. Which I think means this could be used to gain uid 0 in the host userns, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if --uid was not specified (which is allowed only with --unshare-user), then opt_sandbox_uid is set to real_uid in main.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do:

  if (!opt_unshare_user && opt_sandbox_uid != real_uid)
    die ("Specifying --uid requires --unshare-user");

Which looks ok to me.

die ("Unable to set fsuid (was %d)", (int)new_fsid);

if (setfsgid (real_gid) < 0)
die_with_error ("Unable to set fsgid");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. The binary is not setgid, only setuid, so it should have the right gid always. What exactly happens that make this a problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to drop this patch as we discussed on IRC.

For the record, it was an error on my side as I was using "chmod +s" instead of "chmod u+s" for testing locally the bwrap executable

since we set uid after we are in the new namespace, use the uid in the
new user namespace.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@alexlarsson
Copy link
Collaborator

@rh-atomic-bot r+ 5f569bd

@rh-atomic-bot
Copy link

🙀 5f569bd is not a valid commit SHA. Please try again with 51b762c.

@alexlarsson
Copy link
Collaborator

@rh-atomic-bot 5f569bd

@alexlarsson
Copy link
Collaborator

@rh-atomic-bot r+ 5f569bd

@rh-atomic-bot
Copy link

🙀 5f569bd is not a valid commit SHA. Please try again with 51b762c.

@alexlarsson
Copy link
Collaborator

@rh-atomic-bot r+ 5f569bd

@rh-atomic-bot
Copy link

🙀 5f569bd3d7d06b05894a74792ad29a9ff0428379 is not a valid commit SHA. Please try again with 51b762c.

@alexlarsson
Copy link
Collaborator

@rh-atomic-bot r+ 51b762c

@rh-atomic-bot
Copy link

⌛ Testing commit 51b762c with merge d6dcdde...

rh-atomic-bot pushed a commit that referenced this pull request Nov 10, 2016
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #117
Approved by: alexlarsson
rh-atomic-bot pushed a commit that referenced this pull request Nov 10, 2016
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #117
Approved by: alexlarsson
@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: alexlarsson
Pushing d6dcdde to master...

@rh-atomic-bot rh-atomic-bot changed the title random fixes [merged] random fixes Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants