Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be available on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 1410114 - Double cap_free call in ping_common.c
Summary: Double cap_free call in ping_common.c
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: iputils
Version: 7.3
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: rc
: ---
Assignee: Jan Synacek
QA Contact: Robin Hack
URL:
Whiteboard:
Depends On:
Blocks: 1380361
TreeView+ depends on / blocked
 
Reported: 2017-01-04 13:54 UTC by Paulo Andrade
Modified: 2017-08-01 20:47 UTC (History)
2 users (show)

Fixed In Version: iputils-20160308-9.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-08-01 20:47:16 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2017:1987 0 normal SHIPPED_LIVE iputils bug fix update 2017-08-01 18:32:29 UTC

Description Paulo Andrade 2017-01-04 13:54:37 UTC
Pseudo patch:

modify_capability(..)
...
         if (cap_set_proc(cap_p) < 0) {
                 perror("ping: cap_set_proc");
                 goto out;
         }
 
         cap_free(cap_p);
+        cap_p = NULL;
 
         rc = 0;
 out:
         if (cap_p)
                 cap_free(cap_p);
         return rc;
...

Comment 1 Jan Synacek 2017-01-05 09:29:16 UTC
The cap_free() already does that, plus it has several sanity checks:

int cap_free(void *data_p)
{
    if ( !data_p )
        return 0;

    if ( good_cap_t(data_p) ) {
        data_p = -1 + (__u32 *) data_p;
        memset(data_p, 0, sizeof(__u32) + sizeof(struct _cap_struct));
        free(data_p);
        data_p = NULL;
        return 0;
    }

    if ( good_cap_string(data_p) ) {
        int length = strlen(data_p) + sizeof(__u32);
        data_p = -1 + (__u32 *) data_p;
        memset(data_p, 0, length);
        free(data_p);
        data_p = NULL;
        return 0;
    }

    _cap_debug("don't recognize what we're supposed to liberate");
    errno = EINVAL;
    return -1;
}

Comment 2 Paulo Andrade 2017-01-05 11:45:00 UTC
  This issue should not normally happen.

  I found it by accident while debugging the issue
that ended up with filling
https://bugzilla.redhat.com/show_bug.cgi?id=1410154
[crash due to not finding libnss_wins.so.2 dependencies in getaddrinfo]
and uses ping in one of the steps.

  I did some tests with ping from rpbmbuild/BUILD/...
and the problem still exists; it will read 4 bytes of
released memory due to calling cap_free again on a
released pointer.

first call it will be a good_cap_t(), so, it will
change the pointer to 4 bytes behind, where it
"hides" the state information, *and* release the
pointer, so, some new memory allocation can get
that same pointer at some point, and write to it;
the second call, will fail check of good_cap_t,
but only if the memory was not reallocated and
not overwritten, as it is a dangling pointer.

Comment 3 Jan Synacek 2017-01-05 12:39:05 UTC
This looks like a bug in libcap to me. Do you have a reproducer?

Comment 4 Paulo Andrade 2017-01-05 13:23:10 UTC
  This is how I found the issue:

$ rpmbuild -ba SPECS/iputils.spec 
[...]
$ sudo valgrind BUILD/iputils-s20160308/ping redhat.com
==13693== Invalid read of size 4
==13693==    at 0x4E34779: cap_free (cap_alloc.c:119)
==13693==    by 0x405CF6: modify_capability (ping_common.c:166)
==13693==    by 0x402AAA: enable_capability_raw (ping.h:271)
==13693==    by 0x402AAA: main (ping.c:449)
==13693==  Address 0x65526e0 is 0 bytes inside a block of size 36 free'd
==13693==    at 0x4C28CDD: free (vg_replace_malloc.c:530)
==13693==    by 0x4E347FF: cap_free (cap_alloc.c:122)
==13693==    by 0x405D3C: modify_capability (ping_common.c:161)
==13693==    by 0x402AAA: enable_capability_raw (ping.h:271)
==13693==    by 0x402AAA: main (ping.c:449)
==13693==  Block was alloc'd at
==13693==    at 0x4C27BE3: malloc (vg_replace_malloc.c:299)
==13693==    by 0x4E3481F: cap_init (cap_alloc.c:19)
==13693==    by 0x4E34915: cap_get_proc (cap_proc.c:16)
==13693==    by 0x405CB0: modify_capability (ping_common.c:138)
==13693==    by 0x402AAA: enable_capability_raw (ping.h:271)
==13693==    by 0x402AAA: main (ping.c:449)
==13693== 
==13693== Invalid read of size 4
==13693==    at 0x4E34779: cap_free (cap_alloc.c:119)
==13693==    by 0x405CF6: modify_capability (ping_common.c:166)
==13693==    by 0x402AF0: disable_capability_raw (ping.h:272)
==13693==    by 0x402AF0: main (ping.c:454)
==13693==  Address 0x6552750 is 0 bytes inside a block of size 36 free'd
==13693==    at 0x4C28CDD: free (vg_replace_malloc.c:530)
==13693==    by 0x4E347FF: cap_free (cap_alloc.c:122)
==13693==    by 0x405D3C: modify_capability (ping_common.c:161)
==13693==    by 0x402AF0: disable_capability_raw (ping.h:272)
==13693==    by 0x402AF0: main (ping.c:454)
==13693==  Block was alloc'd at
==13693==    at 0x4C27BE3: malloc (vg_replace_malloc.c:299)
==13693==    by 0x4E3481F: cap_init (cap_alloc.c:19)
==13693==    by 0x4E34915: cap_get_proc (cap_proc.c:16)
==13693==    by 0x405CB0: modify_capability (ping_common.c:138)
==13693==    by 0x402AF0: disable_capability_raw (ping.h:272)
==13693==    by 0x402AF0: main (ping.c:454)
==13693== 
PING redhat.com (10.4.164.55) 56(84) bytes of data.
64 bytes from redirect-redhat-com.edge.prod.ext.phx2.redhat.com (10.4.164.55): icmp_seq=1 ttl=248 time=180 ms

--- redhat.com ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 1ms
rtt min/avg/max/mdev = 180.740/180.740/180.740/0.000 ms

  I did the above mostly because I did want to run the full
process under valgrind, and as well check loading of shared
objects in the getaddrinfo call.

  It would be a far more serious issue if this were handled
after libraries were loaded, as then memory would very likely
have been released.

  It is not a bug in libcap, because the second call to cap_free
is passing, again, a pointer it had already released, and the
suggested patch just sets the pointer to NULL to remember about
it, and prevent the second cap_free call.

Comment 6 Jan Synacek 2017-01-06 12:30:16 UTC
https://github.com/iputils/iputils/pull/71

Comment 13 errata-xmlrpc 2017-08-01 20:47:16 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2017:1987


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