Changeset View
Standalone View
sys/security/mac_ipacl/mac_ipacl.c
- This file was added.
| /*- | |||||||||
| * Copyright (c) 2003-2004 Networks Associates Technology, Inc. | |||||||||
| * Copyright (c) 2006 SPARTA, Inc. | |||||||||
| * Copyright (c) 2019, 2023 Shivank Garg <shivank@FreeBSD.org> | |||||||||
| * | |||||||||
| * This software was developed for the FreeBSD Project by Network | |||||||||
bz: Please remove me everywhere from the (c) | |||||||||
| * Associates Laboratories, the Security Research Division of Network | |||||||||
| * Associates, Inc. under DARPA/SPAWAR contract N66001-01-C-8035 ("CBOSS"), | |||||||||
| * as part of the DARPA CHATS research program. | |||||||||
| * | |||||||||
Done Inline ActionsYou might still want to bring the copyrights/attributions/license over from the other mac module. bz: You might still want to bring the copyrights/attributions/license over from the other mac… | |||||||||
Done Inline ActionsI have added (c) from mac_portacl, shivank: I have added (c) from mac_portacl,
License is the same in all modules,
Please suggest… | |||||||||
Done Inline ActionsAll the ones which were on the file where you "copied" the code from. Think of initially copying another file to a new name and then adjusting the code and adding your copyright. bz: All the ones which were on the file where you "copied" the code from. Think of initially… | |||||||||
| * This software was enhanced by SPARTA ISSO under SPAWAR contract | |||||||||
| * N66001-04-C-6019 ("SEFOS"). | |||||||||
| * | |||||||||
| * This code was developed as a Google Summer of Code 2019 project | |||||||||
| * under the guidance of Bjoern A. Zeeb. | |||||||||
| * | |||||||||
| * Redistribution and use in source and binary forms, with or without | |||||||||
| * modification, are permitted provided that the following conditions | |||||||||
| * are met: | |||||||||
| * 1. Redistributions of source code must retain the above copyright | |||||||||
| * notice, this list of conditions and the following disclaimer. | |||||||||
| * 2. Redistributions in binary form must reproduce the above copyright | |||||||||
| * notice, this list of conditions and the following disclaimer in the | |||||||||
| * documentation and/or other materials provided with the distribution. | |||||||||
| * | |||||||||
| * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||||||
| * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||||||
| * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||||||
| * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||||||
| * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||||||
| * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||||||
| * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||||||
Done Inline ActionsThis block should become ifdef INET You'll need to include "opt_inet.h" and "opt_inet6.h" at the beginning for this. bz: This block should become ifdef INET
The block below equally ifdef INET6
You'll need to include… | |||||||||
Done Inline Actions#include <sys/cdefs.h> #include "opt_inet.h" I added the following lines, but it gave an error during compilation - opt_inet.h file not found, similarly for opt_inet6.h shivank: #include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include "opt_inet.h"
#include "opt_inet6.h"
I… | |||||||||
| * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||||||
| * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||||||
| * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||||||
| * SUCH DAMAGE. | |||||||||
| * | |||||||||
| * $FreeBSD$ | |||||||||
| */ | |||||||||
| /* | |||||||||
| * The IP address access control policy module - mac_ipacl allows the root of | |||||||||
| * the host to limit the VNET jail's privileges of setting IPv4 and IPv6 | |||||||||
| * addresses via sysctl(8) interface. So, the host can define rules for jails | |||||||||
| * and their interfaces about IP addresses. | |||||||||
| * sysctl(8) is to be used to modify the rules string in following format- | |||||||||
Done Inline Actionsifdef INET and below ifdef INET6 bz: ifdef INET
and below
ifdef INET6 | |||||||||
| * "jail_id,allow,interface,address_family,IP_addr/prefix_length[@jail_id,...]" | |||||||||
| */ | |||||||||
| #include "opt_inet.h" | |||||||||
| #include "opt_inet6.h" | |||||||||
| #include <sys/param.h> | |||||||||
| #include <sys/module.h> | |||||||||
| #include <sys/errno.h> | |||||||||
| #include <sys/kernel.h> | |||||||||
| #include <sys/mutex.h> | |||||||||
| #include <sys/priv.h> | |||||||||
| #include <sys/queue.h> | |||||||||
| #include <sys/socket.h> | |||||||||
| #include <sys/sysctl.h> | |||||||||
| #include <sys/systm.h> | |||||||||
| #include <sys/types.h> | |||||||||
| #include <sys/ucred.h> | |||||||||
| #include <sys/jail.h> | |||||||||
| #include <net/if.h> | |||||||||
| #include <net/if_var.h> | |||||||||
Done Inline ActionsIt would be nice if comments were aligned and not exceeding 80 chars per line. Some of them are self-explanatory so may not be needed at all? bz: It would be nice if comments were aligned and not exceeding 80 chars per line. Some of them… | |||||||||
| #include <netinet/in.h> | |||||||||
| #include <netinet6/scope6_var.h> | |||||||||
| #include <security/mac/mac_policy.h> | |||||||||
| SYSCTL_DECL(_security_mac); | |||||||||
| static SYSCTL_NODE(_security_mac, OID_AUTO, ipacl, CTLFLAG_RW | CTLFLAG_MPSAFE, 0, | |||||||||
| "TrustedBSD mac_ipacl policy controls"); | |||||||||
Done Inline ActionsI find "toast", while I perfectly understand it, doesn't match the naming from my networking namespace. "destroy_rules"? "delete_rules"? bz: I find "toast", while I perfectly understand it, doesn't match the naming from my networking… | |||||||||
| #ifdef INET | |||||||||
| static int ipacl_ipv4 = 1; | |||||||||
| SYSCTL_INT(_security_mac_ipacl, OID_AUTO, ipv4, CTLFLAG_RWTUN, | |||||||||
| &ipacl_ipv4, 0, "Enforce mac_ipacl for IPv4 addresses"); | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| static int ipacl_ipv6 = 1; | |||||||||
| SYSCTL_INT(_security_mac_ipacl, OID_AUTO, ipv6, CTLFLAG_RWTUN, | |||||||||
| &ipacl_ipv6, 0, "Enforce mac_ipacl for IPv6 addresses"); | |||||||||
| #endif | |||||||||
Done Inline ActionsInsert empty line before mtx_init. bz: Insert empty line before mtx_init. | |||||||||
| static MALLOC_DEFINE(M_IPACL, "ipacl_rule", "Rules for mac_ipacl"); | |||||||||
| #define MAC_RULE_STRING_LEN 1024 | |||||||||
| struct ipacl_addr { | |||||||||
| union { | |||||||||
| #ifdef INET | |||||||||
Done Inline ActionsAgain empty line before mtx_* bz: Again empty line before mtx_* | |||||||||
| struct in_addr ipv4; | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| struct in6_addr ipv6; | |||||||||
| #endif | |||||||||
| u_int8_t addr8[16]; | |||||||||
| u_int16_t addr16[8]; | |||||||||
Done Inline ActionsRemove blank line. bz: Remove blank line. | |||||||||
| u_int32_t addr32[4]; | |||||||||
| } ipa; /* 128 bit address*/ | |||||||||
| #ifdef INET | |||||||||
| #define v4 ipa.ipv4 | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| #define v6 ipa.ipv6 | |||||||||
| #endif | |||||||||
| #define addr8 ipa.addr8 | |||||||||
| #define addr16 ipa.addr16 | |||||||||
Done Inline ActionsIn general: after some blocks below you have an empty line, after some you don't. Being consistent would be good. bz: In general: after some blocks below you have an empty line, after some you don't. Being… | |||||||||
| #define addr32 ipa.addr32 | |||||||||
| }; | |||||||||
| struct ip_rule { | |||||||||
| int jid; | |||||||||
Done Inline ActionsInteresting comment. bz: Interesting comment.
What do you want to do about it?
Should it stay, then maybe put it above… | |||||||||
| bool allow; | |||||||||
| bool subnet_apply; /* Apply rule on whole subnet. */ | |||||||||
| char if_name[IFNAMSIZ]; | |||||||||
| int af; /* Address family. */ | |||||||||
| struct ipacl_addr addr; | |||||||||
| struct ipacl_addr mask; | |||||||||
| TAILQ_ENTRY(ip_rule) r_entries; | |||||||||
| }; | |||||||||
| static struct mtx rule_mtx; | |||||||||
| static TAILQ_HEAD(rulehead, ip_rule) rule_head; | |||||||||
Done Inline ActionsSpaces around the = bz: Spaces around the = | |||||||||
| static char rule_string[MAC_RULE_STRING_LEN]; | |||||||||
| static void | |||||||||
| destroy_rules(struct rulehead *head) | |||||||||
| { | |||||||||
| struct ip_rule *rule; | |||||||||
Done Inline ActionsI think FreeBSD generally uses IFNAMSIZ and not IF_NAMESIZE for native code. bz: I think FreeBSD generally uses IFNAMSIZ and not IF_NAMESIZE for native code. | |||||||||
| while ((rule = TAILQ_FIRST(head)) != NULL) { | |||||||||
| TAILQ_REMOVE(head, rule, r_entries); | |||||||||
| free(rule, M_IPACL); | |||||||||
| } | |||||||||
| } | |||||||||
| static void | |||||||||
| ipacl_init(struct mac_policy_conf *conf) | |||||||||
| { | |||||||||
| mtx_init(&rule_mtx, "rule_mtx", NULL, MTX_DEF); | |||||||||
| TAILQ_INIT(&rule_head); | |||||||||
| } | |||||||||
Done Inline ActionsGetting the ifdef INET / ifdef INET6 right for this might be harder... Using if/else or switch might be easier. Something like this block here: https://svnweb.freebsd.org/base/head/sys/netinet/tcp_input.c?annotate=347407#l709 bz: Getting the ifdef INET / ifdef INET6 right for this might be harder... Using if/else or switch… | |||||||||
| static void | |||||||||
| ipacl_destroy(struct mac_policy_conf *conf) | |||||||||
| { | |||||||||
| mtx_destroy(&rule_mtx); | |||||||||
| destroy_rules(&rule_head); | |||||||||
| } | |||||||||
Done Inline ActionsNit: no blank line needed per the style(9) rules. melifaro: Nit: no blank line needed per the style(9) rules. | |||||||||
| /* | |||||||||
| * Note: parsing routines are destructive on the passed string. | |||||||||
| */ | |||||||||
| static int | |||||||||
| parse_rule_element(char *element, struct ip_rule *rule) | |||||||||
| { | |||||||||
| char *tok, *p; | |||||||||
Done Inline ActionsThe only error you're returning here is EINVAL. melifaro: The only error you're returning here is `EINVAL`.
Maybe it's worth returning `struct ip_rule`… | |||||||||
| int prefix; | |||||||||
| #ifdef INET6 | |||||||||
| int i; | |||||||||
| #endif | |||||||||
| /* Should we support a jail wildcard? */ | |||||||||
| tok = strsep(&element, ","); | |||||||||
| if (tok == NULL) | |||||||||
| return (EINVAL); | |||||||||
| rule->jid = strtol(tok, &p, 10); | |||||||||
| if (*p != '\0') | |||||||||
| return (EINVAL); | |||||||||
| tok = strsep(&element, ","); | |||||||||
Done Inline ActionsYou want to use true/false here for the bool. bz: You want to use true/false here for the bool. | |||||||||
| if (tok == NULL) | |||||||||
| return (EINVAL); | |||||||||
| rule->allow = strtol(tok, &p, 10); | |||||||||
Done Inline Actionsifdef INET; maybe make it a switch block? bz: ifdef INET; maybe make it a switch block? | |||||||||
| if (*p != '\0') | |||||||||
| return (EINVAL); | |||||||||
| tok = strsep(&element, ","); | |||||||||
| if (strlen(tok) + 1 > IFNAMSIZ) | |||||||||
| return (EINVAL); | |||||||||
| /* Empty interface name is wildcard to all interfaces. */ | |||||||||
| strlcpy(rule->if_name, tok, strlen(tok) + 1); | |||||||||
Done Inline Actionsextra blank line, and two lines below is a bit long; try to wrap at 80 characters. bz: extra blank line, and two lines below is a bit long; try to wrap at 80 characters. | |||||||||
Done Inline Actionsdo you actually need goto out here and in the next invocations? melifaro: do you actually need `goto out` here and in the next invocations?
The code mostly uses local… | |||||||||
| tok = strsep(&element, ","); | |||||||||
| if (tok == NULL) | |||||||||
| return (EINVAL); | |||||||||
| rule->af = (strcmp(tok, "AF_INET") == 0) ? AF_INET : | |||||||||
Done Inline ActionsFold else up to same line? However with ifdef INET / INET6 this will change anyway ifdef INET6 bz: Fold else up to same line? However with ifdef INET / INET6 this will change anyway
ifdef… | |||||||||
| (strcmp(tok, "AF_INET6") == 0) ? AF_INET6 : -1; | |||||||||
| if (rule->af == -1) | |||||||||
| return (EINVAL); | |||||||||
| tok = strsep(&element, "/"); | |||||||||
Done Inline Actionssizeof() is a compile-only check in C. melifaro: sizeof() is a compile-only check in C. | |||||||||
| if (tok == NULL) | |||||||||
| return (EINVAL); | |||||||||
| if (inet_pton(rule->af, tok, rule->addr.addr32) != 1) | |||||||||
Done Inline ActionsThe cast to unsigned long into an 8bit field doesn't look right. bz: The cast to unsigned long into an 8bit field doesn't look right. | |||||||||
| return (EINVAL); | |||||||||
| tok = element; | |||||||||
Done Inline ActionsNit: memset & strlcpy. Also, new is already allocated zeroed. melifaro: Nit: memset & strlcpy. Also, `new` is already allocated zeroed. | |||||||||
| if (tok == NULL) | |||||||||
| return (EINVAL); | |||||||||
| prefix = strtol(tok, &p, 10); | |||||||||
| if (*p != '\0') | |||||||||
| return (EINVAL); | |||||||||
| /* Value -1 for prefix make policy applicable to individual IP only. */ | |||||||||
| if (prefix == -1) | |||||||||
| rule->subnet_apply = false; | |||||||||
| else { | |||||||||
| rule->subnet_apply = true; | |||||||||
| switch (rule->af) { | |||||||||
| #ifdef INET | |||||||||
| case AF_INET: | |||||||||
Done Inline Actionsplease use the documentation prefix, e.g., 192.0.2.2 and 2001:db8:: ; maybe use a 32 prefix length for IPv6 as /8 is a bit unrealistic; maybe even /56 or /64 bz: please use the documentation prefix, e.g., 192.0.2.2 and 2001:db8:: ; maybe use a 32 prefix… | |||||||||
| if (prefix < 0 || prefix > 32) | |||||||||
| return (EINVAL); | |||||||||
Done Inline Actionsremove blank line. bz: remove blank line. | |||||||||
| if (prefix == 0) | |||||||||
| rule->mask.addr32[0] = htonl(0); | |||||||||
| else | |||||||||
| rule->mask.addr32[0] = | |||||||||
| htonl(~((1 << (32 - prefix)) - 1)); | |||||||||
| rule->addr.addr32[0] &= rule->mask.addr32[0]; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| case AF_INET6: | |||||||||
| if (prefix < 0 || prefix > 128) | |||||||||
| return (EINVAL); | |||||||||
| for (i = 0; prefix > 0; prefix -= 8, i++) | |||||||||
| rule->mask.addr8[i] = prefix >= 8 ? 0xFF : | |||||||||
| (u_int8_t)((0xFFU << (8 - prefix)) & 0xFFU); | |||||||||
| for (i = 0; i < 16; i++) | |||||||||
| rule->addr.addr8[i] &= rule->mask.addr8[i]; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| } | |||||||||
| } | |||||||||
| return (0); | |||||||||
| } | |||||||||
| /* | |||||||||
| * Format of Rule- jid,allow,interface_name,addr_family,ip_addr/subnet_mask | |||||||||
| * Example: sysctl security.mac.ipacl.rules=1,1,epair0b,AF_INET,192.0.2.2/24 | |||||||||
| */ | |||||||||
| static int | |||||||||
| parse_rules(char *string, struct rulehead *head) | |||||||||
| { | |||||||||
| struct ip_rule *new; | |||||||||
| char *element; | |||||||||
| int error; | |||||||||
| error = 0; | |||||||||
| while ((element = strsep(&string, "@")) != NULL) { | |||||||||
| if (strlen(element) == 0) | |||||||||
| continue; | |||||||||
| new = malloc(sizeof(*new), M_IPACL, M_ZERO | M_WAITOK); | |||||||||
| error = parse_rule_element(element, new); | |||||||||
| if (error != 0) { | |||||||||
| free(new, M_IPACL); | |||||||||
| goto out; | |||||||||
| } | |||||||||
| TAILQ_INSERT_TAIL(head, new, r_entries); | |||||||||
| } | |||||||||
| out: | |||||||||
| if (error != 0) | |||||||||
| destroy_rules(head); | |||||||||
| return (error); | |||||||||
Done Inline ActionsOh, I see how this is done; interesting :-) bz: Oh, I see how this is done; interesting :-) | |||||||||
Done Inline ActionsActually, it's implemented like this in from mac_portacl.c, I copied it from there shivank: Actually, it's implemented like this in from mac_portacl.c, I copied it from there | |||||||||
| } | |||||||||
| static int | |||||||||
| sysctl_rules(SYSCTL_HANDLER_ARGS) | |||||||||
| { | |||||||||
| char *string, *copy_string, *new_string; | |||||||||
| struct rulehead head, save_head; | |||||||||
| int error; | |||||||||
| new_string = NULL; | |||||||||
| if (req->newptr != NULL) { | |||||||||
| new_string = malloc(MAC_RULE_STRING_LEN, M_IPACL, | |||||||||
Done Inline ActionsRemove blank line. bz: Remove blank line. | |||||||||
| M_WAITOK | M_ZERO); | |||||||||
| mtx_lock(&rule_mtx); | |||||||||
| strcpy(new_string, rule_string); | |||||||||
| mtx_unlock(&rule_mtx); | |||||||||
| string = new_string; | |||||||||
| } else | |||||||||
| string = rule_string; | |||||||||
Done Inline Actionscould be a void function? bz: could be a void function? | |||||||||
| error = sysctl_handle_string(oidp, string, MAC_RULE_STRING_LEN, req); | |||||||||
| if (error) | |||||||||
Done Inline ActionsTo the previous comment on error handling & goto - you can alternatively allocate the rule here. melifaro: To the previous comment on error handling & goto - you can alternatively allocate the rule here. | |||||||||
| goto out; | |||||||||
Done Inline Actionsifdef INET / IENT6; please use defined constants here (INET_ADDRSTRLEN, INET6_ADDRSTRLEN) bz: ifdef INET / IENT6; please use defined constants here (INET_ADDRSTRLEN, INET6_ADDRSTRLEN) | |||||||||
Done Inline Actions
melifaro: | |||||||||
| if (req->newptr != NULL) { | |||||||||
| copy_string = strdup(string, M_IPACL); | |||||||||
| TAILQ_INIT(&head); | |||||||||
| error = parse_rules(copy_string, &head); | |||||||||
| free(copy_string, M_IPACL); | |||||||||
Done Inline ActionsThis and some of the following line look long in the review system; please make sure they are wrapped at <= 80 chars bz: This and some of the following line look long in the review system; please make sure they are… | |||||||||
| if (error) | |||||||||
Done Inline Actionsifdef INET; again switch might make this easier. bz: ifdef INET; again switch might make this easier. | |||||||||
| goto out; | |||||||||
| TAILQ_INIT(&save_head); | |||||||||
| mtx_lock(&rule_mtx); | |||||||||
Done Inline ActionsFrankly speaking, I have mixed feeling about these. melifaro: Frankly speaking, I have mixed feeling about these.
We do a lot of string argument parsing &… | |||||||||
Done Inline ActionsI agree with your suggestion. using sysctl makes it very limited. will implement it as future enhancement. shivank: I agree with your suggestion. using sysctl makes it very limited. will implement it as future… | |||||||||
| TAILQ_CONCAT(&save_head, &rule_head, r_entries); | |||||||||
| TAILQ_CONCAT(&rule_head, &head, r_entries); | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| strcpy(rule_string, string); | |||||||||
| mtx_unlock(&rule_mtx); | |||||||||
| destroy_rules(&save_head); | |||||||||
| } | |||||||||
| out: | |||||||||
| if (new_string != NULL) | |||||||||
| free(new_string, M_IPACL); | |||||||||
| return (error); | |||||||||
| } | |||||||||
| SYSCTL_PROC(_security_mac_ipacl, OID_AUTO, rules, | |||||||||
| CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_MPSAFE, 0, | |||||||||
| 0, sysctl_rules, "A", "IP ACL Rules"); | |||||||||
Done Inline Actions4 spaces indent bz: 4 spaces indent | |||||||||
| static int | |||||||||
| rules_check(struct ucred *cred, | |||||||||
| struct ipacl_addr *ip_addr, struct ifnet *ifp) | |||||||||
| { | |||||||||
| struct ip_rule *rule; | |||||||||
| int error; | |||||||||
| #ifdef INET6 | |||||||||
| int i; | |||||||||
| bool same_subnet; | |||||||||
| #endif | |||||||||
| error = EPERM; | |||||||||
| mtx_lock(&rule_mtx); | |||||||||
Done Inline Actionsif (... (add space; be consistent within your code as well) bz: if (... (add space; be consistent within your code as well) | |||||||||
| /* | |||||||||
| * In the case where multiple rules are applicable to an IP address or | |||||||||
Done Inline ActionsPossibly this and a few lines below look rather long in the review system; please try to wrap them at 80 char bz: Possibly this and a few lines below look rather long in the review system; please try to wrap… | |||||||||
| * a set of IP addresses, the rule that is defined later in the list | |||||||||
| * determines the outcome, disregarding any previous rule for that IP | |||||||||
| * address. | |||||||||
| * Walk the policy rules list in reverse order until rule applicable | |||||||||
Done Inline Actionsifdef INET bz: ifdef INET | |||||||||
| * to the requested IP address is found. | |||||||||
| */ | |||||||||
Done Inline ActionsQuestion: why do you do this maths for every check; rather than once when you setup the rules? bz: Question: why do you do this maths for every check; rather than once when you setup the rules? | |||||||||
| TAILQ_FOREACH_REVERSE(rule, &rule_head, rulehead, r_entries) { | |||||||||
| /* Skip if current rule applies to different jail. */ | |||||||||
| if (cred->cr_prison->pr_id != rule->jid) | |||||||||
| continue; | |||||||||
Done Inline Actions} else { in one line; here and everywhere else bz: } else { in one line; here and everywhere else | |||||||||
| if (strcmp(rule->if_name, "\0") && | |||||||||
Done Inline Actionsindent bz: indent | |||||||||
| strcmp(rule->if_name, ifp->if_xname)) | |||||||||
| continue; | |||||||||
| switch (rule->af) { | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| #ifdef INET | |||||||||
| case AF_INET: | |||||||||
Done Inline ActionsI assume this will move elsewhere; for (i = 0; i < 16; i++)\n ; no {} block needed bz: I assume this will move elsewhere; for (i = 0; i < 16; i++)\n ; no {} block needed | |||||||||
| if (rule->subnet_apply) { | |||||||||
| if (rule->addr.v4.s_addr != | |||||||||
| (ip_addr->v4.s_addr & rule->mask.v4.s_addr)) | |||||||||
| continue; | |||||||||
| } else | |||||||||
| if (ip_addr->v4.s_addr != rule->addr.v4.s_addr) | |||||||||
Done Inline Actionscould you just continue immediately not needing "j"? Hmm within the loop; I see. Maybe make j a bool and call it something more descriptive? Possibly also inverting the true/false logic depending on new name? bz: could you just continue immediately not needing "j"? Hmm within the loop; I see. Maybe make j… | |||||||||
| continue; | |||||||||
| break; | |||||||||
| #endif | |||||||||
Done Inline Actionsif (.. (add space) bz: if (.. (add space) | |||||||||
| #ifdef INET6 | |||||||||
| case AF_INET6: | |||||||||
| if (rule->subnet_apply) { | |||||||||
| same_subnet = true; | |||||||||
| for (i = 0; i < 16; i++) | |||||||||
Done Inline ActionsI think the comment can go? bz: I think the comment can go? | |||||||||
| if (rule->addr.v6.s6_addr[i] != | |||||||||
| (ip_addr->v6.s6_addr[i] & | |||||||||
| rule->mask.v6.s6_addr[i])) { | |||||||||
| same_subnet = false; | |||||||||
| break; | |||||||||
| } | |||||||||
| if (!same_subnet) | |||||||||
| continue; | |||||||||
| } else | |||||||||
| if (bcmp(&rule->addr, ip_addr, | |||||||||
| sizeof(*ip_addr))) | |||||||||
| continue; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| } | |||||||||
| if (rule->allow) | |||||||||
| error = 0; | |||||||||
| break; | |||||||||
Done Inline Actionsifdef INET bz: ifdef INET | |||||||||
| } | |||||||||
| mtx_unlock(&rule_mtx); | |||||||||
| return (error); | |||||||||
Done Inline ActionsBlank line between variable declaration and code bz: Blank line between variable declaration and code | |||||||||
| } | |||||||||
Done Inline ActionsFeature request (also equally for IPv6) for once this is all sorted if there is more time. Can we make this a sysctl policy as well defaulting to jails only, but if changed also applying to the base system? bz: Feature request (also equally for IPv6) for once this is all sorted if there is more time. | |||||||||
Done Inline ActionsYeah. Can I add a comment in the code regarding this? shivank: Yeah. Can I add a comment in the code regarding this? | |||||||||
| /* | |||||||||
| * Feature request: Can we make this sysctl policy apply to jails by default, | |||||||||
| * but also allow it to be changed to apply to the base system? | |||||||||
| */ | |||||||||
| #ifdef INET | |||||||||
| static int | |||||||||
| ipacl_ip4_check_jail(struct ucred *cred, | |||||||||
| const struct in_addr *ia, struct ifnet *ifp) | |||||||||
| { | |||||||||
| struct ipacl_addr ip4_addr; | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| ip4_addr.v4 = *ia; | |||||||||
| if (!jailed(cred)) | |||||||||
| return (0); | |||||||||
Done Inline Actionsblank line again bz: blank line again | |||||||||
| /* Checks with the policy only when it is enforced for ipv4. */ | |||||||||
| if (ipacl_ipv4) | |||||||||
| return rules_check(cred, &ip4_addr, ifp); | |||||||||
| return (0); | |||||||||
| } | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| static int | |||||||||
| ipacl_ip6_check_jail(struct ucred *cred, | |||||||||
| const struct in6_addr *ia6, struct ifnet *ifp) | |||||||||
| { | |||||||||
| struct ipacl_addr ip6_addr; | |||||||||
| ip6_addr.v6 = *ia6; /* Make copy to not alter the original. */ | |||||||||
| in6_clearscope(&ip6_addr.v6); /* Clear the scope id. */ | |||||||||
Done Inline Actionsifdef INET bz: ifdef INET | |||||||||
Done Inline ActionsDo I also need to add #ifdef ... #endif in the corresponding declaration in the framework? shivank: Do I also need to add #ifdef ... #endif in the corresponding declaration in the framework?
Same… | |||||||||
| if (!jailed(cred)) | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| return (0); | |||||||||
| /* Checks with the policy when it is enforced for ipv6. */ | |||||||||
| if (ipacl_ipv6) | |||||||||
| return rules_check(cred, &ip6_addr, ifp); | |||||||||
| return (0); | |||||||||
| } | |||||||||
| #endif | |||||||||
| static struct mac_policy_ops ipacl_ops = | |||||||||
| { | |||||||||
| .mpo_init = ipacl_init, | |||||||||
| .mpo_destroy = ipacl_destroy, | |||||||||
| #ifdef INET | |||||||||
| .mpo_ip4_check_jail = ipacl_ip4_check_jail, | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| .mpo_ip6_check_jail = ipacl_ip6_check_jail, | |||||||||
| #endif | |||||||||
| }; | |||||||||
| MAC_POLICY_SET(&ipacl_ops, mac_ipacl, "TrustedBSD MAC/ipacl", | |||||||||
| MPC_LOADTIME_FLAG_UNLOADOK, NULL); | |||||||||
Please remove me everywhere from the (c)