Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

netinet: prevent NULL pointer dereference in in_aifaddr_ioctl() #530

Closed
wants to merge 1 commit into from
Closed

netinet: prevent NULL pointer dereference in in_aifaddr_ioctl() #530

wants to merge 1 commit into from

Conversation

akhramov
Copy link
Contributor

@akhramov akhramov commented Aug 23, 2021 •
edited
Loading

netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()

It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in in_aifaddr_ioctl(). In order to replicate
that, one needs to

  1. Ensure that carp(4) is not loaded

  2. Issue SIOCAIFADDR call setting ifra_vhid field of the request
    to a negative value.

A repro code would look like this.

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}

This change

  • in addition to positive, discards negative vhid values in
    in_aifaddr_ioctl, if carp(4) is not loaded. This prevents NULL
    pointer dereference and kernel panic.

@akhramov
Copy link
Contributor Author

akhramov commented Aug 24, 2021 •
edited
Loading

@bsdimp can you take a look at this one, please? 🙂

the tests are broken in the target branch, too.

bsdimp
bsdimp reviewed Aug 24, 2021
View reviewed changes
share/man/man4/netintro.4 Outdated
@@ -397,6 +397,7 @@ struct ifaliasreq {
struct sockaddr ifra_addr;
struct sockaddr ifra_broadaddr;
struct sockaddr ifra_mask;
int ifra_vhid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks unrelated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg, should be a separate change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks.

bsdimp
bsdimp reviewed Aug 24, 2021
View reviewed changes
sys/netinet/in.c Outdated
@@ -452,7 +452,7 @@ in_aifaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, struct thread *td)
if (ifp->if_flags & IFF_POINTOPOINT)
ia->ia_dstaddr = *dstaddr;

if (vhid != 0) {
if (vhid != 0 && carp_attach_p != NULL) {
Copy link
Member

@bsdimp bsdimp Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why isn't the check at line 380 enough to keep this from happening? (silly github doesn't let me toss a comment on that line)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

freebsd-src/sys/netinet/in.c

Lines 379 to 380 in 80b1995

if (vhid > 0 && carp_attach_p == NULL)
return (EPROTONOSUPPORT);

In our case vhid is negative

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other places, we reject as invalide vhids < 0... I think that someone more familiar with carp than I am is going to look at this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan, thanks! I hope the repro's going to be of use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked Kristof to take a look. His question was why not change the above to vhid != 0? It's not valid to set vhid to any value when carp isn't loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great suggestion. Implemented it.

@akhramov akhramov force-pushed the fix/SIOCGIFADDR-panic branch 2 times, most recently from 7d64200 to 60b0320 Compare August 24, 2021 15:59
@akhramov
netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()
ca4ab95 
It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in `in_aifaddr_ioctl()`. In order to replicate
that, one needs to

1. Ensure that `carp(4)` is not loaded

2. Issue `SIOCAIFADDR` call setting `ifra_vhid` field of the request
   to a negative value.

A repro code would look like this.

```c

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}
```

This change

* in addition to positive, discards negative `vhid` values in
  `in_aifaddr_ioctl`, if `carp(4)` is not loaded. This prevents NULL
  pointer dereference and kernel panic.
@akhramov
Copy link
Contributor Author

akhramov commented Aug 26, 2021 •
edited
Loading

@bsdimp sorry to bug you, but what'd be the next steps? Should we backport the fix to previous releases? They seem to be affected too.

@akhramov akhramov requested a review from bsdimp August 26, 2021 10:24
@bsdimp
Copy link
Member

bsdimp commented Aug 26, 2021

I'll merge it to -current. If there's issues in the back port, I'll be in touch, but it's simple enough it should just apply to stable/12 and stable/13.

@bsdimp
Copy link
Member

bsdimp commented Aug 26, 2021

Committed in 620cf65

@bsdimp bsdimp closed this Aug 26, 2021
@bsdimp
Copy link
Member

bsdimp commented Aug 26, 2021

And the cherry-pick succeeded, so I'll commit those to stable/12 and 13 maybe next week...

freebsd-git pushed a commit that referenced this pull request Aug 26, 2021
@akhramov @bsdimp
netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()
620cf65 
It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in in_aifaddr_ioctl(). In order to replicate
that, one needs to

1. Ensure that carp(4) is not loaded

2. Issue SIOCAIFADDR call setting ifra_vhid field of the request
   to a negative value.

A repro code would look like this.

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}

To fix, discard both positive and negative vhid values in
in_aifaddr_ioctl, if carp(4) is not loaded. This prevents NULL pointer
dereference and kernel panic.

Reviewed by:	imp@
Pull Request:	#530
freebsd-git pushed a commit that referenced this pull request Sep 12, 2021
@akhramov @bsdimp
netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()
7a26124 
It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in in_aifaddr_ioctl(). In order to replicate
that, one needs to

1. Ensure that carp(4) is not loaded

2. Issue SIOCAIFADDR call setting ifra_vhid field of the request
   to a negative value.

A repro code would look like this.

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}

To fix, discard both positive and negative vhid values in
in_aifaddr_ioctl, if carp(4) is not loaded. This prevents NULL pointer
dereference and kernel panic.

Reviewed by:	imp@
Pull Request:	#530

(cherry picked from commit 620cf65)
freebsd-git pushed a commit that referenced this pull request Sep 13, 2021
@akhramov @bsdimp
netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()
8d783b1 
It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in in_aifaddr_ioctl(). In order to replicate
that, one needs to

1. Ensure that carp(4) is not loaded

2. Issue SIOCAIFADDR call setting ifra_vhid field of the request
   to a negative value.

A repro code would look like this.

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}

To fix, discard both positive and negative vhid values in
in_aifaddr_ioctl, if carp(4) is not loaded. This prevents NULL pointer
dereference and kernel panic.

Reviewed by:	imp@
Pull Request:	#530

(cherry picked from commit 620cf65)
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Dec 27, 2021
@akhramov @bsdjhb
netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()
be2d97a 
It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in in_aifaddr_ioctl(). In order to replicate
that, one needs to

1. Ensure that carp(4) is not loaded

2. Issue SIOCAIFADDR call setting ifra_vhid field of the request
   to a negative value.

A repro code would look like this.

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}

To fix, discard both positive and negative vhid values in
in_aifaddr_ioctl, if carp(4) is not loaded. This prevents NULL pointer
dereference and kernel panic.

Reviewed by:	imp@
Pull Request:	freebsd/freebsd-src#530
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Dec 27, 2021
@akhramov @bsdjhb
netinet: prevent NULL pointer dereference in in_aifaddr_ioctl()
ffc602d 
It appears that maliciously crafted ifaliasreq can lead to NULL
pointer dereference in in_aifaddr_ioctl(). In order to replicate
that, one needs to

1. Ensure that carp(4) is not loaded

2. Issue SIOCAIFADDR call setting ifra_vhid field of the request
   to a negative value.

A repro code would look like this.

int main() {
    struct ifaliasreq req;
    struct sockaddr_in sin, mask;
    int fd, error;

    bzero(&sin, sizeof(struct sockaddr_in));
    bzero(&mask, sizeof(struct sockaddr_in));

    sin.sin_len = sizeof(struct sockaddr_in);
    sin.sin_family = AF_INET;
    sin.sin_addr.s_addr = inet_addr("192.168.88.2");

    mask.sin_len = sizeof(struct sockaddr_in);
    mask.sin_family = AF_INET;
    mask.sin_addr.s_addr = inet_addr("255.255.255.0");

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if (fd < 0)
        return (-1);

    memset(&req, 0, sizeof(struct ifaliasreq));
    strlcpy(req.ifra_name, "lo0", sizeof(req.ifra_name));
    memcpy(&req.ifra_addr, &sin, sin.sin_len);
    memcpy(&req.ifra_mask, &mask, mask.sin_len);
    req.ifra_vhid = -1;

    return ioctl(fd, SIOCAIFADDR, (char *)&req);
}

To fix, discard both positive and negative vhid values in
in_aifaddr_ioctl, if carp(4) is not loaded. This prevents NULL pointer
dereference and kernel panic.

Reviewed by:	imp@
Pull Request:	freebsd/freebsd-src#530
hardenedbsd-services pushed a commit to HardenedBSD/hardenedBSD that referenced this pull request Feb 10, 2023
@mjp41 @davidchisnall @nwf-msr
Refactor interface between backend and frontend (freebsd#530)
03c9da6 
* Rename to use Config, rather than StateHandle/Globals/Backend
* Make Backend a type on Config that contains the address space management implementation
* Make Ranges part of the Backend configuration, so we can reuse code for different ways of managing memory
* Pull the common chains of range definitions into separate files for reuse.
* Move PagemapEntry to CommonConfig
* Expose Pagemap through backend, so frontend doesn't see Pagemap directly
* Remove global Pal and use DefaultPal, where one is not pass explicitly.

Co-authored-by: David Chisnall <davidchisnall@users.noreply.github.com>
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>
@emaste emaste added the merged label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsdimp bsdimp Awaiting requested review from bsdimp

Assignees
No one assigned
Labels
merged
Projects
None yet
Milestone
No milestone
3 participants
@akhramov @bsdimp @emaste