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.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 17, 6:01 AM
Unknown Object (File)
Feb 12 2024, 1:43 AM
Unknown Object (File)
Jan 13 2024, 11:44 PM
Unknown Object (File)
Dec 23 2023, 2:00 PM
Unknown Object (File)
Dec 23 2023, 1:59 PM
Unknown Object (File)
Dec 23 2023, 1:59 PM
Unknown Object (File)
Dec 23 2023, 1:59 PM
Unknown Object (File)
Dec 23 2023, 1:59 PM

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 15727
Build 15749: arc lint + arc unit

Event Timeline

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

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

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

This revision is now accepted and ready to land.Mar 22 2018, 11:37 PM
sys/netinet/ip_output.c
1247

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
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.

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

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.

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.