Page MenuHomeFreeBSD

Fix a potential use after free in getsockopt() access to inp_options
ClosedPublic

Authored by jason_eggnet.com on Mar 8 2018, 3:59 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

swills added a subscriber: swills.Mar 8 2018, 4:39 PM
sbruno accepted this revision.Mar 8 2018, 5:14 PM
This revision is now accepted and ready to land.Mar 8 2018, 5:14 PM
melifaro added inline comments.
sys/netinet/ip_output.c
1247 ↗(On Diff #40074)

Shouldn't we calculate min(inp->inp_options->m_len, sopt->sopt_valsize) prior to doing the malloc()?

I'm a bit unsure what will happen if userland program calls this option with 4T buffer..

sys/netinet/ip_output.c
1247 ↗(On Diff #40074)

Yes, we should, will post fix in a few days.

sbruno requested changes to this revision.Mar 20 2018, 5:37 PM
This revision now requires changes to proceed.Mar 20 2018, 5:37 PM

fix len passed to malloc in ip_ctloutput() for getting IP_RETOPTS

use long for len passed to malloc in ip_ctloutput() for getting IP_RETOPTS

sbruno accepted this revision.Mar 22 2018, 11:37 PM
This revision is now accepted and ready to land.Mar 22 2018, 11:37 PM
melifaro added inline comments.Mar 22 2018, 11:52 PM
sys/netinet/ip_output.c
1247 ↗(On Diff #40609)

options and len have the same scope. Why do we declare them in different places?

fix scope of options to match that of len

This revision now requires review to proceed.Jun 26 2018, 11:05 PM
sbruno accepted this revision.Jun 28 2018, 1:30 AM
This revision is now accepted and ready to land.Jun 28 2018, 1:30 AM
This revision was automatically updated to reflect the committed changes.
sbruno reopened this revision.Jul 14 2018, 11:51 PM

Reopen at the request of submitter.

fix a potential use after free in access to inp_options

The latest diff is based on feedback from @jhb via @sbruno :

The output length is an in-out parameter to getsockopt() so that you can determine the size of variable-sized variables (such as this one) with this type of pattern:

socklen_t len;
void *buf;

len = 0;
getsockopt(s, IPPROTO_IP, IP_OPTIONS, NULL, &len);
buf = malloc(len);
getsockopt(s, IPPROTO_IP, IP_OPTIONS, buf, &len);

(In theory you'd really need to use a loop with realloc until you obtain a consistent snapshot.)

However, for this to work, the kernel code that calls sooptcopyout has to always pass the full size. sooptcopyout ensures it doesn't overflow the user's supplied buffer, but depends on the length it is passed to determine the output value of 'len'. The ulmin breaks this since it will return a len of 0 for the first call always.

I think the right way to fix this is to use m_dup() with M_NOWAIT while holding the INP lock and fail with ENOMEM if m_dup() fails:

INP_RLOCK(inp);
if (inp->inp_options) {
        struct mbuf *options;

        options = m_dup(inp->inp_options, M_NOWAIT);
        INP_RUNLOCK(inp);
        if (options != NULL) {
                error = sooptcopyout(sopt,
                    mtod(options, char *), options->m_len);
                m_free(options);
        } else
                error = ENOEM;
} else {
        INP_RUNLOCK(inp);
        sopt->optvalsize = 0;
}

(Someone else may prefer m_freem() to m_free() in case inp_options can be an arbitrary chain.)

I chose to always acquire INP_RLOCK because I would have had to test it again under the lock anyway before calling m_dup() to determine ENOMEM vs no valid options, and getsockopt(IP_OPTIONS) isn't a critical path such that it warrants doing an unlocked check before doing a check under the lock.

jason_eggnet.com retitled this revision from fix memory management for fetching options in ip_ctloutput() to Fix a potential use after free in getsockopt() access to inp_options.Jul 15 2018, 4:11 PM
jhb added a comment.Jul 16 2018, 5:03 PM

I think this looks fine of course since it is what I suggested (:-P), but would prefer to have someone else from transport sign off on it as well.

kbowling accepted this revision.Jul 22 2018, 7:57 PM
This revision is now accepted and ready to land.Jul 22 2018, 7:57 PM
This revision was automatically updated to reflect the committed changes.