From 8cc1c35bbdc5c9bd6b3965311497885ce72317a8 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Thu, 12 Dec 2019 15:48:24 -0800 Subject: Write simple ACCEPT rules to the filter table. This gets us closer to passing the iptables tests and opens up iptables so it can be worked on by multiple people. A few restrictions are enforced for security (i.e. we don't want to let users write a bunch of iptables rules and then just not enforce them): - Only the filter table is writable. - Only ACCEPT rules with no matching criteria can be added. --- pkg/sentry/socket/netfilter/BUILD | 1 + pkg/sentry/socket/netfilter/netfilter.go | 411 ++++++++++++++++++++++++------- pkg/sentry/socket/netstack/netstack.go | 23 +- 3 files changed, 350 insertions(+), 85 deletions(-) (limited to 'pkg/sentry') diff --git a/pkg/sentry/socket/netfilter/BUILD b/pkg/sentry/socket/netfilter/BUILD index 5eb06bbf4..b70047d81 100644 --- a/pkg/sentry/socket/netfilter/BUILD +++ b/pkg/sentry/socket/netfilter/BUILD @@ -14,6 +14,7 @@ go_library( deps = [ "//pkg/abi/linux", "//pkg/binary", + "//pkg/log", "//pkg/sentry/kernel", "//pkg/sentry/usermem", "//pkg/syserr", diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go index 9f87c32f1..8c7f3c7fc 100644 --- a/pkg/sentry/socket/netfilter/netfilter.go +++ b/pkg/sentry/socket/netfilter/netfilter.go @@ -21,6 +21,7 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/binary" + "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/usermem" "gvisor.dev/gvisor/pkg/syserr" @@ -35,6 +36,7 @@ const errorTargetName = "ERROR" // metadata is opaque to netstack. It holds data that we need to translate // between Linux's and netstack's iptables representations. +// TODO(gvisor.dev/issue/170): This might be removable. type metadata struct { HookEntry [linux.NF_INET_NUMHOOKS]uint32 Underflow [linux.NF_INET_NUMHOOKS]uint32 @@ -51,7 +53,7 @@ func GetInfo(t *kernel.Task, ep tcpip.Endpoint, outPtr usermem.Addr) (linux.IPTG } // Find the appropriate table. - table, err := findTable(ep, info.TableName()) + table, err := findTable(ep, info.Name.String()) if err != nil { return linux.IPTGetinfo{}, err } @@ -82,18 +84,19 @@ func GetEntries(t *kernel.Task, ep tcpip.Endpoint, outPtr usermem.Addr, outLen i } // Find the appropriate table. - table, err := findTable(ep, userEntries.TableName()) + table, err := findTable(ep, userEntries.Name.String()) if err != nil { return linux.KernelIPTGetEntries{}, err } // Convert netstack's iptables rules to something that the iptables // tool can understand. - entries, _, err := convertNetstackToBinary(userEntries.TableName(), table) + entries, _, err := convertNetstackToBinary(userEntries.Name.String(), table) if err != nil { return linux.KernelIPTGetEntries{}, err } if binary.Size(entries) > uintptr(outLen) { + log.Infof("Insufficient GetEntries output size: %d", uintptr(outLen)) return linux.KernelIPTGetEntries{}, syserr.ErrInvalidArgument } @@ -142,103 +145,63 @@ func convertNetstackToBinary(name string, table iptables.Table) (linux.KernelIPT // The table name has to fit in the struct. if linux.XT_TABLE_MAXNAMELEN < len(name) { + log.Infof("Table name too long.") return linux.KernelIPTGetEntries{}, metadata{}, syserr.ErrInvalidArgument } copy(entries.Name[:], name) - // Deal with the built in chains first (INPUT, OUTPUT, etc.). Each of - // these chains ends with an unconditional policy entry. - for hook := iptables.Prerouting; hook < iptables.NumHooks; hook++ { - chain, ok := table.BuiltinChains[hook] - if !ok { - // This table doesn't support this hook. - continue - } - - // Sanity check. - if len(chain.Rules) < 1 { - return linux.KernelIPTGetEntries{}, metadata{}, syserr.ErrInvalidArgument - } - - for ruleIdx, rule := range chain.Rules { - // If this is the first rule of a builtin chain, set - // the metadata hook entry point. - if ruleIdx == 0 { + for ruleIdx, rule := range table.Rules { + // Is this a chain entry point? + for hook, hookRuleIdx := range table.BuiltinChains { + if hookRuleIdx == ruleIdx { meta.HookEntry[hook] = entries.Size } - - // Each rule corresponds to an entry. - entry := linux.KernelIPTEntry{ - IPTEntry: linux.IPTEntry{ - NextOffset: linux.SizeOfIPTEntry, - TargetOffset: linux.SizeOfIPTEntry, - }, + } + // Is this a chain underflow point? The underflow rule is the last rule + // in the chain, and is an unconditional rule (i.e. it matches any + // packet). This is enforced when saving iptables. + for underflow, underflowRuleIdx := range table.Underflows { + if underflowRuleIdx == ruleIdx { + meta.Underflow[underflow] = entries.Size } + } - for _, matcher := range rule.Matchers { - // Serialize the matcher and add it to the - // entry. - serialized := marshalMatcher(matcher) - entry.Elems = append(entry.Elems, serialized...) - entry.NextOffset += uint16(len(serialized)) - entry.TargetOffset += uint16(len(serialized)) - } + // Each rule corresponds to an entry. + entry := linux.KernelIPTEntry{ + IPTEntry: linux.IPTEntry{ + NextOffset: linux.SizeOfIPTEntry, + TargetOffset: linux.SizeOfIPTEntry, + }, + } - // Serialize and append the target. - serialized := marshalTarget(rule.Target) + for _, matcher := range rule.Matchers { + // Serialize the matcher and add it to the + // entry. + serialized := marshalMatcher(matcher) entry.Elems = append(entry.Elems, serialized...) entry.NextOffset += uint16(len(serialized)) - - // The underflow rule is the last rule in the chain, - // and is an unconditional rule (i.e. it matches any - // packet). This is enforced when saving iptables. - if ruleIdx == len(chain.Rules)-1 { - meta.Underflow[hook] = entries.Size - } - - entries.Size += uint32(entry.NextOffset) - entries.Entrytable = append(entries.Entrytable, entry) - meta.NumEntries++ + entry.TargetOffset += uint16(len(serialized)) } - } + // Serialize and append the target. + serialized := marshalTarget(rule.Target) + entry.Elems = append(entry.Elems, serialized...) + entry.NextOffset += uint16(len(serialized)) - // TODO(gvisor.dev/issue/170): Deal with the user chains here. Each of - // these starts with an error node holding the chain's name and ends - // with an unconditional return. - - // Lastly, each table ends with an unconditional error target rule as - // its final entry. - errorEntry := linux.KernelIPTEntry{ - IPTEntry: linux.IPTEntry{ - NextOffset: linux.SizeOfIPTEntry, - TargetOffset: linux.SizeOfIPTEntry, - }, + entries.Size += uint32(entry.NextOffset) + entries.Entrytable = append(entries.Entrytable, entry) + meta.NumEntries++ } - var errorTarget linux.XTErrorTarget - errorTarget.Target.TargetSize = linux.SizeOfXTErrorTarget - copy(errorTarget.ErrorName[:], errorTargetName) - copy(errorTarget.Target.Name[:], errorTargetName) - - // Serialize and add it to the list of entries. - errorTargetBuf := make([]byte, 0, linux.SizeOfXTErrorTarget) - serializedErrorTarget := binary.Marshal(errorTargetBuf, usermem.ByteOrder, errorTarget) - errorEntry.Elems = append(errorEntry.Elems, serializedErrorTarget...) - errorEntry.NextOffset += uint16(len(serializedErrorTarget)) - - entries.Size += uint32(errorEntry.NextOffset) - entries.Entrytable = append(entries.Entrytable, errorEntry) - meta.NumEntries++ - meta.Size = entries.Size + meta.Size = entries.Size return entries, meta, nil } func marshalMatcher(matcher iptables.Matcher) []byte { switch matcher.(type) { default: - // TODO(gvisor.dev/issue/170): We don't support any matchers yet, so - // any call to marshalMatcher will panic. + // TODO(gvisor.dev/issue/170): We don't support any matchers + // yet, so any call to marshalMatcher will panic. panic(fmt.Errorf("unknown matcher of type %T", matcher)) } } @@ -246,28 +209,46 @@ func marshalMatcher(matcher iptables.Matcher) []byte { func marshalTarget(target iptables.Target) []byte { switch target.(type) { case iptables.UnconditionalAcceptTarget: - return marshalUnconditionalAcceptTarget() + return marshalStandardTarget(iptables.Accept) + case iptables.UnconditionalDropTarget: + return marshalStandardTarget(iptables.Drop) + case iptables.PanicTarget: + return marshalPanicTarget() default: panic(fmt.Errorf("unknown target of type %T", target)) } } -func marshalUnconditionalAcceptTarget() []byte { +func marshalStandardTarget(verdict iptables.Verdict) []byte { // The target's name will be the empty string. target := linux.XTStandardTarget{ Target: linux.XTEntryTarget{ TargetSize: linux.SizeOfXTStandardTarget, }, - Verdict: translateStandardVerdict(iptables.Accept), + Verdict: translateFromStandardVerdict(verdict), } ret := make([]byte, 0, linux.SizeOfXTStandardTarget) return binary.Marshal(ret, usermem.ByteOrder, target) } -// translateStandardVerdict translates verdicts the same way as the iptables +func marshalPanicTarget() []byte { + // This is an error target named error + target := linux.XTErrorTarget{ + Target: linux.XTEntryTarget{ + TargetSize: linux.SizeOfXTErrorTarget, + }, + } + copy(target.Name[:], errorTargetName) + copy(target.Target.Name[:], errorTargetName) + + ret := make([]byte, 0, linux.SizeOfXTErrorTarget) + return binary.Marshal(ret, usermem.ByteOrder, target) +} + +// translateFromStandardVerdict translates verdicts the same way as the iptables // tool. -func translateStandardVerdict(verdict iptables.Verdict) int32 { +func translateFromStandardVerdict(verdict iptables.Verdict) int32 { switch verdict { case iptables.Accept: return -linux.NF_ACCEPT - 1 @@ -280,7 +261,269 @@ func translateStandardVerdict(verdict iptables.Verdict) int32 { case iptables.Jump: // TODO(gvisor.dev/issue/170): Support Jump. panic("Jump isn't supported yet") + } + panic(fmt.Sprintf("unknown standard verdict: %d", verdict)) +} + +// translateToStandardVerdict translates from the value in a +// linux.XTStandardTarget to an iptables.Verdict. +func translateToStandardVerdict(val int32) (iptables.Verdict, *syserr.Error) { + // TODO(gvisor.dev/issue/170): Support other verdicts. + switch val { + case -linux.NF_ACCEPT - 1: + return iptables.Accept, nil + case -linux.NF_DROP - 1: + return iptables.Drop, nil + case -linux.NF_QUEUE - 1: + log.Infof("Unsupported iptables verdict QUEUE.") + case linux.NF_RETURN: + log.Infof("Unsupported iptables verdict RETURN.") + } + log.Infof("Unknown iptables verdict %d.", val) + return iptables.Invalid, syserr.ErrInvalidArgument +} + +// SetEntries sets iptables rules for a single table. See +// net/ipv4/netfilter/ip_tables.c:translate_table for reference. +func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { + printReplace(optVal) + + // Get the basic rules data (struct ipt_replace). + if len(optVal) < linux.SizeOfIPTReplace { + return syserr.ErrInvalidArgument + } + var replace linux.IPTReplace + replaceBuf := optVal[:linux.SizeOfIPTReplace] + optVal = optVal[linux.SizeOfIPTReplace:] + binary.Unmarshal(replaceBuf, usermem.ByteOrder, &replace) + + // TODO(gvisor.dev/issue/170): Support other tables. + var table iptables.Table + switch replace.Name.String() { + case iptables.TablenameFilter: + table = iptables.EmptyFilterTable() default: - panic(fmt.Sprintf("unknown standard verdict: %d", verdict)) + log.Infof(fmt.Sprintf("We don't yet support writing to the %q table (gvisor.dev/issue/170)", replace.Name.String())) + return syserr.ErrInvalidArgument + } + + // Convert input into a list of rules and their offsets. + var offset uint32 + var offsets []uint32 + for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ { + // Get the struct ipt_entry. + if len(optVal) < linux.SizeOfIPTEntry { + return syserr.ErrInvalidArgument + } + var entry linux.IPTEntry + buf := optVal[:linux.SizeOfIPTEntry] + optVal = optVal[linux.SizeOfIPTEntry:] + binary.Unmarshal(buf, usermem.ByteOrder, &entry) + if entry.TargetOffset != linux.SizeOfIPTEntry { + // TODO(gvisor.dev/issue/170): Support matchers. + return syserr.ErrInvalidArgument + } + + // TODO(gvisor.dev/issue/170): We should support IPTIP + // filtering. We reject any nonzero IPTIP values for now. + emptyIPTIP := linux.IPTIP{} + if entry.IP != emptyIPTIP { + return syserr.ErrInvalidArgument + } + + // Get the target of the rule. + target, consumed, err := parseTarget(optVal) + if err != nil { + return err + } + optVal = optVal[consumed:] + + table.Rules = append(table.Rules, iptables.Rule{Target: target}) + offsets = append(offsets, offset) + offset += linux.SizeOfIPTEntry + consumed + } + + // Go through the list of supported hooks for this table and, for each + // one, set the rule it corresponds to. + for hook, _ := range replace.HookEntry { + if table.ValidHooks()&uint32(hook) != 0 { + hk := hookFromLinux(hook) + for ruleIdx, offset := range offsets { + if offset == replace.HookEntry[hook] { + table.BuiltinChains[hk] = ruleIdx + } + if offset == replace.Underflow[hook] { + table.Underflows[hk] = ruleIdx + } + } + if ruleIdx := table.BuiltinChains[hk]; ruleIdx == iptables.HookUnset { + log.Infof("Hook %v is unset.", hk) + return syserr.ErrInvalidArgument + } + if ruleIdx := table.Underflows[hk]; ruleIdx == iptables.HookUnset { + log.Infof("Underflow %v is unset.", hk) + return syserr.ErrInvalidArgument + } + } + } + + ipt := stack.IPTables() + table.SetMetadata(metadata{ + HookEntry: replace.HookEntry, + Underflow: replace.Underflow, + NumEntries: replace.NumEntries, + Size: replace.Size, + }) + ipt.Tables[replace.Name.String()] = table + // TODO: Do we need to worry about locking? We could write rules while + // packets traverse tables. + stack.SetIPTables(ipt) + + return nil +} + +// parseTarget parses a target from the start of optVal and returns the target +// along with the number of bytes it occupies in optVal. +func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { + if len(optVal) < linux.SizeOfXTEntryTarget { + return nil, 0, syserr.ErrInvalidArgument + } + var target linux.XTEntryTarget + buf := optVal[:linux.SizeOfXTEntryTarget] + binary.Unmarshal(buf, usermem.ByteOrder, &target) + switch target.Name.String() { + case "": + // Standard target. + if len(optVal) < linux.SizeOfXTStandardTarget { + return nil, 0, syserr.ErrInvalidArgument + } + var target linux.XTStandardTarget + buf = optVal[:linux.SizeOfXTStandardTarget] + binary.Unmarshal(buf, usermem.ByteOrder, &target) + + verdict, err := translateToStandardVerdict(target.Verdict) + if err != nil { + return nil, 0, err + } + switch verdict { + case iptables.Accept: + return iptables.UnconditionalAcceptTarget{}, linux.SizeOfXTStandardTarget, nil + case iptables.Drop: + // TODO(gvisor.dev/issue/170): Return an + // iptables.UnconditionalDropTarget to support DROP. + log.Infof("netfilter DROP is not supported yet.") + return nil, 0, syserr.ErrInvalidArgument + default: + panic(fmt.Sprintf("Unknown verdict: %v", verdict)) + } + + case errorTargetName: + // Error target. + if len(optVal) < linux.SizeOfXTErrorTarget { + return nil, 0, syserr.ErrInvalidArgument + } + var target linux.XTErrorTarget + buf = optVal[:linux.SizeOfXTErrorTarget] + binary.Unmarshal(buf, usermem.ByteOrder, &target) + + // Error targets are used in 2 cases: + // * An actual error case. These rules have an error + // named errorTargetName. The last entry of the table + // is usually an error case to catch any packets that + // somehow fall through every rule. + // * To mark the start of a user defined chain. These + // rules have an error with the name of the chain. + switch target.Name.String() { + case errorTargetName: + return iptables.PanicTarget{}, linux.SizeOfXTErrorTarget, nil + default: + log.Infof("Unknown error target %q doesn't exist or isn't supported yet.", target.Name.String()) + return nil, 0, syserr.ErrInvalidArgument + } + } + + // Unknown target. + log.Infof("Unknown target %q doesn't exist or isn't supported yet.", target.Name.String()) + return nil, 0, syserr.ErrInvalidArgument +} + +func chainNameFromHook(hook int) string { + switch hook { + case linux.NF_INET_PRE_ROUTING: + return iptables.ChainNamePrerouting + case linux.NF_INET_LOCAL_IN: + return iptables.ChainNameInput + case linux.NF_INET_FORWARD: + return iptables.ChainNameForward + case linux.NF_INET_LOCAL_OUT: + return iptables.ChainNameOutput + case linux.NF_INET_POST_ROUTING: + return iptables.ChainNamePostrouting + } + panic(fmt.Sprintf("Unknown hook %d does not correspond to a builtin chain")) +} + +func hookFromLinux(hook int) iptables.Hook { + switch hook { + case linux.NF_INET_PRE_ROUTING: + return iptables.Prerouting + case linux.NF_INET_LOCAL_IN: + return iptables.Input + case linux.NF_INET_FORWARD: + return iptables.Forward + case linux.NF_INET_LOCAL_OUT: + return iptables.Output + case linux.NF_INET_POST_ROUTING: + return iptables.Postrouting + } + panic(fmt.Sprintf("Unknown hook %d does not correspond to a builtin chain")) +} + +// printReplace prints information about the struct ipt_replace in optVal. It +// is only for debugging. +func printReplace(optVal []byte) { + // Basic replace info. + var replace linux.IPTReplace + replaceBuf := optVal[:linux.SizeOfIPTReplace] + optVal = optVal[linux.SizeOfIPTReplace:] + binary.Unmarshal(replaceBuf, usermem.ByteOrder, &replace) + log.Infof("kevin: Replacing table %q: %+v", replace.Name.String(), replace) + + // Read in the list of entries at the end of replace. + var totalOffset uint16 + for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ { + var entry linux.IPTEntry + entryBuf := optVal[:linux.SizeOfIPTEntry] + binary.Unmarshal(entryBuf, usermem.ByteOrder, &entry) + log.Infof("kevin: Entry %d (total offset %d): %+v", entryIdx, totalOffset, entry) + + totalOffset += entry.NextOffset + if entry.TargetOffset == linux.SizeOfIPTEntry { + log.Infof("kevin: Entry has no matches.") + } else { + log.Infof("kevin: Entry has matches.") + } + + var target linux.XTEntryTarget + targetBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTEntryTarget] + binary.Unmarshal(targetBuf, usermem.ByteOrder, &target) + log.Infof("kevin: Target named %q: %+v", target.Name.String(), target) + + switch target.Name.String() { + case "": + var standardTarget linux.XTStandardTarget + stBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTStandardTarget] + binary.Unmarshal(stBuf, usermem.ByteOrder, &standardTarget) + log.Infof("kevin: Standard target with verdict %q (%d).", linux.VerdictStrings[standardTarget.Verdict], standardTarget.Verdict) + case errorTargetName: + var errorTarget linux.XTErrorTarget + etBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTErrorTarget] + binary.Unmarshal(etBuf, usermem.ByteOrder, &errorTarget) + log.Infof("kevin: Error target with name %q.", errorTarget.Name.String()) + default: + log.Infof("kevin: Unknown target type.") + } + + optVal = optVal[entry.NextOffset:] } } diff --git a/pkg/sentry/socket/netstack/netstack.go b/pkg/sentry/socket/netstack/netstack.go index 140851c17..f7caa45b4 100644 --- a/pkg/sentry/socket/netstack/netstack.go +++ b/pkg/sentry/socket/netstack/netstack.go @@ -326,7 +326,7 @@ func AddressAndFamily(sfamily int, addr []byte, strict bool) (tcpip.FullAddress, } family := usermem.ByteOrder.Uint16(addr) - if family != uint16(sfamily) && (strict || family != linux.AF_UNSPEC) { + if family != uint16(sfamily) && (!strict && family != linux.AF_UNSPEC) { return tcpip.FullAddress{}, family, syserr.ErrAddressFamilyNotSupported } @@ -1356,6 +1356,27 @@ func (s *SocketOperations) SetSockOpt(t *kernel.Task, level int, name int, optVa return nil } + if s.skType == linux.SOCK_RAW && level == linux.IPPROTO_IP { + if name == linux.IPT_SO_SET_REPLACE { + if len(optVal) < linux.SizeOfIPTReplace { + return syserr.ErrInvalidArgument + } + + stack := inet.StackFromContext(t) + if stack == nil { + return syserr.ErrNoDevice + } + // Stack must be a netstack stack. + if err := netfilter.SetEntries(stack.(*Stack).Stack, optVal); err != nil { + return err + } + return nil + } else if name == linux.IPT_SO_SET_ADD_COUNTERS { + // TODO(gvisor.dev/issue/170): Counter support. + return nil + } + } + return SetSockOpt(t, s, s.Endpoint, level, name, optVal) } -- cgit v1.2.3 From 1e1921e2acdb7357972257219fdffb9edf17bf55 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 8 Jan 2020 11:15:46 -0800 Subject: Minor fixes to comments and logging --- pkg/sentry/socket/netfilter/netfilter.go | 10 +++++++--- pkg/sentry/socket/netstack/netstack.go | 8 +++++--- pkg/sentry/syscalls/linux/sys_socket.go | 2 +- pkg/tcpip/iptables/targets.go | 2 +- pkg/tcpip/iptables/types.go | 28 ++++++---------------------- test/iptables/filter_input.go | 3 +-- test/iptables/iptables_test.go | 22 +++++++++++----------- 7 files changed, 32 insertions(+), 43 deletions(-) (limited to 'pkg/sentry') diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go index 8c7f3c7fc..b7867a576 100644 --- a/pkg/sentry/socket/netfilter/netfilter.go +++ b/pkg/sentry/socket/netfilter/netfilter.go @@ -157,9 +157,7 @@ func convertNetstackToBinary(name string, table iptables.Table) (linux.KernelIPT meta.HookEntry[hook] = entries.Size } } - // Is this a chain underflow point? The underflow rule is the last rule - // in the chain, and is an unconditional rule (i.e. it matches any - // packet). This is enforced when saving iptables. + // Is this a chain underflow point? for underflow, underflowRuleIdx := range table.Underflows { if underflowRuleIdx == ruleIdx { meta.Underflow[underflow] = entries.Size @@ -290,6 +288,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { // Get the basic rules data (struct ipt_replace). if len(optVal) < linux.SizeOfIPTReplace { + log.Infof("netfilter.SetEntries: optVal has insufficient size for replace %d", len(optVal)) return syserr.ErrInvalidArgument } var replace linux.IPTReplace @@ -313,6 +312,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ { // Get the struct ipt_entry. if len(optVal) < linux.SizeOfIPTEntry { + log.Infof("netfilter: optVal has insufficient size for entry %d", len(optVal)) return syserr.ErrInvalidArgument } var entry linux.IPTEntry @@ -328,6 +328,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { // filtering. We reject any nonzero IPTIP values for now. emptyIPTIP := linux.IPTIP{} if entry.IP != emptyIPTIP { + log.Infof("netfilter: non-empty struct iptip found") return syserr.ErrInvalidArgument } @@ -386,6 +387,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { // along with the number of bytes it occupies in optVal. func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { if len(optVal) < linux.SizeOfXTEntryTarget { + log.Infof("netfilter: optVal has insufficient size for entry target %d", len(optVal)) return nil, 0, syserr.ErrInvalidArgument } var target linux.XTEntryTarget @@ -395,6 +397,7 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { case "": // Standard target. if len(optVal) < linux.SizeOfXTStandardTarget { + log.Infof("netfilter.SetEntries: optVal has insufficient size for standard target %d", len(optVal)) return nil, 0, syserr.ErrInvalidArgument } var target linux.XTStandardTarget @@ -420,6 +423,7 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { case errorTargetName: // Error target. if len(optVal) < linux.SizeOfXTErrorTarget { + log.Infof("netfilter.SetEntries: optVal has insufficient size for error target %d", len(optVal)) return nil, 0, syserr.ErrInvalidArgument } var target linux.XTErrorTarget diff --git a/pkg/sentry/socket/netstack/netstack.go b/pkg/sentry/socket/netstack/netstack.go index f7caa45b4..8c07eef4b 100644 --- a/pkg/sentry/socket/netstack/netstack.go +++ b/pkg/sentry/socket/netstack/netstack.go @@ -326,7 +326,7 @@ func AddressAndFamily(sfamily int, addr []byte, strict bool) (tcpip.FullAddress, } family := usermem.ByteOrder.Uint16(addr) - if family != uint16(sfamily) && (!strict && family != linux.AF_UNSPEC) { + if family != uint16(sfamily) && (strict || family != linux.AF_UNSPEC) { return tcpip.FullAddress{}, family, syserr.ErrAddressFamilyNotSupported } @@ -1357,7 +1357,8 @@ func (s *SocketOperations) SetSockOpt(t *kernel.Task, level int, name int, optVa } if s.skType == linux.SOCK_RAW && level == linux.IPPROTO_IP { - if name == linux.IPT_SO_SET_REPLACE { + switch name { + case linux.IPT_SO_SET_REPLACE: if len(optVal) < linux.SizeOfIPTReplace { return syserr.ErrInvalidArgument } @@ -1371,7 +1372,8 @@ func (s *SocketOperations) SetSockOpt(t *kernel.Task, level int, name int, optVa return err } return nil - } else if name == linux.IPT_SO_SET_ADD_COUNTERS { + + case linux.IPT_SO_SET_ADD_COUNTERS: // TODO(gvisor.dev/issue/170): Counter support. return nil } diff --git a/pkg/sentry/syscalls/linux/sys_socket.go b/pkg/sentry/syscalls/linux/sys_socket.go index 4b5aafcc0..cda517a81 100644 --- a/pkg/sentry/syscalls/linux/sys_socket.go +++ b/pkg/sentry/syscalls/linux/sys_socket.go @@ -41,7 +41,7 @@ const maxListenBacklog = 1024 const maxAddrLen = 200 // maxOptLen is the maximum sockopt parameter length we're willing to accept. -const maxOptLen = 1024 +const maxOptLen = 1024 * 8 // maxControlLen is the maximum length of the msghdr.msg_control buffer we're // willing to accept. Note that this limit is smaller than Linux, which allows diff --git a/pkg/tcpip/iptables/targets.go b/pkg/tcpip/iptables/targets.go index 03c9f19ff..2c3598e3d 100644 --- a/pkg/tcpip/iptables/targets.go +++ b/pkg/tcpip/iptables/targets.go @@ -34,7 +34,7 @@ func (UnconditionalDropTarget) Action(packet buffer.VectorisedView) (Verdict, st return Drop, "" } -// PanicTarget just panics. +// PanicTarget just panics. It represents a target that should be unreachable. type PanicTarget struct{} // Actions implements Target.Action. diff --git a/pkg/tcpip/iptables/types.go b/pkg/tcpip/iptables/types.go index 76364ff1f..fe0394a31 100644 --- a/pkg/tcpip/iptables/types.go +++ b/pkg/tcpip/iptables/types.go @@ -107,20 +107,19 @@ type IPTables struct { Priorities map[Hook][]string } -// A Table defines a set of chains and hooks into the network stack. The -// currently supported tables are: -// * nat -// * mangle +// A Table defines a set of chains and hooks into the network stack. It is +// really just a list of rules with some metadata for entrypoints and such. type Table struct { - // A table is just a list of rules with some entrypoints. + // Rules holds the rules that make up the table. Rules []Rule + // BuiltinChains maps builtin chains to their entrypoints. BuiltinChains map[Hook]int + // Underflows maps builtin chains to their underflow point (i.e. the + // rule to execute if the chain returns without a verdict). Underflows map[Hook]int - // DefaultTargets map[Hook]int - // UserChains holds user-defined chains for the keyed by name. Users // can give their chains arbitrary names. UserChains map[string]int @@ -149,21 +148,6 @@ func (table *Table) SetMetadata(metadata interface{}) { table.metadata = metadata } -//// A Chain defines a list of rules for packet processing. When a packet -//// traverses a chain, it is checked against each rule until either a rule -//// returns a verdict or the chain ends. -//// -//// By convention, builtin chains end with a rule that matches everything and -//// returns either Accept or Drop. User-defined chains end with Return. These -//// aren't strictly necessary here, but the iptables tool writes tables this way. -//type Chain struct { -// // Name is the chain name. -// Name string - -// // Rules is the list of rules to traverse. -// Rules []Rule -//} - // A Rule is a packet processing rule. It consists of two pieces. First it // contains zero or more matchers, each of which is a specification of which // packets this rule applies to. If there are no matchers in the rule, it diff --git a/test/iptables/filter_input.go b/test/iptables/filter_input.go index 0cb668635..34a85db97 100644 --- a/test/iptables/filter_input.go +++ b/test/iptables/filter_input.go @@ -43,8 +43,7 @@ func (FilterInputDropUDP) Name() string { // ContainerAction implements TestCase.ContainerAction. func (FilterInputDropUDP) ContainerAction(ip net.IP) error { - if err := filterTable("-A", "INPUT", "-p", "udp", "-j", "DROP"); err != nil { - // if err := filterTable("-A", "INPUT", "-j", "ACCEPT"); err != nil { + if err := filterTable("-A", "INPUT", "-j", "ACCEPT"); err != nil { return err } diff --git a/test/iptables/iptables_test.go b/test/iptables/iptables_test.go index e761e0f2f..2465a4e65 100644 --- a/test/iptables/iptables_test.go +++ b/test/iptables/iptables_test.go @@ -167,14 +167,14 @@ func TestFilterInputDropUDP(t *testing.T) { } } -// func TestFilterInputDropUDPPort(t *testing.T) { -// if err := singleTest(FilterInputDropUDPPort{}); err != nil { -// t.Fatal(err) -// } -// } - -// func TestFilterInputDropDifferentUDPPort(t *testing.T) { -// if err := singleTest(FilterInputDropDifferentUDPPort{}); err != nil { -// t.Fatal(err) -// } -// } +func TestFilterInputDropUDPPort(t *testing.T) { + if err := singleTest(FilterInputDropUDPPort{}); err != nil { + t.Fatal(err) + } +} + +func TestFilterInputDropDifferentUDPPort(t *testing.T) { + if err := singleTest(FilterInputDropDifferentUDPPort{}); err != nil { + t.Fatal(err) + } +} -- cgit v1.2.3 From 446a250996d9c946d9a5279f7fd081cc1be0bd11 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 8 Jan 2020 11:20:48 -0800 Subject: Comment cleanup. --- pkg/abi/linux/netfilter.go | 2 ++ pkg/sentry/socket/netfilter/netfilter.go | 2 -- pkg/tcpip/iptables/types.go | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) (limited to 'pkg/sentry') diff --git a/pkg/abi/linux/netfilter.go b/pkg/abi/linux/netfilter.go index 0bcb232de..35d66d622 100644 --- a/pkg/abi/linux/netfilter.go +++ b/pkg/abi/linux/netfilter.go @@ -42,6 +42,8 @@ const ( NF_RETURN = -NF_REPEAT - 1 ) +// VerdictStrings maps int verdicts to the strings they represent. It is used +// for debugging. var VerdictStrings = map[int32]string{ -NF_DROP - 1: "DROP", -NF_ACCEPT - 1: "ACCEPT", diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go index b7867a576..347342f98 100644 --- a/pkg/sentry/socket/netfilter/netfilter.go +++ b/pkg/sentry/socket/netfilter/netfilter.go @@ -376,8 +376,6 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { Size: replace.Size, }) ipt.Tables[replace.Name.String()] = table - // TODO: Do we need to worry about locking? We could write rules while - // packets traverse tables. stack.SetIPTables(ipt) return nil diff --git a/pkg/tcpip/iptables/types.go b/pkg/tcpip/iptables/types.go index fe0394a31..540f8c0b4 100644 --- a/pkg/tcpip/iptables/types.go +++ b/pkg/tcpip/iptables/types.go @@ -113,11 +113,11 @@ type Table struct { // Rules holds the rules that make up the table. Rules []Rule - // BuiltinChains maps builtin chains to their entrypoints. + // BuiltinChains maps builtin chains to their entrypoint rule in Rules. BuiltinChains map[Hook]int - // Underflows maps builtin chains to their underflow point (i.e. the - // rule to execute if the chain returns without a verdict). + // Underflows maps builtin chains to their underflow rule in Rules + // (i.e. the rule to execute if the chain returns without a verdict). Underflows map[Hook]int // UserChains holds user-defined chains for the keyed by name. Users -- cgit v1.2.3 From f26a576984052a235b63ec79081a8c4a8c8ffc00 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 8 Jan 2020 16:35:01 -0800 Subject: Addressed GH comments --- pkg/abi/linux/netfilter.go | 15 ++++-- pkg/sentry/socket/netfilter/netfilter.go | 87 +++++++++++++------------------- pkg/sentry/socket/netstack/netstack.go | 5 +- 3 files changed, 47 insertions(+), 60 deletions(-) (limited to 'pkg/sentry') diff --git a/pkg/abi/linux/netfilter.go b/pkg/abi/linux/netfilter.go index 35d66d622..c4f4ea0b1 100644 --- a/pkg/abi/linux/netfilter.go +++ b/pkg/abi/linux/netfilter.go @@ -298,6 +298,7 @@ type IPTReplace struct { // Entries [0]IPTEntry } +// KernelIPTEntry is identical to IPTReplace, but includes the Entries field. type KernelIPTReplace struct { IPTReplace Entries [0]IPTEntry @@ -306,28 +307,32 @@ type KernelIPTReplace struct { // SizeOfIPTReplace is the size of an IPTReplace. const SizeOfIPTReplace = 96 +// ExtensionName holds the name of a netfilter extension. type ExtensionName [XT_EXTENSION_MAXNAMELEN]byte // String implements fmt.Stringer. func (en ExtensionName) String() string { - return name(en[:]) + return goString(en[:]) } +// ExtensionName holds the name of a netfilter table. type TableName [XT_TABLE_MAXNAMELEN]byte // String implements fmt.Stringer. func (tn TableName) String() string { - return name(tn[:]) + return goString(tn[:]) } +// ExtensionName holds the name of a netfilter error. These can also hold +// user-defined chains. type ErrorName [XT_FUNCTION_MAXNAMELEN]byte // String implements fmt.Stringer. -func (fn ErrorName) String() string { - return name(fn[:]) +func (en ErrorName) String() string { + return goString(en[:]) } -func name(cstring []byte) string { +func goString(cstring []byte) string { for i, c := range cstring { if c == 0 { return string(cstring[:i]) diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go index 347342f98..799865b03 100644 --- a/pkg/sentry/socket/netfilter/netfilter.go +++ b/pkg/sentry/socket/netfilter/netfilter.go @@ -53,7 +53,7 @@ func GetInfo(t *kernel.Task, ep tcpip.Endpoint, outPtr usermem.Addr) (linux.IPTG } // Find the appropriate table. - table, err := findTable(ep, info.Name.String()) + table, err := findTable(ep, info.Name) if err != nil { return linux.IPTGetinfo{}, err } @@ -84,7 +84,7 @@ func GetEntries(t *kernel.Task, ep tcpip.Endpoint, outPtr usermem.Addr, outLen i } // Find the appropriate table. - table, err := findTable(ep, userEntries.Name.String()) + table, err := findTable(ep, userEntries.Name) if err != nil { return linux.KernelIPTGetEntries{}, err } @@ -96,19 +96,19 @@ func GetEntries(t *kernel.Task, ep tcpip.Endpoint, outPtr usermem.Addr, outLen i return linux.KernelIPTGetEntries{}, err } if binary.Size(entries) > uintptr(outLen) { - log.Infof("Insufficient GetEntries output size: %d", uintptr(outLen)) + log.Warningf("Insufficient GetEntries output size: %d", uintptr(outLen)) return linux.KernelIPTGetEntries{}, syserr.ErrInvalidArgument } return entries, nil } -func findTable(ep tcpip.Endpoint, tableName string) (iptables.Table, *syserr.Error) { +func findTable(ep tcpip.Endpoint, tablename linux.TableName) (iptables.Table, *syserr.Error) { ipt, err := ep.IPTables() if err != nil { return iptables.Table{}, syserr.FromError(err) } - table, ok := ipt.Tables[tableName] + table, ok := ipt.Tables[tablename.String()] if !ok { return iptables.Table{}, syserr.ErrInvalidArgument } @@ -138,17 +138,17 @@ func FillDefaultIPTables(stack *stack.Stack) { // format expected by the iptables tool. Linux stores each table as a binary // blob that can only be traversed by parsing a bit, reading some offsets, // jumping to those offsets, parsing again, etc. -func convertNetstackToBinary(name string, table iptables.Table) (linux.KernelIPTGetEntries, metadata, *syserr.Error) { +func convertNetstackToBinary(tablename string, table iptables.Table) (linux.KernelIPTGetEntries, metadata, *syserr.Error) { // Return values. var entries linux.KernelIPTGetEntries var meta metadata // The table name has to fit in the struct. - if linux.XT_TABLE_MAXNAMELEN < len(name) { - log.Infof("Table name too long.") + if linux.XT_TABLE_MAXNAMELEN < len(tablename) { + log.Warningf("Table name %q too long.", tablename) return linux.KernelIPTGetEntries{}, metadata{}, syserr.ErrInvalidArgument } - copy(entries.Name[:], name) + copy(entries.Name[:], tablename) for ruleIdx, rule := range table.Rules { // Is this a chain entry point? @@ -273,11 +273,12 @@ func translateToStandardVerdict(val int32) (iptables.Verdict, *syserr.Error) { case -linux.NF_DROP - 1: return iptables.Drop, nil case -linux.NF_QUEUE - 1: - log.Infof("Unsupported iptables verdict QUEUE.") + log.Warningf("Unsupported iptables verdict QUEUE.") case linux.NF_RETURN: - log.Infof("Unsupported iptables verdict RETURN.") + log.Warningf("Unsupported iptables verdict RETURN.") + default: + log.Warningf("Unknown iptables verdict %d.", val) } - log.Infof("Unknown iptables verdict %d.", val) return iptables.Invalid, syserr.ErrInvalidArgument } @@ -288,7 +289,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { // Get the basic rules data (struct ipt_replace). if len(optVal) < linux.SizeOfIPTReplace { - log.Infof("netfilter.SetEntries: optVal has insufficient size for replace %d", len(optVal)) + log.Warningf("netfilter.SetEntries: optVal has insufficient size for replace %d", len(optVal)) return syserr.ErrInvalidArgument } var replace linux.IPTReplace @@ -302,7 +303,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { case iptables.TablenameFilter: table = iptables.EmptyFilterTable() default: - log.Infof(fmt.Sprintf("We don't yet support writing to the %q table (gvisor.dev/issue/170)", replace.Name.String())) + log.Warningf("We don't yet support writing to the %q table (gvisor.dev/issue/170)", replace.Name.String()) return syserr.ErrInvalidArgument } @@ -312,7 +313,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { for entryIdx := uint32(0); entryIdx < replace.NumEntries; entryIdx++ { // Get the struct ipt_entry. if len(optVal) < linux.SizeOfIPTEntry { - log.Infof("netfilter: optVal has insufficient size for entry %d", len(optVal)) + log.Warningf("netfilter: optVal has insufficient size for entry %d", len(optVal)) return syserr.ErrInvalidArgument } var entry linux.IPTEntry @@ -328,7 +329,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { // filtering. We reject any nonzero IPTIP values for now. emptyIPTIP := linux.IPTIP{} if entry.IP != emptyIPTIP { - log.Infof("netfilter: non-empty struct iptip found") + log.Warningf("netfilter: non-empty struct iptip found") return syserr.ErrInvalidArgument } @@ -358,11 +359,11 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { } } if ruleIdx := table.BuiltinChains[hk]; ruleIdx == iptables.HookUnset { - log.Infof("Hook %v is unset.", hk) + log.Warningf("Hook %v is unset.", hk) return syserr.ErrInvalidArgument } if ruleIdx := table.Underflows[hk]; ruleIdx == iptables.HookUnset { - log.Infof("Underflow %v is unset.", hk) + log.Warningf("Underflow %v is unset.", hk) return syserr.ErrInvalidArgument } } @@ -385,7 +386,7 @@ func SetEntries(stack *stack.Stack, optVal []byte) *syserr.Error { // along with the number of bytes it occupies in optVal. func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { if len(optVal) < linux.SizeOfXTEntryTarget { - log.Infof("netfilter: optVal has insufficient size for entry target %d", len(optVal)) + log.Warningf("netfilter: optVal has insufficient size for entry target %d", len(optVal)) return nil, 0, syserr.ErrInvalidArgument } var target linux.XTEntryTarget @@ -395,14 +396,14 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { case "": // Standard target. if len(optVal) < linux.SizeOfXTStandardTarget { - log.Infof("netfilter.SetEntries: optVal has insufficient size for standard target %d", len(optVal)) + log.Warningf("netfilter.SetEntries: optVal has insufficient size for standard target %d", len(optVal)) return nil, 0, syserr.ErrInvalidArgument } - var target linux.XTStandardTarget + var standardTarget linux.XTStandardTarget buf = optVal[:linux.SizeOfXTStandardTarget] - binary.Unmarshal(buf, usermem.ByteOrder, &target) + binary.Unmarshal(buf, usermem.ByteOrder, &standardTarget) - verdict, err := translateToStandardVerdict(target.Verdict) + verdict, err := translateToStandardVerdict(standardTarget.Verdict) if err != nil { return nil, 0, err } @@ -424,9 +425,9 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { log.Infof("netfilter.SetEntries: optVal has insufficient size for error target %d", len(optVal)) return nil, 0, syserr.ErrInvalidArgument } - var target linux.XTErrorTarget + var errorTarget linux.XTErrorTarget buf = optVal[:linux.SizeOfXTErrorTarget] - binary.Unmarshal(buf, usermem.ByteOrder, &target) + binary.Unmarshal(buf, usermem.ByteOrder, &errorTarget) // Error targets are used in 2 cases: // * An actual error case. These rules have an error @@ -435,11 +436,11 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { // somehow fall through every rule. // * To mark the start of a user defined chain. These // rules have an error with the name of the chain. - switch target.Name.String() { + switch errorTarget.Name.String() { case errorTargetName: return iptables.PanicTarget{}, linux.SizeOfXTErrorTarget, nil default: - log.Infof("Unknown error target %q doesn't exist or isn't supported yet.", target.Name.String()) + log.Infof("Unknown error target %q doesn't exist or isn't supported yet.", errorTarget.Name.String()) return nil, 0, syserr.ErrInvalidArgument } } @@ -449,22 +450,6 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { return nil, 0, syserr.ErrInvalidArgument } -func chainNameFromHook(hook int) string { - switch hook { - case linux.NF_INET_PRE_ROUTING: - return iptables.ChainNamePrerouting - case linux.NF_INET_LOCAL_IN: - return iptables.ChainNameInput - case linux.NF_INET_FORWARD: - return iptables.ChainNameForward - case linux.NF_INET_LOCAL_OUT: - return iptables.ChainNameOutput - case linux.NF_INET_POST_ROUTING: - return iptables.ChainNamePostrouting - } - panic(fmt.Sprintf("Unknown hook %d does not correspond to a builtin chain")) -} - func hookFromLinux(hook int) iptables.Hook { switch hook { case linux.NF_INET_PRE_ROUTING: @@ -489,7 +474,7 @@ func printReplace(optVal []byte) { replaceBuf := optVal[:linux.SizeOfIPTReplace] optVal = optVal[linux.SizeOfIPTReplace:] binary.Unmarshal(replaceBuf, usermem.ByteOrder, &replace) - log.Infof("kevin: Replacing table %q: %+v", replace.Name.String(), replace) + log.Infof("Replacing table %q: %+v", replace.Name.String(), replace) // Read in the list of entries at the end of replace. var totalOffset uint16 @@ -497,33 +482,33 @@ func printReplace(optVal []byte) { var entry linux.IPTEntry entryBuf := optVal[:linux.SizeOfIPTEntry] binary.Unmarshal(entryBuf, usermem.ByteOrder, &entry) - log.Infof("kevin: Entry %d (total offset %d): %+v", entryIdx, totalOffset, entry) + log.Infof("Entry %d (total offset %d): %+v", entryIdx, totalOffset, entry) totalOffset += entry.NextOffset if entry.TargetOffset == linux.SizeOfIPTEntry { - log.Infof("kevin: Entry has no matches.") + log.Infof("Entry has no matches.") } else { - log.Infof("kevin: Entry has matches.") + log.Infof("Entry has matches.") } var target linux.XTEntryTarget targetBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTEntryTarget] binary.Unmarshal(targetBuf, usermem.ByteOrder, &target) - log.Infof("kevin: Target named %q: %+v", target.Name.String(), target) + log.Infof("Target named %q: %+v", target.Name.String(), target) switch target.Name.String() { case "": var standardTarget linux.XTStandardTarget stBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTStandardTarget] binary.Unmarshal(stBuf, usermem.ByteOrder, &standardTarget) - log.Infof("kevin: Standard target with verdict %q (%d).", linux.VerdictStrings[standardTarget.Verdict], standardTarget.Verdict) + log.Infof("Standard target with verdict %q (%d).", linux.VerdictStrings[standardTarget.Verdict], standardTarget.Verdict) case errorTargetName: var errorTarget linux.XTErrorTarget etBuf := optVal[entry.TargetOffset : entry.TargetOffset+linux.SizeOfXTErrorTarget] binary.Unmarshal(etBuf, usermem.ByteOrder, &errorTarget) - log.Infof("kevin: Error target with name %q.", errorTarget.Name.String()) + log.Infof("Error target with name %q.", errorTarget.Name.String()) default: - log.Infof("kevin: Unknown target type.") + log.Infof("Unknown target type.") } optVal = optVal[entry.NextOffset:] diff --git a/pkg/sentry/socket/netstack/netstack.go b/pkg/sentry/socket/netstack/netstack.go index 8c07eef4b..cd3dd1a53 100644 --- a/pkg/sentry/socket/netstack/netstack.go +++ b/pkg/sentry/socket/netstack/netstack.go @@ -1368,10 +1368,7 @@ func (s *SocketOperations) SetSockOpt(t *kernel.Task, level int, name int, optVa return syserr.ErrNoDevice } // Stack must be a netstack stack. - if err := netfilter.SetEntries(stack.(*Stack).Stack, optVal); err != nil { - return err - } - return nil + return netfilter.SetEntries(stack.(*Stack).Stack, optVal) case linux.IPT_SO_SET_ADD_COUNTERS: // TODO(gvisor.dev/issue/170): Counter support. -- cgit v1.2.3 From ae060a63d9ad1bfb65b84a2ccbaf2893c5a50b76 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 8 Jan 2020 17:30:08 -0800 Subject: More GH comments. --- pkg/abi/linux/netfilter.go | 6 +++--- pkg/sentry/socket/netfilter/netfilter.go | 8 ++++---- pkg/tcpip/iptables/BUILD | 5 ++++- pkg/tcpip/iptables/iptables.go | 6 +++--- pkg/tcpip/iptables/targets.go | 16 +++++++++++----- 5 files changed, 25 insertions(+), 16 deletions(-) (limited to 'pkg/sentry') diff --git a/pkg/abi/linux/netfilter.go b/pkg/abi/linux/netfilter.go index c4f4ea0b1..33fcc6c95 100644 --- a/pkg/abi/linux/netfilter.go +++ b/pkg/abi/linux/netfilter.go @@ -298,7 +298,7 @@ type IPTReplace struct { // Entries [0]IPTEntry } -// KernelIPTEntry is identical to IPTReplace, but includes the Entries field. +// KernelIPTReplace is identical to IPTReplace, but includes the Entries field. type KernelIPTReplace struct { IPTReplace Entries [0]IPTEntry @@ -315,7 +315,7 @@ func (en ExtensionName) String() string { return goString(en[:]) } -// ExtensionName holds the name of a netfilter table. +// TableName holds the name of a netfilter table. type TableName [XT_TABLE_MAXNAMELEN]byte // String implements fmt.Stringer. @@ -323,7 +323,7 @@ func (tn TableName) String() string { return goString(tn[:]) } -// ExtensionName holds the name of a netfilter error. These can also hold +// ErrorName holds the name of a netfilter error. These can also hold // user-defined chains. type ErrorName [XT_FUNCTION_MAXNAMELEN]byte diff --git a/pkg/sentry/socket/netfilter/netfilter.go b/pkg/sentry/socket/netfilter/netfilter.go index 799865b03..60bb30a9f 100644 --- a/pkg/sentry/socket/netfilter/netfilter.go +++ b/pkg/sentry/socket/netfilter/netfilter.go @@ -210,8 +210,8 @@ func marshalTarget(target iptables.Target) []byte { return marshalStandardTarget(iptables.Accept) case iptables.UnconditionalDropTarget: return marshalStandardTarget(iptables.Drop) - case iptables.PanicTarget: - return marshalPanicTarget() + case iptables.ErrorTarget: + return marshalErrorTarget() default: panic(fmt.Errorf("unknown target of type %T", target)) } @@ -230,7 +230,7 @@ func marshalStandardTarget(verdict iptables.Verdict) []byte { return binary.Marshal(ret, usermem.ByteOrder, target) } -func marshalPanicTarget() []byte { +func marshalErrorTarget() []byte { // This is an error target named error target := linux.XTErrorTarget{ Target: linux.XTEntryTarget{ @@ -438,7 +438,7 @@ func parseTarget(optVal []byte) (iptables.Target, uint32, *syserr.Error) { // rules have an error with the name of the chain. switch errorTarget.Name.String() { case errorTargetName: - return iptables.PanicTarget{}, linux.SizeOfXTErrorTarget, nil + return iptables.ErrorTarget{}, linux.SizeOfXTErrorTarget, nil default: log.Infof("Unknown error target %q doesn't exist or isn't supported yet.", errorTarget.Name.String()) return nil, 0, syserr.ErrInvalidArgument diff --git a/pkg/tcpip/iptables/BUILD b/pkg/tcpip/iptables/BUILD index cc5f531e2..64769c333 100644 --- a/pkg/tcpip/iptables/BUILD +++ b/pkg/tcpip/iptables/BUILD @@ -11,5 +11,8 @@ go_library( ], importpath = "gvisor.dev/gvisor/pkg/tcpip/iptables", visibility = ["//visibility:public"], - deps = ["//pkg/tcpip/buffer"], + deps = [ + "//pkg/log", + "//pkg/tcpip/buffer", + ], ) diff --git a/pkg/tcpip/iptables/iptables.go b/pkg/tcpip/iptables/iptables.go index 9e7005374..db0450a21 100644 --- a/pkg/tcpip/iptables/iptables.go +++ b/pkg/tcpip/iptables/iptables.go @@ -45,7 +45,7 @@ func DefaultTables() IPTables { Rule{Target: UnconditionalAcceptTarget{}}, Rule{Target: UnconditionalAcceptTarget{}}, Rule{Target: UnconditionalAcceptTarget{}}, - Rule{Target: PanicTarget{}}, + Rule{Target: ErrorTarget{}}, }, BuiltinChains: map[Hook]int{ Prerouting: 0, @@ -65,7 +65,7 @@ func DefaultTables() IPTables { Rules: []Rule{ Rule{Target: UnconditionalAcceptTarget{}}, Rule{Target: UnconditionalAcceptTarget{}}, - Rule{Target: PanicTarget{}}, + Rule{Target: ErrorTarget{}}, }, BuiltinChains: map[Hook]int{ Prerouting: 0, @@ -82,7 +82,7 @@ func DefaultTables() IPTables { Rule{Target: UnconditionalAcceptTarget{}}, Rule{Target: UnconditionalAcceptTarget{}}, Rule{Target: UnconditionalAcceptTarget{}}, - Rule{Target: PanicTarget{}}, + Rule{Target: ErrorTarget{}}, }, BuiltinChains: map[Hook]int{ Input: 0, diff --git a/pkg/tcpip/iptables/targets.go b/pkg/tcpip/iptables/targets.go index 2c3598e3d..d65ed8df5 100644 --- a/pkg/tcpip/iptables/targets.go +++ b/pkg/tcpip/iptables/targets.go @@ -16,7 +16,10 @@ package iptables -import "gvisor.dev/gvisor/pkg/tcpip/buffer" +import ( + "gvisor.dev/gvisor/pkg/log" + "gvisor.dev/gvisor/pkg/tcpip/buffer" +) // UnconditionalAcceptTarget accepts all packets. type UnconditionalAcceptTarget struct{} @@ -34,10 +37,13 @@ func (UnconditionalDropTarget) Action(packet buffer.VectorisedView) (Verdict, st return Drop, "" } -// PanicTarget just panics. It represents a target that should be unreachable. -type PanicTarget struct{} +// ErrorTarget logs an error and drops the packet. It represents a target that +// should be unreachable. +type ErrorTarget struct{} // Actions implements Target.Action. -func (PanicTarget) Action(packet buffer.VectorisedView) (Verdict, string) { - panic("PanicTarget triggered.") +func (ErrorTarget) Action(packet buffer.VectorisedView) (Verdict, string) { + log.Warningf("ErrorTarget triggered.") + return Drop, "" + } -- cgit v1.2.3