new MAC policy module - mac_ipacl

[shivank]

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:

1. Apply the patch and boot into the modified kernel.

2. Load the mac_ipacl policy module using the following command:

kldload mac_ipacl

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

kldstat

4. Create a virtual interface using the following command:

ifconfig epair create

5. 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

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

jls

7. 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"

8. 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)

[shivank]

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

[bz]

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).

[shivank]

• correct the IP addresses which were not in the documentation range

[dch]

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

[shivank]

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?

[editor_callfortesting.org]

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

[pauamma_gundo.com]

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)

[editor_callfortesting.org]

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.

[shivank]

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

[shivank]

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

[melifaro]

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:

1. 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.
2. SIOCAIFADDR is hardly readable and you have to have some domain knowledge of the ioctl code to understand what it actually means.
3. 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.

[melifaro]

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!

[pauamma_gundo.com]

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.

[dch]

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

Or

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?

[meka_tilda.center]

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>

[shivank]

• 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.

[shivank]

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.

[dch]

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

[bz]

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.

[shivank]

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?

[shivank]

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

[bz]

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

[shivank]

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

[shivank]

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.