Page MenuHomeFreeBSD
Differential D20967

new MAC policy module - mac_ipacl
ClosedPublic
Actions

Authored by shivank on Jul 16 2019, 9:49 AM.
Tags
Referenced Files
F81874153: D20967.id.diff
Sun, Nov 24, 6:41 PM
F81872877: D20967.id.diff
Sun, Nov 24, 10:57 AM
Unknown Object (File)
Thu, Nov 21, 12:56 PM
Unknown Object (File)
Thu, Nov 21, 9:07 AM
Unknown Object (File)
Thu, Nov 21, 7:12 AM
Unknown Object (File)
Thu, Nov 21, 3:19 AM
Unknown Object (File)
Thu, Nov 21, 1:54 AM
Unknown Object (File)
Thu, Nov 21, 1:54 AM
View All Files
Subscribers
0mp
ae
antranigv_freebsd.am
crest_freebsd_rlwinm.de
dch
debdrup
editor_callfortesting.org
View All 15 Subscribers

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:

  1. Apply the patch and boot into the modified kernel.
  1. Load the mac_ipacl policy module using the following command:
kldload mac_ipacl
  1. Verify that the module is loaded by checking the kernel module status:
kldstat
  1. Create a virtual interface using the following command:
ifconfig epair create
  1. 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
  1. Check the jail ID (jid) using the following command:
jls
  1. 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"
  1. 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

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
shivank updated this revision to Diff 60510.Aug 6 2019, 4:25 PM
  • add ipacl entry in tests Makefile
  • fix minor issues in mac_ipacl.4
bz added a comment.Aug 9 2019, 8:53 AM

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
95

That .112 is not a TEST-NET but a public assigned address of someone.

105

Again .112 is not good.

172

Is that intended? 1:470:... doesn't look like an appropriate address.

182

See above.

267

This also is not a good address to use in testing. It's a public APNIC address (possibly unassigned currently).

shivank updated this revision to Diff 60617.Aug 9 2019, 5:42 PM
shivank marked 3 inline comments as done.
  • 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
dch added a comment.Jan 21 2021, 10:05 AM

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 added a comment.Jan 21 2021, 11:31 AM
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 added a subscriber: editor_callfortesting.org.Jun 14 2023, 10:48 PM
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
pauamma_gundo.com added a subscriber: pauamma_gundo.com.Jun 16 2023, 11:08 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
55–61

Otherwise it looks like "via sysctl(8) interface" applies to "setting IPv4 and IPv6 addresses". (While there, tweak wording a bit.)

70
103
110
114
115
128
132
134
139
141–143
editor_callfortesting.org added a comment.Jun 19 2023, 5:34 AM
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 added a comment.Jun 19 2023, 6:04 AM

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

shivank updated this revision to Diff 124071.Jul 1 2023, 6:10 PM
shivank marked 11 inline comments as done.
shivank edited the summary of this revision. (Show Details)
shivank edited the test plan for this revision. (Show Details)
  • 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
497
sys/netinet6/in6.c
577
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 added a comment.Jul 2 2023, 11:10 AM

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 added a comment.Jul 3 2023, 12:21 AM

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

share/man/man4/mac_ipacl.4
68

Missed that the first time: US spelling.

dch requested changes to this revision.Jul 6 2023, 10:35 AM

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
37

s/for compiling/to compile/

44

"To load the mac_ipacl policy module at boot time, add the following lien to your kernel configuration file" reads better.

97

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.

150

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
meka_tilda.center added a subscriber: meka_tilda.center.Jul 6 2023, 5:06 PM

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 updated this revision to Diff 124363.Jul 8 2023, 8:45 PM
shivank marked 16 inline comments as done.
shivank edited the summary of this revision. (Show Details)
shivank edited the test plan for this revision. (Show Details)
  • 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 added a comment.Jul 8 2023, 9:20 PM

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
97

It's a good suggestion to use a jail wildcard, like we did for Interface.
will consider it as future enhancement.

150

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
dch accepted this revision.Jul 15 2023, 10:15 AM

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
bz added a comment.Jul 15 2023, 4:40 PM

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 added a comment.EditedJul 15 2023, 9:12 PM
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 this revision to Diff 124732.Jul 16 2023, 10:58 AM

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
bz added a comment.Jul 16 2023, 1:35 PM

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 added a comment.Jul 16 2023, 9:35 PM
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 added a comment.Jul 17 2023, 11:30 AM

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.

Revision Contents
Changeset List

PathSize
etc/
mtree/
2 lines
share/
man/
man4/
1 line
4 lines
166 lines
sys/
conf/
1 line
1 line
1 line
modules/
2 lines
mac_ipacl/
9 lines
netinet/
11 lines
netinet6/
10 lines
security/
mac/
9 lines
11 lines
11 lines
12 lines
mac_ipacl/
453 lines
tests/
sys/
mac/
1 line
ipacl/
11 lines
282 lines
18 lines

Diff 125165

View Options

etc/mtree/BSD.tests.dist

Loading...
View Options

share/man/man4/Makefile

Loading...
View Options

share/man/man4/mac.4

Loading...
View Options

share/man/man4/mac_ipacl.4

Loading...
View Options

sys/conf/NOTES

Loading...
View Options

sys/conf/files

Loading...
View Options

sys/conf/options

Loading...
View Options

sys/modules/Makefile

Loading...
View Options

sys/modules/mac_ipacl/Makefile

Loading...
View Options

sys/netinet/in.c

Loading...
View Options

sys/netinet6/in6.c

Loading...
View Options

sys/security/mac/mac_framework.h

Loading...
View Options

sys/security/mac/mac_inet.c

Loading...
View Options

sys/security/mac/mac_inet6.c

Loading...
View Options

sys/security/mac/mac_policy.h

Loading...
View Options

sys/security/mac_ipacl/mac_ipacl.c

Loading...
View Options

tests/sys/mac/Makefile

Loading...
View Options

tests/sys/mac/ipacl/Makefile

Loading...
View Options

tests/sys/mac/ipacl/ipacl_test.sh

Loading...
View Options

tests/sys/mac/ipacl/utils.subr

Loading...