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

sys/openbsd: avoid using BIOCSETIF in causes "tun: read failed" #5016

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sys/openbsd: avoid using BIOCSETIF in causes "tun: read failed" #5016

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions sys/openbsd/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
func InitTarget(target *prog.Target) {
arch := &arch{
unix: targets.MakeUnixNeutralizer(target),
BIOCSETIF: target.GetConst("BIOCSETIF"),
CLOCK_REALTIME: target.GetConst("CLOCK_REALTIME"),
CTL_KERN: target.GetConst("CTL_KERN"),
DIOCCLRSTATES: target.GetConst("DIOCCLRSTATES"),
Expand All @@ -37,6 +38,7 @@ func InitTarget(target *prog.Target) {

type arch struct {
unix *targets.UnixNeutralizer
BIOCSETIF uint64
CLOCK_REALTIME uint64
CTL_KERN uint64
DIOCCLRSTATES uint64
Expand Down Expand Up @@ -113,6 +115,14 @@ func (arch *arch) neutralize(c *prog.Call, fixStructure bool) error {
if request.Val == arch.DIOCCLRSTATES || request.Val == arch.DIOCKILLSTATES {
request.Val = 0
}
// BIOCSETIF on tap leads to "tun: read failed"
if request.Val == arch.BIOCSETIF {
// Ideally this should also check Args[2] against "tap" as
// we only want to prevent the following from happening:
// ioctl$BIOCSETIF(-1, 0x8020426c, &(0x...)={'tap', 0x0})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dvyukov could you recommend the incantation to tighten this check?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you interested in how exactly you could check for the tap string in the third argument or something more general, e.g. if there are other ways to restrict it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just want this ioctl to not happen to tap interfaces because we rely on them working for packet injection.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK there's currently to code that does something similar enough to this, but we could try to construct it from pieces.

  1. Cast c.Args[2] to *prog.PointerArg. Let it be ptrArg.
  2. Cast ptrArg.Res to *prog.GroupArg. Let it be groupArg.
  3. Cast groupArg.Inner[0] to *prog.DataArg. Let it be dataArg.
  4. Verify dataArg.Data() and patch it, if necessary. Like here:

syzkaller/sys/linux/init.go

Lines 241 to 253 in 32fcf98

dataArg, ok := unionArg.Option.(*prog.DataArg)
if !ok {
return
}
if dataArg.Dir() == prog.DirOut {
return
}
// Clear the first 16 bytes to prevent overcoming the limitation by squashing the struct.
data := append([]byte{}, dataArg.Data()...)
for i := 0; i < 16 && i < len(data); i++ {
data[i] = 0
}
dataArg.SetData(data)

One this to watch out for are ANY types -- in that case, syzkaller has squahed the whole struct into one binary blob. We cannot really build a good tight sanity check in that case, so I think we can check for ANY and boldly patch it like you've already done it in this PR.

Here's an example of a check for ANY. You can do it for c.Args[2].

syzkaller/sys/linux/init_vusb.go

Lines 61 to 63 in 32fcf98

if g.Target().ArgContainsAny(arg) {
return
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your guidance.

I implemented 1-4 but I'm not sure where to get a *prog.Gen to call .Target on.

Also not sure if there's an easy way to test this without pushing it to prod :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also not sure if there's an easy way to test this without pushing it to prod :)

We do have quite a lot of tests for call sanitization in Linux: https://github.com/google/syzkaller/blob/master/sys/linux/init_test.go

I'm not sure where to get a *prog.Gen to call .Target on.

At the very least we could remember it in the struct arch {} -- the prog.Target pointer is passed to InitTarget.

request.Val = 0
}

case "mknodat":
argStart = 2
fallthrough
Expand Down
Loading