Bugzilla will be upgraded to version 5.0. The upgrade date is tentatively scheduled for 2 December 2018, pending final testing and feedback.
Bug 1410114 - Double cap_free call in ping_common.c
Double cap_free call in ping_common.c
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: iputils (Show other bugs)
7.3
All Linux
unspecified Severity medium
: rc
: ---
Assigned To: Jan Synacek
Robin Hack
: Patch, Reopened
Depends On:
Blocks: 1380361
  Show dependency treegraph
 
Reported: 2017-01-04 08:54 EST by Paulo Andrade
Modified: 2017-08-01 16:47 EDT (History)
2 users (show)

See Also:
Fixed In Version: iputils-20160308-9.el7
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-08-01 16:47:16 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2017:1987 normal SHIPPED_LIVE iputils bug fix update 2017-08-01 14:32:29 EDT

  None (edit)
Description Paulo Andrade 2017-01-04 08:54:37 EST
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 04:29:16 EST
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 06:45:00 EST
  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 07:39:05 EST
This looks like a bug in libcap to me. Do you have a reproducer?
Comment 4 Paulo Andrade 2017-01-05 08:23:10 EST
  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 07:30:16 EST
https://github.com/iputils/iputils/pull/71
Comment 13 errata-xmlrpc 2017-08-01 16:47:16 EDT
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.