Opened 9 years ago

Closed 9 years ago

#268 closed enhancement (fixed)

review: setuid via -u on b10-auth

Reported by: shane Owned by: jelte
Priority: medium Milestone: 06. 4th Incremental Release
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 1.0 Internal?: no

Description

Now that the boss process has the "-u" flag, we need to complete the task of dropping privileges by also doing it for the b10-auth process.

This is the 2nd half of ticket #180, both of which are a preamble for doing permissions "properly" via PriviligedSocketCreator?.

Subtickets

Change History (17)

comment:1 Changed 9 years ago by shane

  • Summary changed from setuide via -u on b10-auth to setuid via -u on b10-auth

comment:2 Changed 9 years ago by jinmei

  • billable set to 1
  • Internal? unset

branches/trac268 is ready for review.

diff can be retrieved by:

svn diff -r2524 svn+ssh://bind10.isc.org/svn/bind10/branches/trac268

The patch should be straightforward.

Proposed changelog entry is:

  76.?	[func]		jinmei
	bin/auth: Added -u option to allow the effective process user
	of the authoritative server after invocation.  The same option to
	the boss process will be propagated to b10-auth, too.
	(Trac #268, svnTBD)

p.s. once this patch is merged I'll start my own dogfood:-)

comment:3 Changed 9 years ago by jinmei

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:4 Changed 9 years ago by jinmei

  • Summary changed from setuid via -u on b10-auth to review: setuid via -u on b10-auth

comment:5 Changed 9 years ago by jinmei

slightly updating the proposed changelog entry:

  76.?	[func]		jinmei
	bin/auth: Added -u option to allow changing the effective process
	user of the authoritative server after invocation.  The same
	option to the boss process will be propagated to b10-auth, too.
	(Trac #268, svnTBD)

comment:6 Changed 9 years ago by shane

  • Owner changed from UnAssigned to shane

comment:7 follow-up: Changed 9 years ago by shane

  • Add Hours to Ticket changed from 0.0 to 1.0
  • Owner changed from shane to jinmei
  • Total Hours changed from 0.0 to 1.0

One bug:

If the user name does not exist on the system, then getpwnam() will return NULL. The code will then try to use value as a PID. If this is not a valid PID then we will get a casting error and raise a "Failed to parse" exception. We should probably not raise an "Failed to parse" exception but rather let the code fall through and raise the "Unknown user name or UID" exception. This is because a valid - but non-existent - user name should not signal a parse error.

Concern about reentrancy:

The code uses getpwnam() and getpwuid(), neither of which is re-entrant. While we don't expect this code to be used generally, it is probably worth it to use getpwnam_r() and getpwuid_r() instead.

Proposal:

When the code fails after setuid(), we leave the group ID set. Perhaps we can try to change back to the original group ID in that case? It's an attempt to have fewer side effects...

ChangeLog:

The proposed ChangeLog update mentions effective user ID. However we change the real user ID. I suggest just removing the word "effective".

Documentation:

The b10-auth.8 page is not updated. Maybe this is something to leave for Jeremy, but our review procedure mentions documentation. :)

Note about tests:

The boss process tests do not check the user of the subprocesses. I couldn't think of a portable way to do this. It's not important - no changes to this patch are necessary- but thought I would mention it.

comment:8 in reply to: ↑ 7 Changed 9 years ago by jinmei

  • Owner changed from jinmei to shane

Replying to shane:

Thanks for the review.

I've adopted some of the suggestions, and in what follows explained my
thoughts about the comments I didn't apply for now. Please check.

One bug:

If the user name does not exist on the system, then getpwnam() will return NULL. The code will then try to use value as a PID. If this is not a valid PID then we will get a casting error and raise a "Failed to parse" exception. We should probably not raise an "Failed to parse" exception but rather let the code fall through and raise the "Unknown user name or UID" exception. This is because a valid - but non-existent - user name should not signal a parse error.

Changed the code this way. r2571.

ChangeLog:

The proposed ChangeLog update mentions effective user ID. However we change the real user ID. I suggest just removing the word "effective".

Okay. I've also updated the description in change_user.h.

I've not made changes to the following at the moment:

Concern about reentrancy:

The code uses getpwnam() and getpwuid(), neither of which is re-entrant. While we don't expect this code to be used generally, it is probably worth it to use getpwnam_r() and getpwuid_r() instead.

I noticed the reentrancy issue, but chose to not use the _r version for simplicity. In practice, we won't even call this function more than once, and this function may be a shortterm workaround and isn't intended to be called outside the auth server implementation, so I didn't think it makes much sense to make it perfect with additional code. Maybe we should note this in the function document though.

But if you think we should still ensure the reentrancy, I don't oppose
to that.

Proposal:

When the code fails after setuid(), we leave the group ID set. Perhaps we can try to change back to the original group ID in that case? It's an attempt to have fewer side effects...

Like the issue of reentrancy, I personally don't think the gain isn't
worth the complexity of the additional code. I generally prefer
providing stronger exception/failure guarantees, but, realistically,
if setuid() fails the server will simply die. So, I'd like to simply
note this function only provides a weak (basic) guarantee and an
intermediate change could remain on failure (and in any case the
expected behavior is to terminate the process at the caller side).

Again, I don't necessarily oppose to providing a stronger guarantee if
you strongly want.

Documentation:

The b10-auth.8 page is not updated. Maybe this is something to leave for Jeremy, but our review procedure mentions documentation. :)

Okay, added. r2572.
(there are other options that are not documented)

Note about tests:

The boss process tests do not check the user of the subprocesses. I couldn't think of a portable way to do this. It's not important - no changes to this patch are necessary- but thought I would mention it.

I noticed the boss process didn't have tests for '-u' and understood
it wouldn't be easy. Also, the test wouldn't specific to this option
for b10-auth. So (while I knew it was not ideal) I didn't touch this
part this time.

comment:9 Changed 9 years ago by jreed

  • Owner changed from shane to UnAssigned

comment:10 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte
  • Status changed from reviewing to accepted

comment:11 follow-up: Changed 9 years ago by jelte

  • Status changed from accepted to reviewing

I think this is ok and can be merged.

One minor nit/question;

In the last unit test, should we try to check for uid, not username? (not sure what USER is set to if you have a second superuser. If this is still root there's no problem. On some OS'es i think there are even other ways to be a superuser, but this whole problem may be gone before we encounter that anyway, if ever)

comment:12 Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

comment:13 in reply to: ↑ 11 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

Replying to jelte:

I think this is ok and can be merged.

One minor nit/question;

In the last unit test, should we try to check for uid, not username? (not sure what USER is set to if you have a second superuser. If this is still root there's no problem. On some OS'es i think there are even other ways to be a superuser, but this whole problem may be gone before we encounter that anyway, if ever)

Do you mean it's better to do this?

    if (getuid() == 0) {
        cerr << "Already a super user, skipping the test" << endl;
        return;
    }

I'm okay with that, although it may cause another question that we can always assume the user ID of 0 means a super user (or does the standard reserve this ID for this purpose?).

BTW, I've added some comments based on my previous respnse to Shane. It's r2667.

comment:14 follow-up: Changed 9 years ago by jelte

Well the old trick for getting a second superuser account is to set its uid to 0. Don't know if it's in the standards (i suspect they left specifying that out on purpose), and I *think* that having a second uid=0 account and then asking for its username will return root again, so in practice I don't think it really matters, but i believe that that is the check the system actually makes (unless there are capabilities or other such modern nonsense)

comment:15 in reply to: ↑ 14 Changed 9 years ago by jinmei

Replying to jelte:

Well the old trick for getting a second superuser account is to set its uid to 0. Don't know if it's in the standards (i suspect they left specifying that out on purpose), and I *think* that having a second uid=0 account and then asking for its username will return root again, so in practice I don't think it really matters, but i believe that that is the check the system actually makes (unless there are capabilities or other such modern nonsense)

I've changed the check to uid==0 (r2670).

I've googled on whether it's specified in some standard or just a convention, but couldn't find an answer. But at least uid==0 wouldn't be worse than username="root", and probably a bit more portable.

Is it now okay to merge?

comment:16 Changed 9 years ago by jelte

Ack. Woohoo! another one down :)

comment:17 Changed 9 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed

okay, thanks. merged to trunk, closing.

Note: See TracTickets for help on using tickets.