Merge pull request #2678 from nybidari:iptables

PiperOrigin-RevId: 311203776

3 files changed, 230 insertions, 40 deletions

diff --git a/pkg/sentry/socket/netfilter/owner_matcher.go b/pkg/sentry/socket/netfilter/owner_matcher.go
index 5949a7c29..3863293c7 100644
--- a/pkg/sentry/socket/netfilter/owner_matcher.go
+++ b/pkg/sentry/socket/netfilter/owner_matcher.go
@@ -45,14 +45,18 @@ func (ownerMarshaler) marshal(mr stack.Matcher) []byte {
 		GID: matcher.gid,
 	}
 
-	// Support for UID match.
-	// TODO(gvisor.dev/issue/170): Need to support gid match.
+	// Support for UID and GID match.
 	if matcher.matchUID {
 		iptOwnerInfo.Match = linux.XT_OWNER_UID
-	} else if matcher.matchGID {
-		panic("GID match is not supported.")
-	} else {
-		panic("UID match is not set.")
+		if matcher.invertUID {
+			iptOwnerInfo.Invert = linux.XT_OWNER_UID
+		}
+	}
+	if matcher.matchGID {
+		iptOwnerInfo.Match |= linux.XT_OWNER_GID
+		if matcher.invertGID {
+			iptOwnerInfo.Invert |= linux.XT_OWNER_GID
+		}
 	}
 
 	buf := make([]byte, 0, linux.SizeOfIPTOwnerInfo)
@@ -71,31 +75,34 @@ func (ownerMarshaler) unmarshal(buf []byte, filter stack.IPHeaderFilter) (stack.
 	binary.Unmarshal(buf[:linux.SizeOfIPTOwnerInfo], usermem.ByteOrder, &matchData)
 	nflog("parseMatchers: parsed IPTOwnerInfo: %+v", matchData)
 
-	if matchData.Invert != 0 {
-		return nil, fmt.Errorf("invert flag is not supported for owner match")
-	}
-
-	// Support for UID match.
-	// TODO(gvisor.dev/issue/170): Need to support gid match.
-	if matchData.Match&linux.XT_OWNER_UID != linux.XT_OWNER_UID {
-		return nil, fmt.Errorf("owner match is only supported for uid")
-	}
-
-	// Check Flags.
 	var owner OwnerMatcher
 	owner.uid = matchData.UID
 	owner.gid = matchData.GID
-	owner.matchUID = true
+
+	// Check flags.
+	if matchData.Match&linux.XT_OWNER_UID != 0 {
+		owner.matchUID = true
+		if matchData.Invert&linux.XT_OWNER_UID != 0 {
+			owner.invertUID = true
+		}
+	}
+	if matchData.Match&linux.XT_OWNER_GID != 0 {
+		owner.matchGID = true
+		if matchData.Invert&linux.XT_OWNER_GID != 0 {
+			owner.invertGID = true
+		}
+	}
 
 	return &owner, nil
 }
 
 type OwnerMatcher struct {
-	uid      uint32
-	gid      uint32
-	matchUID bool
-	matchGID bool
-	invert   uint8
+	uid       uint32
+	gid       uint32
+	matchUID  bool
+	matchGID  bool
+	invertUID bool
+	invertGID bool
 }
 
 // Name implements Matcher.Name.
@@ -112,16 +119,30 @@ func (om *OwnerMatcher) Match(hook stack.Hook, pkt stack.PacketBuffer, interface
 	}
 
 	// If the packet owner is not set, drop the packet.
-	// Support for uid match.
-	// TODO(gvisor.dev/issue/170): Need to support gid match.
-	if pkt.Owner == nil || !om.matchUID {
+	if pkt.Owner == nil {
 		return false, true
 	}
 
-	// TODO(gvisor.dev/issue/170): Need to add tests to verify
-	// drop rule when packet UID does not match owner matcher UID.
-	if pkt.Owner.UID() != om.uid {
-		return false, false
+	var matches bool
+	// Check for UID match.
+	if om.matchUID {
+		if pkt.Owner.UID() == om.uid {
+			matches = true
+		}
+		if matches == om.invertUID {
+			return false, false
+		}
+	}
+
+	// Check for GID match.
+	if om.matchGID {
+		matches = false
+		if pkt.Owner.GID() == om.gid {
+			matches = true
+		}
+		if matches == om.invertGID {
+			return false, false
+		}
 	}
 
 	return true, false
diff --git a/test/iptables/filter_output.go b/test/iptables/filter_output.go
index c145bd1e9..ba0d6fc29 100644
--- a/test/iptables/filter_output.go
+++ b/test/iptables/filter_output.go
@@ -29,6 +29,11 @@ func init() {
 	RegisterTestCase(FilterOutputAcceptUDPOwner{})
 	RegisterTestCase(FilterOutputDropUDPOwner{})
 	RegisterTestCase(FilterOutputOwnerFail{})
+	RegisterTestCase(FilterOutputAcceptGIDOwner{})
+	RegisterTestCase(FilterOutputDropGIDOwner{})
+	RegisterTestCase(FilterOutputInvertGIDOwner{})
+	RegisterTestCase(FilterOutputInvertUIDOwner{})
+	RegisterTestCase(FilterOutputInvertUIDAndGIDOwner{})
 	RegisterTestCase(FilterOutputInterfaceAccept{})
 	RegisterTestCase(FilterOutputInterfaceDrop{})
 	RegisterTestCase(FilterOutputInterface{})
@@ -116,20 +121,12 @@ func (FilterOutputAcceptTCPOwner) ContainerAction(ip net.IP) error {
 	}
 
 	// Listen for TCP packets on accept port.
-	if err := listenTCP(acceptPort, sendloopDuration); err != nil {
-		return fmt.Errorf("connection on port %d should be accepted, but got dropped", acceptPort)
-	}
-
-	return nil
+	return listenTCP(acceptPort, sendloopDuration)
 }
 
 // LocalAction implements TestCase.LocalAction.
 func (FilterOutputAcceptTCPOwner) LocalAction(ip net.IP) error {
-	if err := connectTCP(ip, acceptPort, sendloopDuration); err != nil {
-		return fmt.Errorf("connection destined to port %d should be accepted, but got dropped", acceptPort)
-	}
-
-	return nil
+	return connectTCP(ip, acceptPort, sendloopDuration)
 }
 
 // FilterOutputDropTCPOwner tests that TCP connections from uid owner are dropped.
@@ -239,6 +236,158 @@ func (FilterOutputOwnerFail) LocalAction(ip net.IP) error {
 	return nil
 }
 
+// FilterOutputAcceptGIDOwner tests that TCP connections from gid owner are accepted.
+type FilterOutputAcceptGIDOwner struct{}
+
+// Name implements TestCase.Name.
+func (FilterOutputAcceptGIDOwner) Name() string {
+	return "FilterOutputAcceptGIDOwner"
+}
+
+// ContainerAction implements TestCase.ContainerAction.
+func (FilterOutputAcceptGIDOwner) ContainerAction(ip net.IP) error {
+	if err := filterTable("-A", "OUTPUT", "-p", "tcp", "-m", "owner", "--gid-owner", "root", "-j", "ACCEPT"); err != nil {
+		return err
+	}
+
+	// Listen for TCP packets on accept port.
+	return listenTCP(acceptPort, sendloopDuration)
+}
+
+// LocalAction implements TestCase.LocalAction.
+func (FilterOutputAcceptGIDOwner) LocalAction(ip net.IP) error {
+	return connectTCP(ip, acceptPort, sendloopDuration)
+}
+
+// FilterOutputDropGIDOwner tests that TCP connections from gid owner are dropped.
+type FilterOutputDropGIDOwner struct{}
+
+// Name implements TestCase.Name.
+func (FilterOutputDropGIDOwner) Name() string {
+	return "FilterOutputDropGIDOwner"
+}
+
+// ContainerAction implements TestCase.ContainerAction.
+func (FilterOutputDropGIDOwner) ContainerAction(ip net.IP) error {
+	if err := filterTable("-A", "OUTPUT", "-p", "tcp", "-m", "owner", "--gid-owner", "root", "-j", "DROP"); err != nil {
+		return err
+	}
+
+	// Listen for TCP packets on accept port.
+	if err := listenTCP(acceptPort, sendloopDuration); err == nil {
+		return fmt.Errorf("connection on port %d should not be accepted, but got accepted", acceptPort)
+	}
+
+	return nil
+}
+
+// LocalAction implements TestCase.LocalAction.
+func (FilterOutputDropGIDOwner) LocalAction(ip net.IP) error {
+	if err := connectTCP(ip, acceptPort, sendloopDuration); err == nil {
+		return fmt.Errorf("connection destined to port %d should not be accepted, but got accepted", acceptPort)
+	}
+
+	return nil
+}
+
+// FilterOutputInvertGIDOwner tests that TCP connections from gid owner are dropped.
+type FilterOutputInvertGIDOwner struct{}
+
+// Name implements TestCase.Name.
+func (FilterOutputInvertGIDOwner) Name() string {
+	return "FilterOutputInvertGIDOwner"
+}
+
+// ContainerAction implements TestCase.ContainerAction.
+func (FilterOutputInvertGIDOwner) ContainerAction(ip net.IP) error {
+	rules := [][]string{
+		{"-A", "OUTPUT", "-p", "tcp", "-m", "owner", "!", "--gid-owner", "root", "-j", "ACCEPT"},
+		{"-A", "OUTPUT", "-p", "tcp", "-j", "DROP"},
+	}
+	if err := filterTableRules(rules); err != nil {
+		return err
+	}
+
+	// Listen for TCP packets on accept port.
+	if err := listenTCP(acceptPort, sendloopDuration); err == nil {
+		return fmt.Errorf("connection on port %d should not be accepted, but got accepted", acceptPort)
+	}
+
+	return nil
+}
+
+// LocalAction implements TestCase.LocalAction.
+func (FilterOutputInvertGIDOwner) LocalAction(ip net.IP) error {
+	if err := connectTCP(ip, acceptPort, sendloopDuration); err == nil {
+		return fmt.Errorf("connection destined to port %d should not be accepted, but got accepted", acceptPort)
+	}
+
+	return nil
+}
+
+// FilterOutputInvertUIDOwner tests that TCP connections from gid owner are dropped.
+type FilterOutputInvertUIDOwner struct{}
+
+// Name implements TestCase.Name.
+func (FilterOutputInvertUIDOwner) Name() string {
+	return "FilterOutputInvertUIDOwner"
+}
+
+// ContainerAction implements TestCase.ContainerAction.
+func (FilterOutputInvertUIDOwner) ContainerAction(ip net.IP) error {
+	rules := [][]string{
+		{"-A", "OUTPUT", "-p", "tcp", "-m", "owner", "!", "--uid-owner", "root", "-j", "DROP"},
+		{"-A", "OUTPUT", "-p", "tcp", "-j", "ACCEPT"},
+	}
+	if err := filterTableRules(rules); err != nil {
+		return err
+	}
+
+	// Listen for TCP packets on accept port.
+	return listenTCP(acceptPort, sendloopDuration)
+}
+
+// LocalAction implements TestCase.LocalAction.
+func (FilterOutputInvertUIDOwner) LocalAction(ip net.IP) error {
+	return connectTCP(ip, acceptPort, sendloopDuration)
+}
+
+// FilterOutputInvertUIDAndGIDOwner tests that TCP connections from uid and gid
+// owner are dropped.
+type FilterOutputInvertUIDAndGIDOwner struct{}
+
+// Name implements TestCase.Name.
+func (FilterOutputInvertUIDAndGIDOwner) Name() string {
+	return "FilterOutputInvertUIDAndGIDOwner"
+}
+
+// ContainerAction implements TestCase.ContainerAction.
+func (FilterOutputInvertUIDAndGIDOwner) ContainerAction(ip net.IP) error {
+	rules := [][]string{
+		{"-A", "OUTPUT", "-p", "tcp", "-m", "owner", "!", "--uid-owner", "root", "!", "--gid-owner", "root", "-j", "ACCEPT"},
+		{"-A", "OUTPUT", "-p", "tcp", "-j", "DROP"},
+	}
+	if err := filterTableRules(rules); err != nil {
+		return err
+	}
+
+	// Listen for TCP packets on accept port.
+	if err := listenTCP(acceptPort, sendloopDuration); err == nil {
+		return fmt.Errorf("connection on port %d should not be accepted, but got accepted", acceptPort)
+	}
+
+	return nil
+}
+
+// LocalAction implements TestCase.LocalAction.
+func (FilterOutputInvertUIDAndGIDOwner) LocalAction(ip net.IP) error {
+	if err := connectTCP(ip, acceptPort, sendloopDuration); err == nil {
+		return fmt.Errorf("connection destined to port %d should not be accepted, but got accepted", acceptPort)
+	}
+
+	return nil
+}
+
 // FilterOutputDestination tests that we can selectively allow packets to
 // certain destinations.
 type FilterOutputDestination struct{}
diff --git a/test/iptables/iptables_test.go b/test/iptables/iptables_test.go
index 84eb75a40..4fd2cb46a 100644
--- a/test/iptables/iptables_test.go
+++ b/test/iptables/iptables_test.go
@@ -167,6 +167,26 @@ func TestFilterOutputOwnerFail(t *testing.T) {
 	singleTest(t, FilterOutputOwnerFail{})
 }
 
+func TestFilterOutputAcceptGIDOwner(t *testing.T) {
+	singleTest(t, FilterOutputAcceptGIDOwner{})
+}
+
+func TestFilterOutputDropGIDOwner(t *testing.T) {
+	singleTest(t, FilterOutputDropGIDOwner{})
+}
+
+func TestFilterOutputInvertGIDOwner(t *testing.T) {
+	singleTest(t, FilterOutputInvertGIDOwner{})
+}
+
+func TestFilterOutputInvertUIDOwner(t *testing.T) {
+	singleTest(t, FilterOutputInvertUIDOwner{})
+}
+
+func TestFilterOutputInvertUIDAndGIDOwner(t *testing.T) {
+	singleTest(t, FilterOutputInvertUIDAndGIDOwner{})
+}
+
 func TestFilterOutputInterfaceAccept(t *testing.T) {
 	singleTest(t, FilterOutputInterfaceAccept{})
 }