Details

Reviewers

bz
dch

Group Reviewers

manpages

Commits

rG215bab7924f6: mac_ipacl: new MAC policy module to limit jail/vnet IP configuration

Summary

GSoC'19 project on creating a new MAC policy module to limit the VNET jail privileges of setting its IP address - mac_ipacl

The mac_ipacl policy module enables fine-grained control over IP address access within VNET jails on a host system. It allows the root user to define rules governing IP addresses for jails and their interfaces using the sysctl interface.

Steps to dynamically load module and basic test:

Apply the patch and boot into the modified kernel.

Load the mac_ipacl policy module using the following command:

kldload mac_ipacl

Verify that the module is loaded by checking the kernel module status:

kldstat

Create a virtual interface using the following command:

ifconfig epair create

Create a VNET jail and attach the newly created interface (let's say epair0b) using the following command:

jail -c name=jvnet host.hostname=jvnet persist vnet vnet.interface=epair0b

Check the jail ID (jid) using the following command:

jls

Add the IP address access control rules for jail (let's say jid is 1):

sysctl security.mac.ipacl.ipv4=1
sysctl security.mac.ipacl.rules="1,1,,AF_INET,169.254.123.123/24"

Set different IP addresses within the jail to test the access control. For example:

Successful attempt:
     jexec 1 ifconfig epair0b inet 169.254.123.123/24 up
Unsuccessful attempt (permission denied):
     jexec 1 ifconfig epair0b inet 169.254.120.123/24 up

Test Plan

wrote test scripts(Kyua)

kyua test -k /usr/tests/sys/mac/ipacl/Kyuafile
ipacl_test:ipacl_v4 -> passed [0.375s]
ipacl_test:ipacl_v6 -> passed [0.458s]

2/2 passed (0 failed)

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

shivank updated this revision to Diff 60419.Aug 3 2019, 9:43 AM

add ipacl entry in tests Makefile
fix minor issues in mac_ipacl.4

There's a couple of public IP(v6) addresses in the test scripts. We'd prefer not to have accidents with people. Can you please change them?

tests/sys/mac/ipacl/ipacl_test.sh
94 ↗	(On Diff #60510)	That .112 is not a TEST-NET but a public assigned address of someone.
104 ↗	(On Diff #60510)	Again .112 is not good.
171 ↗	(On Diff #60510)	Is that intended? 1:470:... doesn't look like an appropriate address.
181 ↗	(On Diff #60510)	See above.
266 ↗	(On Diff #60510)	This also is not a good address to use in testing. It's a public APNIC address (possibly unassigned currently).

correct the IP addresses which were not in the documentation range

shivank marked 5 inline comments as done.Aug 9 2019, 5:44 PM

shivank updated this revision to Diff 60618.Aug 9 2019, 6:54 PM

debdrup added a subscriber: debdrup.Aug 10 2019, 6:51 AM

bz accepted this revision.Aug 19 2019, 1:22 PM

This revision is now accepted and ready to land.Aug 19 2019, 1:22 PM

emaste added a subscriber: emaste.Feb 25 2020, 11:05 PM

Herald added a subscriber: melifaro. · View Herald TranscriptFeb 25 2020, 11:05 PM

marieheleneka_gmail.com added a subscriber: marieheleneka_gmail.com.Apr 17 2020, 7:16 PM

kfv_kfv.io added a subscriber: kfv_kfv.io.Jun 17 2020, 6:34 AM

dch added a subscriber: dch.Jun 17 2020, 10:23 PM

antranigv_freebsd.am added a subscriber: antranigv_freebsd.am.Jun 20 2020, 7:07 PM

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

In D20967#632260, @dch wrote:

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

If that's the case, I can test this patch again. I can also devote some time to work on it, if there's suggestion from community.
What do you think @bz?

In D20967#632260, @dch wrote:

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

This appears to have been accepted but not merged - it would be great to have it get into 14.0 if there's still time

Herald added a subscriber: glebius. · View Herald TranscriptJun 14 2023, 10:48 PM

Some nits in the manual page, which can probably be fixed in a follow-up commit (with .Dd bumped) if the code still works.

share/man/man4/mac_ipacl.4
54–60 ↗	(On Diff #60618)	Otherwise it looks like "via sysctl(8) interface" applies to "setting IPv4 and IPv6 addresses". (While there, tweak wording a bit.)
69 ↗	(On Diff #60618)
102 ↗	(On Diff #60618)
109 ↗	(On Diff #60618)
113 ↗	(On Diff #60618)
114 ↗	(On Diff #60618)
127 ↗	(On Diff #60618)
131 ↗	(On Diff #60618)
133 ↗	(On Diff #60618)
138 ↗	(On Diff #60618)
140–142 ↗	(On Diff #60618)

In D20967#923964, @pauamma_gundo.com wrote:

Some nits in the manual page, which can probably be fixed in a follow-up commit (with .Dd bumped) if the code still works.

Agreed, but let's see if the patches still apply. I will attempt to reach out to the original author.

Hi,
thanks for the comments.
I'll test my patch and wil apply the suggested changes in man page.
Thanks

rebased the code on top of latest src changes
made changes to man page as suggested in comments

This revision now requires review to proceed.Jul 1 2023, 6:10 PM

melifaro added inline comments.Jul 2 2023, 11:09 AM

sys/netinet/in.c
379
sys/netinet6/in6.c
562
sys/security/mac/mac_inet.c
113	I'd suggest naming it something like `mac_inet_check_add_addr()` for the following reasons: there are 2 entry points for adding the addresses - it's ioctl and netlink & the code will be changed to call business logic from both, instead of netlink calling ioctl. SIOCAIFADDR is hardly readable and you have to have some domain knowledge of the ioctl code to understand what it actually means. it will be consistent with other function names
sys/security/mac_ipacl/mac_ipacl.c
157	Nit: no blank line needed per the style(9) rules.
165	The only error you're returning here is `EINVAL`. Maybe it's worth returning `struct ip_rule` directly and return `EINVAL` in the caller code if the rule is NULL?
188	do you actually need `goto out` here and in the next invocations? The code mostly uses local variables to save the parsed state. I'd suggest to either stick to that pattern & allocate the rule when parsing is done, eliminating got, or skip local variables and save the state directly to the allocated rule, but not the mixture of those.
196	sizeof() is a compile-only check in C.
201	Nit: memset & strlcpy. Also, `new` is already allocated zeroed.
292	To the previous comment on error handling & goto - you can alternatively allocate the rule here.
293
304	Frankly speaking, I have mixed feeling about these. We do a lot of string argument parsing & generation in the kernel - the thing that people typically avoid, especially in security-related code. The "serialized" argument one needs to. read/write is also hard to remember & doesn't allow a lot of extendability. I wouldn't reject the review based on that - it's outside of my domain expertise, but I strongly suggest looking into creating generic netlink family for that instead. With that approach you'll get easily extenable format & avoid all string parsing alltogether.

Overall is a really nice addition and I'd love to have it in base. I have one concern on the rule import/export implementation - happy to discuss this further & left a couple of non-critical comments on the code.
Thank you for working on this!

Manual page LGTM now, will rereview once the source code is finalized.

share/man/man4/mac_ipacl.4
67 ↗	(On Diff #124071)	Missed that the first time: US spelling.

The @ character is visually quite large and makes it hard to scan and read for a human.

I can see the port_acl module uses : as a separator, but that is presumably problematic for IPv6 addresses?

Either require IPv6 to have [....] wrappers, or perhaps consider a different character that is less visually obtrusive?

Some of ,_;-/ might work?

Finally, if practical, using a common / to separate optional subnet, would at least let the IP/subnet part be the same syntax as everywhere else:

1@1@epair0b@AF_INET@192.0.2.2@24

could become 1,1,epair0b,AF_INET,192.0.2.2/24

1,1,epair0b,AF_INET,1.2.3.4/24
1,1,epair0b,AF_INET6,fd::7/128

1:1:epair0b:AF_INET:192.0.2.2/24
1:1:epair0b:AF_INET6:[fd::7/128]

Melifaro's point about avoiding parsing in the kernel is a good one. What do you think could be improved here?

share/man/man4/mac_ipacl.4
36 ↗	(On Diff #124071)	s/for compiling/to compile/
43 ↗	(On Diff #124071)	"To load the mac_ipacl policy module at boot time, add the following lien to your kernel configuration file" reads better.
96 ↗	(On Diff #124071)	is it possible to set the jail id to * such that it applies to all jails? The jid is not necessarily known in advance of jail creation, which makes assigning them in a static config more tricky. No need to change the code today, but this would be a great future enhancement. As you note further down in the manpage, a better way of setting this would be nice. Perhaps these can be attached somehow at jail creation.
149 ↗	(On Diff #124071)	I think this should be more clear - is this, if an IP address, (or subnet), is mentioned repeatedly, only the final rule is applied? i.e. all previous ones are ignored? or are they somehow cumulative, in order, with the last one potentially overriding?

This revision now requires changes to proceed.Jul 6 2023, 10:35 AM

If you're going to change format, why not use tree, as it feels natural for sysctl. One existing example would be dev.pcm.<number> so following that, we could have security.mac.ipacl.<jid>.<rule> and .family and .address (or maybe .range) as leafs. It is not set in stone that it has to be like that, but given it's sysctl, it feels more natural to me, so please give it a thought. Also, as it is about jail, it would also feel more natural to have this tunables under security.jail.<jid>

Implemented recommended changes
Made style changes
modifed the rules_check function to traverse the list in reverse order and stop when first applicable rule (matching IP address) is found. Since, rules defined later determine have higer priority, checking last matching rule is enough.

Melifaro's point about avoiding parsing in the kernel is a good one. What do you think could be improved here?

If you're going to change format, why not use tree, as it feels natural for sysctl. One existing example would be dev.pcm.<number> so following that, we could have security.mac.ipacl.<jid>.<rule> and .family and .address (or maybe .range) as leafs. It is not set in stone that it has to be like that, but given it's sysctl, it feels more natural to me, so please give it a thought. Also, as it is about jail, it would also feel more natural to have this tunables under security.jail.<jid>

I agree with Meliaro's point to avoid parsing rule in kernel and his suggestion to use netlink interface. I'll implement it as enhancement.

Thanks for the sugestion. It'll a good approach to store multiple rules but would require some significant design and tests modification. I'll consider it as future work.
In future, it can be cosidered to extend this policy to host. Also, it may good to keep mac_ipacl with other mac policies under security.mac

share/man/man4/mac_ipacl.4
96 ↗	(On Diff #124071)	It's a good suggestion to use a jail wildcard, like we did for Interface. will consider it as future enhancement.
149 ↗	(On Diff #124071)	rephrased.
sys/security/mac_ipacl/mac_ipacl.c
304	I agree with your suggestion. using sysctl makes it very limited. will implement it as future enhancement.

crest_freebsd_rlwinm.de added a subscriber: crest_freebsd_rlwinm.de.Jul 12 2023, 5:03 PM

LGTM, but as I don't have a src bit I can't commit this. Anybody else willing to do that?

This revision is now accepted and ready to land.Jul 15 2023, 10:15 AM

Someone should do (at least) an amd64 universe build for this to make sure the NO-INET NO-INET6 NO-IP (do we still have that) builds are surviving.

In D20967#934400, @bz wrote:

Someone should do (at least) an amd64 universe build for this to make sure the NO-INET NO-INET6 NO-IP (do we still have that) builds are surviving.

- ident		GENERIC
+ include		LINT-NOINET
+ include		LINT-NOINET6
+ include		LINT-NOIP
+ ident		MYKERNEL

- options 	SCHED_ULE		# ULE scheduler
+ #options 	SCHED_ULE		# ULE scheduler

- options 	INET			# InterNETworking
- options 	INET6			# IPv6 communications protocols
+ #options 	INET			# InterNETworking
+ #options 	INET6			# IPv6 communications protocols

Hi @bz,
thanks for the suggestion.
I'm trying to modify GENERIC config to compile the NO-INET, NO-INET6, NO-IP build.
I realized my changes are breaking the build. It had a minor issue that some variables become unused when above config is used.
following lines should fix it.

--- a/sys/security/mac_ipacl/mac_ipacl.c
+++ b/sys/security/mac_ipacl/mac_ipacl.c
@@ -163,7 +163,10 @@ static int
 parse_rule_element(char *element, struct ip_rule *rule)
 {
        char *tok, *p;
-       int prefix, i;
+       int prefix;
+#ifdef INET6
+       int i;
+#endif
@@ -319,8 +322,13 @@ rules_check(struct ucred *cred,
     struct ipacl_addr *ip_addr, struct ifnet *ifp)
 {
        struct ip_rule *rule;
-       int error, i;
+       int error;
+#ifdef INET6
+       int i;
        bool same_subnet;
+#endif

Though I'm still encountering some issues - linking errors from undefined symbols, which do not originate from my modifications.
ld: error: undefined symbol: in_pseudo
ld: error: undefined symbol: in_cksum_skip
... so on.

Can you please point to correct kernel config to be used?

updated the variable declaration and scope considering NO-INET, NO-INET6, NO-IP.

This revision now requires review to proceed.Jul 16 2023, 10:58 AM

Can you please point to correct kernel config to be used?

make -j<n> tinderbox TARGETS=amd64

will do all of them for you and leave log files for each in case something doesn't work. Replace <n> with the value of the number of CPUs (sysctl hw.ncpu).

So for example with 30 cores you would run:

make -j30 tinderbox TARGETS=amd64

In D20967#934539, @bz wrote:

make -j<n> tinderbox TARGETS=amd64

root@freebsd:/usr/src # cat _.tinderbox.failed
amd64 LINT-NOIP kernel failed, check _.amd64.LINT-NOIP for details
amd64 LINT-NOINET kernel failed, check _.amd64.LINT-NOINET for details

root@freebsd:/usr/src # tail -15 _.amd64.LINT-NOINET
linking kernel
ld: error: undefined symbol: arp_ifinit
>>> referenced by qlnx_os.c
>>>               qlnx_os.o:(qlnx_ioctl)
*** [kernel] Error code 1

make[5]: stopped in /usr/obj/usr/src/amd64.amd64/sys/LINT-NOINET
1 error

root@freebsd:/usr/src # tail -15 _.amd64.LINT-NOIP
linking kernel
ld: error: undefined symbol: arp_ifinit
>>> referenced by qlnx_os.c
>>>               qlnx_os.o:(qlnx_ioctl)
*** [kernel] Error code 1

make[5]: stopped in /usr/obj/usr/src/amd64.amd64/sys/LINT-NOIP
1 error

The error does not look related to my code. Above two builds are failing in qlnx_os.c

ld: error: undefined symbol: arp_ifinit

this issue is fixed in latest code in a commit last week https://github.com/freebsd/freebsd-src/commit/5684c8783b64e33f0dab058126b36776adcc8e82

I pulled the latest changes to freebsd HEAD. NO-INET, NO-INET6,NO-IP are building without any issues.
btw these started

root@freebsd:/usr/src # cat _.tinderbox.failed
amd64 FIRECRACKER kernel failed, check _.amd64.FIRECRACKER for details
amd64 MINIMAL kernel failed, check _.amd64.MINIMAL for details
amd64 MINIMALUP kernel failed, check _.amd64.MINIMALUP for details

Issue seem to be unrelated to this code.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 26 2023, 12:12 AM

Closed by commit rG215bab7924f6: mac_ipacl: new MAC policy module to limit jail/vnet IP configuration (authored by shivank, committed by bz). · Explain Why

This revision was automatically updated to reflect the committed changes.

bz added a commit: rG215bab7924f6: mac_ipacl: new MAC policy module to limit jail/vnet IP configuration.

new MAC policy module - mac_ipacl
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 59937

sys/modules/mac_ipacl/Makefile

sys/netinet/in.c

sys/netinet6/in6.c

sys/security/mac/mac_framework.h

sys/security/mac/mac_inet.c

sys/security/mac/mac_inet6.c

sys/security/mac/mac_policy.h

sys/security/mac_ipacl/Makefile

sys/security/mac_ipacl/mac_ipacl.4

sys/security/mac_ipacl/mac_ipacl.c

sys/security/mac_ipacl/tests/Makefile

sys/security/mac_ipacl/tests/ip4_test.sh

sys/security/mac_ipacl/tests/ip6_test.sh

sys/security/mac_ipacl/tests/ipacl_script.sh

new MAC policy module - mac_ipaclClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 59937

sys/modules/mac_ipacl/Makefile

sys/netinet/in.c

sys/netinet6/in6.c

sys/security/mac/mac_framework.h

sys/security/mac/mac_inet.c

sys/security/mac/mac_inet6.c

sys/security/mac/mac_policy.h

sys/security/mac_ipacl/Makefile

sys/security/mac_ipacl/mac_ipacl.4

sys/security/mac_ipacl/mac_ipacl.c

sys/security/mac_ipacl/tests/Makefile

sys/security/mac_ipacl/tests/ip4_test.sh

sys/security/mac_ipacl/tests/ip6_test.sh

sys/security/mac_ipacl/tests/ipacl_script.sh

new MAC policy module - mac_ipacl
ClosedPublic
Actions

Revision Contents
Changeset List