Bug 802197 - ping failed to set mark on icmp packets
ping failed to set mark on icmp packets
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: iputils (Show other bugs)
rawhide
x86_64 Linux
unspecified Severity medium
: ---
: ---
Assigned To: Jan Synacek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-11 22:23 EDT by Rodolfo
Modified: 2012-06-26 04:14 EDT (History)
7 users (show)

See Also:
Fixed In Version: iputils-20101006-15.fc18
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-26 04:14:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Hacky patch to fix cap handling (1.79 KB, patch)
2012-03-13 11:36 EDT, Josh Boyer
no flags Details | Diff
A less hacky attempt to fix ping caps (3.64 KB, patch)
2012-05-11 07:19 EDT, Jan Synacek
no flags Details | Diff
A less hacky attempt to fix ping caps (2.64 KB, patch)
2012-06-25 04:52 EDT, Jan Synacek
no flags Details | Diff

  None (edit)
Description Rodolfo 2012-03-11 22:23:24 EDT
Description of problem:
When you use ping with "m" option to set mark on packets you get a warning message telling that it has failed to set mark.

Version-Release number of selected component (if applicable):
iputils-sss20101006

How reproducible:
Try to use ping with the "m" option.

Steps to Reproduce:
1.Open the terminal.
2.Type "ping 8.8.8.8 -m 2"
3.Press enter
  
Actual results:
"Warning: Failed to set mark 2"

Expected results:
Quietly mark the packets
Comment 1 Rodolfo 2012-03-11 22:43:39 EDT
I tried to use it with Fedora 15 and 16 x86_64, so, it's not a problem related to old kernel versions.
Comment 2 Jiri Skala 2012-03-12 09:02:16 EDT
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?
Comment 3 Rodolfo 2012-03-12 14:13:07 EDT
(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.
Comment 4 Josh Boyer 2012-03-12 14:24:00 EDT
        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.
Comment 5 Jiri Skala 2012-03-13 05:07:19 EDT
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?
Comment 6 Josh Boyer 2012-03-13 09:23:50 EDT
Do you have an strace of this?  Something other that actually shows the error returned by setsockopt?
Comment 7 Jiri Skala 2012-03-13 09:55:41 EDT
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
Comment 8 Josh Boyer 2012-03-13 11:36:26 EDT
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]$
Comment 9 Jiri Skala 2012-03-13 12:16:03 EDT
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.
Comment 10 Josh Boyer 2012-03-13 12:37:46 EDT
(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.
Comment 11 Fedora Admin XMLRPC Client 2012-03-29 04:31:27 EDT
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.
Comment 12 Jan Synacek 2012-05-11 02:14:59 EDT
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.
Comment 13 Jan Synacek 2012-05-11 07:19:02 EDT
Created attachment 583806 [details]
A less hacky attempt to fix ping caps

Fix attempt. Patch against the current ping logic
Comment 14 Jan Synacek 2012-05-11 07:27:00 EDT
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 (?).
Comment 15 Jan Synacek 2012-05-11 07:30:10 EDT
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).
Comment 16 Jan Synacek 2012-06-25 04:52:23 EDT
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.
Comment 17 Jan Synacek 2012-06-26 03:58:30 EDT
Added to rawhide:
http://lists.fedoraproject.org/pipermail/scm-commits/2012-June/809383.html

We shall see.

Note You need to log in before you can comment on or make changes to this bug.