Bug 802197

I tried to use it with Fedora 15 and 16 x86_64, so, it's not a problem related to old kernel versions.

This warning is generated due to error return code of following function call:

... setsockopt(icmp_sock, SOL_SOCKET, SO_MARK, &mark, sizeof(mark)) ...

Does it mean this socket option is obsolete?

(In reply to comment #2)
> This warning is generated due to error return code of following function call:
> 
> ... setsockopt(icmp_sock, SOL_SOCKET, SO_MARK, &mark, sizeof(mark)) ...
> 
> Does it mean this socket option is obsolete?

I don't think so, because when I got the source from http://www.skbuff.net/iputils/ and compiled ping, it worked fine although I had to run it as root.

        case SO_MARK:
                if (!capable(CAP_NET_ADMIN))
                        ret = -EPERM;
                else
                        sk->sk_mark = val;
                break;

is the code in question.  It's been like that since SO_MARK was introduced 4 years ago.  So whatever user calling ping needs to have the NET_ADMIN capability.

I really don't see this as a kernel problem.

Josh, thank you for the comment.

The ping[6] binaries use capability but only cap_net_raw+ep. I've tried adding cap_net_admin+eip. Unfortunately the warning is still printed. Any idea?

Do you have an strace of this?  Something other that actually shows the error returned by setsockopt?

strace output:

...
write(1, "PING c1.idnes.cz (194.79.52.192)"..., 55) = 55
setsockopt(3, SOL_SOCKET, SO_TIMESTAMP, [1], 4) = 0
setsockopt(3, SOL_SOCKET, 0x24 /* SO_??? */, [2], 4) = -1 EPERM (Operation not permitted)
write(2, "Warning: Failed to set mark 2\n", 30) = 30
setsockopt(3, SOL_SOCKET, SO_SNDTIMEO, "\1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 16) = 0
...


capability:

# getcap /bin/ping
/bin/ping = cap_net_admin+eip cap_net_raw+ep

Created attachment 569704 [details]
Hacky patch to fix cap handling

So I looked at the iputils code.  It does this immediately after opening the icmp_sock:

#ifdef HAVE_CAPABILITIES
        /* drop all capabilities unconditionally so even root isn't special anymore */
        caps = cap_init();
        if (cap_set_proc(caps) < 0) {
                perror("ping: cap_set_proc");
                exit(-1);
        }
        cap_free(caps);
#endif

which means it drops any and all capabilities it may have had.  That is done before the options are even processed, which means that once it gets around to calling the setup() function, it has no capabilities any longer.  The setup() function in ping_common.c is where it attempts to set SO_MARK, but since it decided it didn't want caps, it will always fail for non-root users.

The attached patch at least allows it to work if cap_net_admin is set, but it isn't complete and I'm not even sure it's a great idea to begin with.


[jwboyer@zod iputils-s20101006]$ sudo setcap - ./ping
Please enter caps for file [empty line to end]:
cap_net_raw+ep

[jwboyer@zod iputils-s20101006]$ ./ping 8.8.8.8 -m 2
Warning: Failed to set mark 2
PING 8.8.8.8 (8.8.8.8) 56(84) bytes of data.
64 bytes from 8.8.8.8: icmp_req=1 ttl=50 time=27.3 ms
64 bytes from 8.8.8.8: icmp_req=2 ttl=50 time=24.5 ms
^C
--- 8.8.8.8 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1000ms
rtt min/avg/max/mdev = 24.579/25.956/27.334/1.386 ms
[jwboyer@zod iputils-s20101006]$ sudo setcap - ./ping
Please enter caps for file [empty line to end]:
cap_net_raw+ep
cap_net_admin+eip

[jwboyer@zod iputils-s20101006]$ ./ping 8.8.8.8 -m 2
PING 8.8.8.8 (8.8.8.8) 56(84) bytes of data.
64 bytes from 8.8.8.8: icmp_req=1 ttl=50 time=25.7 ms
64 bytes from 8.8.8.8: icmp_req=2 ttl=50 time=25.8 ms
64 bytes from 8.8.8.8: icmp_req=3 ttl=50 time=25.9 ms
64 bytes from 8.8.8.8: icmp_req=4 ttl=50 time=24.1 ms
^C
--- 8.8.8.8 ping statistics ---
4 packets transmitted, 4 received, 0% packet loss, time 3005ms
rtt min/avg/max/mdev = 24.130/25.418/25.926/0.745 ms
[jwboyer@zod iputils-s20101006]$

Josh, thanks. I thought about dropping capability as a source of the issue for a while. Unfortunately I've forgot to verify this case. Sorry for this.

I don't think the current implementation checking capability for SO_MARK option is correct because the setsockopt() returns EPERM when I'm pinging as a root. This doesn't seem to be correct.

I see the issue on both sides while fixing in kernel should be easier.

Dropping capability was requested due to security reason and I don't see removing this code reasonable/possible. This option could be available only for root (preferred solution). I don's see moving dropping capability behind parsing option too probable.

(In reply to comment #9)
> Josh, thanks. I thought about dropping capability as a source of the issue for
> a while. Unfortunately I've forgot to verify this case. Sorry for this.
> 
> I don't think the current implementation checking capability for SO_MARK option
> is correct because the setsockopt() returns EPERM when I'm pinging as a root.
> This doesn't seem to be correct.

That's because it drops ALL caps.  Even for root.  The comment in the iputils code says so.

> I see the issue on both sides while fixing in kernel should be easier.

Fix it how?  By removing the cap check?  If that's the solution you desire, you'd need to send a patch upstream to convince the network developers.

> Dropping capability was requested due to security reason and I don't see
> removing this code reasonable/possible. This option could be available only for
> root (preferred solution). I don's see moving dropping capability behind
> parsing option too probable.

Yeah, it didn't really seem like a good idea to me either.

This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Since there are checks only for capabilities spread throughout the kernel, and not for caps AND root uid, I think this has to be resolved in the iputils sources.

A few solutions come to mind:

1) If set, keep cap_net_admin regardless of uid (not sure about possible security consequences) only for packet marking - this is weird, I admit

2) Don't drop capabilities when running as root - this seems as a reasonable functionality to me, as it doesn't make any sense to deny functionality to root, especially considering that caps are there to enable some of root's privileges to other users

3) Keep cap_net_admin until the check for -m option and drop accordingly - similar to Josh's solution, less hacky, yet still very hacky:)

4) Some kind of combination of the above - probably implement 2 with 3

The ideal solution would probably be to clarify and fix capabilities in the kernel, but I don't see this happening, as it would, AFAIK, require somewhat more complex rewrite.

Any ideas/suggestions welcome.

Created attachment 583806 [details]
A less hacky attempt to fix ping caps

Fix attempt. Patch against the current ping logic

The patch implements 2 + a bit of 3.

When ping is run by root, all caps stay. As I stated earlier, root is root.

When run by another user, a check for CAP_NET_ADMIN is performed. If the cap is set, it basically waits with dropping until it's done the setsockopt() to set SO_MARK. Otherwise drops all the caps straight away, as they are no longer needed.

Note that this patch fixes only ping.c, not ping6.c, and changes setup() API, thus compiling ping6.c is not possible. I made this as a short preview.

I also could not check for memleaks with valgrind when not running as root, it seems that valgrind would require the CAP_NET_ADMIN as well (?).

I also found out that I cannot set process capabilities, if they are not all cleared (would need CAP_PSET I guess). That basically rules out options 1) and part of 3).

Created attachment 594132 [details]
A less hacky attempt to fix ping caps

Fix attempt #2. Simplified the previous version. Now the patch looks almost like the original, but slightly less hacky.

Added to rawhide:
http://lists.fedoraproject.org/pipermail/scm-commits/2012-June/809383.html

We shall see.