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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25564
Build 24170: arc lint + arc unit

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
1109	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.
2603–2604	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
1821	Whitespace change.
2737	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
1109	Thanks. It has been implemented.
1821	The change has been excluded.
2603–2604	Fixed.
2737	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	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	We may use nitems.
1044	Can we limit stdio as well?
2836	If it's initializing global struct, maybe this function should just do that?
2853	You can use nitems I guess.
2856	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	It's copied from `sbin/ping/Makefile`. Should I remove the warning?
sbin/ping6/ping6.c
986	Fixed.
1044	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	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	Fixed.
2856	Fixed.

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

sbin/ping6/ping6.c
1044	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	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	I will leave it for now.
sbin/ping6/ping6.c
1044	caph_limit_stdio() keeps CAP_READ for stdin. I will use caph_limit_stdout() and caph_limit_stderr() for the other stdio descriptors.