Page MenuHomeFreeBSD

Capsicumize ping6
ClosedPublic

Authored by asomers on Jul 24 2019, 4:09 AM.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

asomers created this revision.Jul 24 2019, 4:09 AM
markj added a comment.Jul 24 2019, 2:55 PM

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.

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?

markj added a comment.Jul 27 2019, 4:08 PM

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.

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

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.

Limit stdio rights.

Try to Use arc diff so that all the changes are included in the diff.

markj accepted this revision.Jul 30 2019, 1:48 PM
This revision is now accepted and ready to land.Jul 30 2019, 1:48 PM
This revision was automatically updated to reflect the committed changes.