Details

Reviewers

pjd
markj
oshogbo

Commits

rS351962: MFC r350556:
rS350556: Capsicumize ping6

Summary

Capsicumize ping6

Add capsicum support to ping6, mostly copying the strategy used for ping.

Submitted by: Ján Sučan <jansucan@gmail.com>
Sponsored by: Google, inc. (Google Summer of Code 2019)

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

asomers created this revision.Jul 24 2019, 4:09 AM

Herald added subscribers: jonathan, imp. · View Herald TranscriptJul 24 2019, 4:09 AM

Harbormaster completed remote builds in B25483: Diff 60077.Jul 24 2019, 4:09 AM

Nice. This mostly looks good to me. There are some seemingly unrelated changes: we drop privileges earlier, we set SO_DEBUG on both sockets, some whitespace changes. I would suggest having those be separate commits.

sbin/ping6/ping6.c
1080 ↗	(On Diff #60077)	Rather than duplicating the lists of rights, you can define two separate cap_rights_t sets, one for each socket, and use cap_rights_clear() to remove CAP_SETSOCKOPT before further limiting rights on those sockets.
2555 ↗	(On Diff #60077)	This line should be wrapped to 80 columns.

markj added a reviewer: oshogbo.Jul 24 2019, 2:55 PM

jansucan added a subscriber: jansucan.Jul 24 2019, 7:05 PM

In D21050#456890, @markj wrote:

There are some seemingly unrelated changes: we drop privileges earlier, we set SO_DEBUG on both sockets, some whitespace changes. I would suggest having those be separate commits.

I will implement dropping privileges earlier in a separate commit.
Could you please explain why is setting SO_DEBUG on both socket considered unrelated to the capsicumization?
I am not able to identify the whitespace changes. Could you point them out?

In D21050#457536, @sucanjan_gmail.com wrote:

In D21050#456890, @markj wrote:

There are some seemingly unrelated changes: we drop privileges earlier, we set SO_DEBUG on both sockets, some whitespace changes. I would suggest having those be separate commits.

I will implement dropping privileges earlier in a separate commit.

Thanks.

Could you please explain why is setting SO_DEBUG on both socket considered unrelated to the capsicumization?

I had misunderstood how srecv is used, please ignore my comment about that.

I am not able to identify the whitespace changes. Could you point them out?

I pointed out two. I thought I saw more when I took a first pass through the diff, but I can't find them.

sbin/ping6/ping6.c
1794 ↗	(On Diff #60077)	Whitespace change.
2688 ↗	(On Diff #60077)	Whitespace change.

Define two separate cap_rights_t sets, one for each socket, and use cap_rights_clear() to remove CAP_SETSOCKOPT before further limiting rights on those sockets.

Wrap a long line to 80 columns.

Do not change whitespaces.

Harbormaster completed remote builds in B25542: Diff 60200.Jul 27 2019, 7:26 PM

jansucan added inline comments.Jul 27 2019, 7:33 PM

sbin/ping6/ping6.c
1080 ↗	(On Diff #60077)	Thanks. It has been implemented.
1794 ↗	(On Diff #60077)	The change has been excluded.
2555 ↗	(On Diff #60077)	Fixed.
2688 ↗	(On Diff #60077)	The change has been excluded.

markj accepted this revision.Jul 28 2019, 6:35 PM

This revision is now accepted and ready to land.Jul 28 2019, 6:35 PM

In general path looks good to me.
I have some minor points.

sbin/ping6/Makefile
18 ↗	(On Diff #60200)	Do we want this warning? If so: It's a little bit more complicated because if somebody don't want CASPER explicite and don't build with DYNAMICROOT he will get this warning.
sbin/ping6/ping6.c
986 ↗	(On Diff #60200)	We may use nitems.
1044 ↗	(On Diff #60200)	Can we limit stdio as well?
2836 ↗	(On Diff #60200)	If it's initializing global struct, maybe this function should just do that?
2853 ↗	(On Diff #60200)	You can use nitems I guess.
2856 ↗	(On Diff #60200)	Same here.

Use nitems() for getting size of an array.

This revision now requires review to proceed.Jul 29 2019, 2:57 PM

Harbormaster completed remote builds in B25564: Diff 60233.Jul 29 2019, 2:57 PM

jansucan added inline comments.Jul 29 2019, 2:57 PM

sbin/ping6/Makefile
18 ↗	(On Diff #60200)	It's copied from `sbin/ping/Makefile`. Should I remove the warning?
sbin/ping6/ping6.c
986 ↗	(On Diff #60200)	Fixed.
1044 ↗	(On Diff #60200)	Like this? cap_rights_init(&rights_stdio, CAP_WRITE); if (caph_rights_limit(STDOUT_FILENO, &rights_stdio) < 0) err(1, "cap_rights_limit stdout"); if (caph_rights_limit(STDERR_FILENO, &rights_stdio) < 0) err(1, "cap_rights_limit stderr"); cap_rights_clear(&rights_stdio, CAP_WRITE); if (caph_rights_limit(STDIN_FILENO, &rights_stdio) < 0) err(1, "cap_rights_limit stdin");
2836 ↗	(On Diff #60200)	I'm not sure if I understand. Do you mean that the return value should be changed to `void` and the function should set the global variable directly?
2853 ↗	(On Diff #60200)	Fixed.
2856 ↗	(On Diff #60200)	Fixed.

markj added inline comments.Jul 29 2019, 3:03 PM

sbin/ping6/ping6.c
1044 ↗	(On Diff #60200)	The caph_limit_stdio() helper can be used instead. It looks like IPv4 ping is missing this as well.

asomers removed a subscriber: jansucan.Jul 29 2019, 5:32 PM

asomers added inline comments.

sbin/ping6/Makefile
18 ↗	(On Diff #60200)	Since the warning is already present, you should leave it in for now. If you decide to remove it, do that for both ping and ping6, and do it in a separate commit.

jansucan added inline comments.Jul 30 2019, 6:56 AM

sbin/ping6/Makefile
18 ↗	(On Diff #60200)	I will leave it for now.
sbin/ping6/ping6.c
1044 ↗	(On Diff #60200)	caph_limit_stdio() keeps CAP_READ for stdin. I will use caph_limit_stdout() and caph_limit_stderr() for the other stdio descriptors.