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.

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

merged 1 commit into from
Apr 2, 2023

Conversation

konradh
Copy link
Contributor

@konradh konradh commented Mar 13, 2023

This also implements parsing of concatenated data types and makes parsing datatypes of sets from magic values more efficient by using a map instead of looping over an array.

I think this fixes Problem 1 from #171.

This also implements parsing of concatenated data types.
@stapelberg
Copy link
Collaborator

Looks reasonable to me, but I haven’t worked much with sets.

cc @sbezverk and @turekt in case they have any additional comments

Otherwise, I’ll merge this next week

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?

@stapelberg stapelberg merged commit a93939a into google:main Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants