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

Fix getting concatenated data types for maps #217

Merged
merged 1 commit into from
Apr 2, 2023
Merged

Fix getting concatenated data types for maps #217

Show file tree
Hide file tree
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
171 changes: 85 additions & 86 deletions set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,53 +117,53 @@ var (
TypeTimeDay = SetDatatype{Name: "day", Bytes: 1, nftMagic: 45}
TypeCGroupV2 = SetDatatype{Name: "cgroupsv2", Bytes: 8, nftMagic: 46}

nftDatatypes = map[string]SetDatatype{
TypeVerdict.Name: TypeVerdict,
TypeNFProto.Name: TypeNFProto,
TypeBitmask.Name: TypeBitmask,
TypeInteger.Name: TypeInteger,
TypeString.Name: TypeString,
TypeLLAddr.Name: TypeLLAddr,
TypeIPAddr.Name: TypeIPAddr,
TypeIP6Addr.Name: TypeIP6Addr,
TypeEtherAddr.Name: TypeEtherAddr,
TypeEtherType.Name: TypeEtherType,
TypeARPOp.Name: TypeARPOp,
TypeInetProto.Name: TypeInetProto,
TypeInetService.Name: TypeInetService,
TypeICMPType.Name: TypeICMPType,
TypeTCPFlag.Name: TypeTCPFlag,
TypeDCCPPktType.Name: TypeDCCPPktType,
TypeMHType.Name: TypeMHType,
TypeTime.Name: TypeTime,
TypeMark.Name: TypeMark,
TypeIFIndex.Name: TypeIFIndex,
TypeARPHRD.Name: TypeARPHRD,
TypeRealm.Name: TypeRealm,
TypeClassID.Name: TypeClassID,
TypeUID.Name: TypeUID,
TypeGID.Name: TypeGID,
TypeCTState.Name: TypeCTState,
TypeCTDir.Name: TypeCTDir,
TypeCTStatus.Name: TypeCTStatus,
TypeICMP6Type.Name: TypeICMP6Type,
TypeCTLabel.Name: TypeCTLabel,
TypePktType.Name: TypePktType,
TypeICMPCode.Name: TypeICMPCode,
TypeICMPV6Code.Name: TypeICMPV6Code,
TypeICMPXCode.Name: TypeICMPXCode,
TypeDevGroup.Name: TypeDevGroup,
TypeDSCP.Name: TypeDSCP,
TypeECN.Name: TypeECN,
TypeFIBAddr.Name: TypeFIBAddr,
TypeBoolean.Name: TypeBoolean,
TypeCTEventBit.Name: TypeCTEventBit,
TypeIFName.Name: TypeIFName,
TypeIGMPType.Name: TypeIGMPType,
TypeTimeDate.Name: TypeTimeDate,
TypeTimeHour.Name: TypeTimeHour,
TypeTimeDay.Name: TypeTimeDay,
TypeCGroupV2.Name: TypeCGroupV2,
nftDatatypes = []SetDatatype{
TypeVerdict,
TypeNFProto,
TypeBitmask,
TypeInteger,
TypeString,
TypeLLAddr,
TypeIPAddr,
TypeIP6Addr,
TypeEtherAddr,
TypeEtherType,
TypeARPOp,
TypeInetProto,
TypeInetService,
TypeICMPType,
TypeTCPFlag,
TypeDCCPPktType,
TypeMHType,
TypeTime,
TypeMark,
TypeIFIndex,
TypeARPHRD,
TypeRealm,
TypeClassID,
TypeUID,
TypeGID,
TypeCTState,
TypeCTDir,
TypeCTStatus,
TypeICMP6Type,
TypeCTLabel,
TypePktType,
TypeICMPCode,
TypeICMPV6Code,
TypeICMPXCode,
TypeDevGroup,
TypeDSCP,
TypeECN,
TypeFIBAddr,
TypeBoolean,
TypeCTEventBit,
TypeIFName,
TypeIGMPType,
TypeTimeDate,
TypeTimeHour,
TypeTimeDay,
TypeCGroupV2,
}

// ctLabelBitSize is defined in https://git.netfilter.org/nftables/tree/src/ct.c.
Expand All @@ -177,6 +177,19 @@ var (
sizeOfGIDT uint32 = 4
)

var nftDatatypesByName map[string]SetDatatype
var nftDatatypesByMagic map[uint32]SetDatatype

// Create maps for efficient datatype lookup.
func init() {
nftDatatypesByName = make(map[string]SetDatatype, len(nftDatatypes))
nftDatatypesByMagic = make(map[uint32]SetDatatype, len(nftDatatypes))
for _, dt := range nftDatatypes {
nftDatatypesByName[dt.Name] = dt
nftDatatypesByMagic[dt.nftMagic] = dt
}
}

// ErrTooManyTypes is the error returned by ConcatSetType, if nftMagic would overflow.
var ErrTooManyTypes = errors.New("too many types to concat")

Expand Down Expand Up @@ -221,7 +234,7 @@ func ConcatSetTypeElements(t SetDatatype) []SetDatatype {
names := strings.Split(t.Name, " . ")
types := make([]SetDatatype, len(names))
for i, n := range names {
types[i] = nftDatatypes[n]
types[i] = nftDatatypesByName[n]
}
return types
}
Expand Down Expand Up @@ -678,60 +691,46 @@ func setsFromMsg(msg netlink.Message) (*Set, error) {
set.Concatenation = (flags & NFT_SET_CONCAT) != 0
case unix.NFTA_SET_KEY_TYPE:
nftMagic := ad.Uint32()
if invalidMagic, ok := validateKeyType(nftMagic); !ok {
return nil, fmt.Errorf("could not determine key type %+v", invalidMagic)
}
set.KeyType.nftMagic = nftMagic
for _, dt := range nftDatatypes {
// If this is a non-concatenated type, we can assign the descriptor.
if nftMagic == dt.nftMagic {
set.KeyType = dt
break
}
dt, err := parseSetDatatype(nftMagic)
if err != nil {
return nil, fmt.Errorf("could not determine data type: %w", err)
}
set.KeyType = dt
case unix.NFTA_SET_DATA_TYPE:
nftMagic := ad.Uint32()
// Special case for the data type verdict, in the message it is stored as 0xffffff00 but it is defined as 1
if nftMagic == 0xffffff00 {
set.KeyType = TypeVerdict
break
}
for _, dt := range nftDatatypes {
if nftMagic == dt.nftMagic {
set.DataType = dt
break
}
}
if set.DataType.nftMagic == 0 {
return nil, fmt.Errorf("could not determine data type %x", nftMagic)
dt, err := parseSetDatatype(nftMagic)
if err != nil {
return nil, fmt.Errorf("could not determine data type: %w", err)
}
set.DataType = dt
}
}
return &set, nil
}

func validateKeyType(bits uint32) ([]uint32, bool) {
var unpackTypes []uint32
var invalidTypes []uint32
found := false
valid := true
for bits != 0 {
unpackTypes = append(unpackTypes, bits&SetConcatTypeMask)
bits = bits >> SetConcatTypeBits
}
for _, t := range unpackTypes {
for _, dt := range nftDatatypes {
if t == dt.nftMagic {
found = true
}
}
if !found {
invalidTypes = append(invalidTypes, t)
valid = false
func parseSetDatatype(magic uint32) (SetDatatype, error) {
types := make([]SetDatatype, 0, 32/SetConcatTypeBits)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line made me wonder whether cap of 5 types is valid so I researched a bit and found this line in the original source https://git.netfilter.org/nftables/tree/src/datatype.c?id=1acc2fd48c755a8931fa87b8d0560b750316059f#n1165

Code never restricts concat type length to 5 and it is possible to add 6 types via the CLI:

$ nft flush ruleset
$ nft add table ip filter
$ nft add set filter setA { type nf_proto . ipv4_addr . inet_service . ipv4_addr . ipv6_addr . ipv6_addr \; flags timeout \; }
$ nft list ruleset
table ip filter {
	set setA {
		type nf_proto . ipv4_addr . inet_service . ipv4_addr . ipv6_addr . ipv6_addr
		flags timeout
	}
}

In case there is an overflow, the first type gets either ignored or converted (type is reduced to two bits, e.g. nf_proto), meaning that the cap is not specifically checked:

$ nft flush ruleset
$ nft add table ip filter
$ # ether_type is 10 so it gets reduced to two bits and converted into nf_proto (2)
$ nft add set filter setB { type ether_type . ipv4_addr . inet_service . ipv4_addr . ipv6_addr . ipv6_addr \; flags timeout \; }
$ nft list ruleset
table ip filter {
	set setB {
		type nf_proto . ipv4_addr . inet_service . ipv4_addr . ipv6_addr . ipv6_addr
		flags timeout
	}
}

Now, I am not sure whether using 6 types is a legitimate use case but I see that we check if there is more than 5 types beforehand

nftables/set.go

Line 197 in 2729c5a

if len(types) > 32/SetConcatTypeBits {
and with this check, the cap makes sense.

I will leave it up to @konradh to decide if he wants to leave this cap or not.

cc @stapelberg for awareness of this limitation, maybe good to note down in case someone raises an issue in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@konradh what do you think?

for magic != 0 {
t := magic & SetConcatTypeMask
magic = magic >> SetConcatTypeBits
dt, ok := nftDatatypesByMagic[t]
if !ok {
return TypeInvalid, fmt.Errorf("could not determine data type %+v", dt)
}
found = false
// Because we start with the last type, we insert the later types at the front.
types = append([]SetDatatype{dt}, types...)
}

dt, err := ConcatSetType(types...)
if err != nil {
return TypeInvalid, fmt.Errorf("could not create data type: %w", err)
}
return invalidTypes, valid
return dt, nil
}

var elemHeaderType = netlink.HeaderType((unix.NFNL_SUBSYS_NFTABLES << 8) | unix.NFT_MSG_NEWSETELEM)
Expand Down
38 changes: 21 additions & 17 deletions set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package nftables

import (
"reflect"
"testing"
)

Expand All @@ -17,58 +16,63 @@ func genSetKeyType(types ...uint32) uint32 {
return c
}

func TestValidateNFTMagic(t *testing.T) {
func TestParseSetDatatype(t *testing.T) {
t.Parallel()
tests := []struct {
name string
nftMagicPacked uint32
pass bool
invalid []uint32
typeName string
typeBytes uint32
}{
{
name: "Single valid nftMagic",
nftMagicPacked: genSetKeyType(7),
nftMagicPacked: genSetKeyType(TypeIPAddr.nftMagic),
pass: true,
invalid: nil,
typeName: "ipv4_addr",
typeBytes: 4,
},
{
name: "Single unknown nftMagic",
nftMagicPacked: genSetKeyType(unknownNFTMagic),
pass: false,
invalid: []uint32{unknownNFTMagic},
},
{
name: "Multiple valid nftMagic",
nftMagicPacked: genSetKeyType(7, 13),
nftMagicPacked: genSetKeyType(TypeIPAddr.nftMagic, TypeInetService.nftMagic),
pass: true,
invalid: nil,
typeName: "ipv4_addr . inet_service",
typeBytes: 8,
},
{
name: "Multiple nftMagic with 1 unknown",
nftMagicPacked: genSetKeyType(7, 13, unknownNFTMagic),
nftMagicPacked: genSetKeyType(TypeIPAddr.nftMagic, TypeInetService.nftMagic, unknownNFTMagic),
pass: false,
invalid: []uint32{unknownNFTMagic},
},
{
name: "Multiple nftMagic with 2 unknown",
nftMagicPacked: genSetKeyType(7, 13, unknownNFTMagic, unknownNFTMagic+1),
nftMagicPacked: genSetKeyType(TypeIPAddr.nftMagic, TypeInetService.nftMagic, unknownNFTMagic, unknownNFTMagic+1),
pass: false,
invalid: []uint32{unknownNFTMagic + 1, unknownNFTMagic},
// Invalid entries will appear in reverse order
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
invalid, pass := validateKeyType(tt.nftMagicPacked)
datatype, err := parseSetDatatype(tt.nftMagicPacked)
pass := err == nil
if pass && !tt.pass {
t.Fatalf("expected to fail but succeeded")
}
if !pass && tt.pass {
t.Fatalf("expected to succeed but failed with invalid nftMagic: %+v", invalid)
t.Fatalf("expected to succeed but failed: %s", err)
}
if !reflect.DeepEqual(tt.invalid, invalid) {
t.Fatalf("Expected invalid: %+v but got: %+v", tt.invalid, invalid)
expected := SetDatatype{
Name: tt.typeName,
Bytes: tt.typeBytes,
nftMagic: tt.nftMagicPacked,
}
if pass && datatype != expected {
t.Fatalf("invalid datatype: expected %+v but got %+v", expected, datatype)
}
})
}
Expand Down