Page MenuHomeFreeBSD

Capsicumize ping6
ClosedPublic

Authored by asomers on Jul 24 2019, 4:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 12:35 PM
Unknown Object (File)
Sat, Mar 16, 12:35 PM
Unknown Object (File)
Sat, Mar 16, 12:35 PM
Unknown Object (File)
Sat, Mar 16, 12:35 PM
Unknown Object (File)
Dec 23 2023, 3:30 PM
Unknown Object (File)
Dec 13 2023, 8:24 AM
Unknown Object (File)
Nov 7 2023, 4:10 PM
Unknown Object (File)
Nov 4 2023, 10:55 PM

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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?

In D21050#457536, @sucanjan_gmail.com 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.

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.

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.

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.

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

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.