Changeset View
Standalone View
sys/security/mac_ipacl/mac_ipacl.c
- This file was added.
| /*- | |||||||||
| * SPDX-License-Identifier: BSD-2-Clause | |||||||||
| * | |||||||||
| * Copyright (c) 2019 Shivank Garg <shivank@FreeBSD.org> | |||||||||
| * Copyright (c) 2019 Bjoern A. Zeeb <bz@FreeBSD.org> | |||||||||
bz: Please remove me everywhere from the (c) | |||||||||
| * | |||||||||
| * All rights reserved. | |||||||||
| * This code was developed as a Google Summer of Code 2019 project | |||||||||
| * under the guidance of Mr. Bjoern A. Zeeb. | |||||||||
| * | |||||||||
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… | |||||||||
| * 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) | |||||||||
| * 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$ | |||||||||
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… | |||||||||
| */ | |||||||||
| /* | |||||||||
| * 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- | |||||||||
| * "jail_id@allow@interface@address_family@IP_addr@prefix_length[,jail_id@...]" | |||||||||
| */ | |||||||||
| #include "opt_inet.h" | |||||||||
| #include "opt_inet6.h" | |||||||||
Done Inline Actionsifdef INET and below ifdef INET6 bz: ifdef INET
and below
ifdef INET6 | |||||||||
| #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> | |||||||||
| #include <netinet/in.h> | |||||||||
| #include <netinet6/scope6_var.h> | |||||||||
| #include <security/mac/mac_policy.h> | |||||||||
| SYSCTL_DECL(_security_mac); | |||||||||
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… | |||||||||
| static SYSCTL_NODE(_security_mac, OID_AUTO, ipacl, CTLFLAG_RW, 0, | |||||||||
| "TrustedBSD mac_ipacl policy controls"); | |||||||||
| #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 | |||||||||
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 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 | |||||||||
| static MALLOC_DEFINE(M_IPACL, "ipacl_rule", "Rules for mac_ipacl"); | |||||||||
| #define MAC_RULE_STRING_LEN 1024 | |||||||||
| struct ipacl_addr { | |||||||||
| union { | |||||||||
Done Inline ActionsInsert empty line before mtx_init. bz: Insert empty line before mtx_init. | |||||||||
| #ifdef INET | |||||||||
| struct in_addr ipv4; | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| struct in6_addr ipv6; | |||||||||
| #endif | |||||||||
| u_int8_t addr8[16]; | |||||||||
Done Inline ActionsAgain empty line before mtx_* bz: Again empty line before mtx_* | |||||||||
| u_int16_t addr16[8]; | |||||||||
| u_int32_t addr32[4]; | |||||||||
| } ipa; /* 128 bit address*/ | |||||||||
| #ifdef INET | |||||||||
| #define v4 ipa.ipv4 | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
Done Inline ActionsRemove blank line. bz: Remove blank line. | |||||||||
| #define v6 ipa.ipv6 | |||||||||
| #endif | |||||||||
| #define addr8 ipa.addr8 | |||||||||
| #define addr16 ipa.addr16 | |||||||||
| #define addr32 ipa.addr32 | |||||||||
| }; | |||||||||
| struct ip_rule { | |||||||||
| int jid; | |||||||||
| bool allow; | |||||||||
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… | |||||||||
| bool subnet_apply; /*rule applied on whole subnet*/ | |||||||||
| char if_name[IFNAMSIZ]; /*network interface name*/ | |||||||||
| int af; /*address family*/ | |||||||||
| struct ipacl_addr addr; | |||||||||
| struct ipacl_addr mask; | |||||||||
Done Inline ActionsInteresting comment. bz: Interesting comment.
What do you want to do about it?
Should it stay, then maybe put it above… | |||||||||
| TAILQ_ENTRY(ip_rule) r_entries; | |||||||||
| }; | |||||||||
| static struct mtx rule_mtx; | |||||||||
| static TAILQ_HEAD(rulehead, ip_rule) rule_head; | |||||||||
| static char rule_string[MAC_RULE_STRING_LEN]; | |||||||||
| static void | |||||||||
| destroy_rules(struct rulehead *head) | |||||||||
| { | |||||||||
| struct ip_rule *rule; | |||||||||
Done Inline ActionsSpaces around the = bz: Spaces around the = | |||||||||
| while ((rule = TAILQ_FIRST(head)) != NULL) { | |||||||||
| TAILQ_REMOVE(head, rule, r_entries); | |||||||||
| free(rule, M_IPACL); | |||||||||
| } | |||||||||
| } | |||||||||
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. | |||||||||
| static void | |||||||||
| ipacl_init(struct mac_policy_conf *conf) | |||||||||
| { | |||||||||
| mtx_init(&rule_mtx, "rule_mtx", NULL, MTX_DEF); | |||||||||
| TAILQ_INIT(&rule_head); | |||||||||
| } | |||||||||
| static void | |||||||||
| ipacl_destroy(struct mac_policy_conf *conf) | |||||||||
| { | |||||||||
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… | |||||||||
| mtx_destroy(&rule_mtx); | |||||||||
| destroy_rules(&rule_head); | |||||||||
| } | |||||||||
| /* | |||||||||
| * Note: parsing routines are destructive on the passed string. | |||||||||
| */ | |||||||||
Done Inline ActionsNit: no blank line needed per the style(9) rules. melifaro: Nit: no blank line needed per the style(9) rules. | |||||||||
| static int | |||||||||
| parse_rule_element(char *element, struct ip_rule **rule) | |||||||||
| { | |||||||||
| char *jid, *allow, *if_name, *fam, *ip_addr, *mask, *p; | |||||||||
| struct ip_rule *new; | |||||||||
| int error, prefix, i; | |||||||||
| error = 0; | |||||||||
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`… | |||||||||
| new = malloc(sizeof(*new), M_IPACL, M_ZERO | M_WAITOK); | |||||||||
| /* Should we support a jail wildcard? */ | |||||||||
| jid = strsep(&element, "@"); | |||||||||
| if (jid == NULL) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| new->jid = strtol(jid, &p, 10); | |||||||||
| if (*p != '\0') { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| allow = 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 (allow == NULL) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
Done Inline Actionsifdef INET; maybe make it a switch block? bz: ifdef INET; maybe make it a switch block? | |||||||||
| } | |||||||||
| new->allow = strtol(allow, &p, 10); | |||||||||
| if (*p != '\0') { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| if_name = strsep(&element, "@"); | |||||||||
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… | |||||||||
| if (sizeof(if_name) > IFNAMSIZ) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
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… | |||||||||
| /* Empty interface name is wildcard to all interfaces.*/ | |||||||||
| bzero(new->if_name, IFNAMSIZ); | |||||||||
| bcopy(if_name, new->if_name, strlen(if_name)); | |||||||||
| fam = strsep(&element, "@"); | |||||||||
Done Inline Actionssizeof() is a compile-only check in C. melifaro: sizeof() is a compile-only check in C. | |||||||||
| if (fam == NULL) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
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. | |||||||||
| } | |||||||||
| new->af = (strcmp(fam, "AF_INET") == 0) ? AF_INET : | |||||||||
Done Inline ActionsNit: memset & strlcpy. Also, new is already allocated zeroed. melifaro: Nit: memset & strlcpy. Also, `new` is already allocated zeroed. | |||||||||
| (strcmp(fam, "AF_INET6") == 0) ? AF_INET6 : -1; | |||||||||
| if (new->af == -1) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| ip_addr = strsep(&element, "@"); | |||||||||
| if (ip_addr == NULL) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| if (inet_pton(new->af, ip_addr, new->addr.addr32) != 1) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
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… | |||||||||
| } | |||||||||
| mask = element; | |||||||||
Done Inline Actionsremove blank line. bz: remove blank line. | |||||||||
| if (mask == NULL) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| prefix = strtol(mask, &p, 10); | |||||||||
| if (*p != '\0') { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| /*Value -1 for prefix make policy applicable to individual IP only.*/ | |||||||||
| if (prefix == -1) | |||||||||
| new->subnet_apply = false; | |||||||||
| else { | |||||||||
| new->subnet_apply = true; | |||||||||
| switch (new->af) { | |||||||||
| #ifdef INET | |||||||||
| case AF_INET: | |||||||||
| if (prefix < 0 || prefix > 32) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| if (prefix == 0) | |||||||||
| new->mask.addr32[0] = htonl(0); | |||||||||
| else | |||||||||
| new->mask.addr32[0] = | |||||||||
| htonl(~((1 << (32 - prefix)) - 1)); | |||||||||
| new->addr.addr32[0] &= new->mask.addr32[0]; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| case AF_INET6: | |||||||||
| if (prefix < 0 || prefix > 128) { | |||||||||
| error = EINVAL; | |||||||||
| goto out; | |||||||||
| } | |||||||||
| for (i = 0; prefix > 0; prefix -= 8, i++) | |||||||||
| new->mask.addr8[i] = | |||||||||
| prefix >= 8 ? 0xFF : (u_int8_t) | |||||||||
| ((0xFFU << (8 - prefix)) & 0xFFU); | |||||||||
| for (i=0; i<16; i++) | |||||||||
| new->addr.addr8[i] | |||||||||
| &= new->mask.addr8[i]; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| } | |||||||||
| } | |||||||||
| out: | |||||||||
| if (error != 0) { | |||||||||
| free(new, M_IPACL); | |||||||||
| *rule = NULL; | |||||||||
| } | |||||||||
| else | |||||||||
| *rule = new; | |||||||||
| 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 | |||||||||
| } | |||||||||
| /* | |||||||||
| * 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; | |||||||||
Done Inline ActionsRemove blank line. bz: Remove blank line. | |||||||||
| error = 0; | |||||||||
| while ((element = strsep(&string, ",")) != NULL) { | |||||||||
| if (strlen(element) == 0) | |||||||||
| continue; | |||||||||
| error = parse_rule_element(element, &new); | |||||||||
| if (error) | |||||||||
| goto out; | |||||||||
Done Inline Actionscould be a void function? bz: could be a void function? | |||||||||
| TAILQ_INSERT_TAIL(head, new, r_entries); | |||||||||
| } | |||||||||
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. | |||||||||
| 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 (error != 0) | |||||||||
| destroy_rules(head); | |||||||||
| return (error); | |||||||||
| } | |||||||||
| static int | |||||||||
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… | |||||||||
| sysctl_rules(SYSCTL_HANDLER_ARGS) | |||||||||
Done Inline Actionsifdef INET; again switch might make this easier. bz: ifdef INET; again switch might make this easier. | |||||||||
| { | |||||||||
| char *string, *copy_string, *new_string; | |||||||||
| struct rulehead head, save_head; | |||||||||
| int error; | |||||||||
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… | |||||||||
| new_string = NULL; | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| if (req->newptr != NULL) { | |||||||||
| new_string = malloc(MAC_RULE_STRING_LEN, M_IPACL, | |||||||||
| M_WAITOK | M_ZERO); | |||||||||
| mtx_lock(&rule_mtx); | |||||||||
| strcpy(new_string, rule_string); | |||||||||
| mtx_unlock(&rule_mtx); | |||||||||
| string = new_string; | |||||||||
| } else | |||||||||
| string = rule_string; | |||||||||
| error = sysctl_handle_string(oidp, string, MAC_RULE_STRING_LEN, req); | |||||||||
| if (error) | |||||||||
| goto out; | |||||||||
Done Inline Actions4 spaces indent bz: 4 spaces indent | |||||||||
| if (req->newptr != NULL) { | |||||||||
| copy_string = strdup(string, M_IPACL); | |||||||||
| TAILQ_INIT(&head); | |||||||||
| error = parse_rules(copy_string, &head); | |||||||||
| free(copy_string, M_IPACL); | |||||||||
| if (error) | |||||||||
| goto out; | |||||||||
| TAILQ_INIT(&save_head); | |||||||||
| mtx_lock(&rule_mtx); | |||||||||
| TAILQ_CONCAT(&save_head, &rule_head, r_entries); | |||||||||
| TAILQ_CONCAT(&rule_head, &head, r_entries); | |||||||||
| strcpy(rule_string, string); | |||||||||
| mtx_unlock(&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) | |||||||||
| destroy_rules(&save_head); | |||||||||
| } | |||||||||
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… | |||||||||
| out: | |||||||||
| if (new_string != NULL) | |||||||||
| free(new_string, M_IPACL); | |||||||||
| return (error); | |||||||||
Done Inline Actionsifdef INET bz: ifdef INET | |||||||||
| } | |||||||||
| SYSCTL_PROC(_security_mac_ipacl, OID_AUTO, rules, | |||||||||
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? | |||||||||
| CTLTYPE_STRING|CTLFLAG_RW, 0, 0, sysctl_rules, "A", "IP ACL Rules"); | |||||||||
| static int | |||||||||
| rules_check(struct ucred *cred, | |||||||||
Done Inline Actions} else { in one line; here and everywhere else bz: } else { in one line; here and everywhere else | |||||||||
| struct ipacl_addr *ip_addr, struct ifnet *ifp) | |||||||||
| { | |||||||||
Done Inline Actionsindent bz: indent | |||||||||
| struct ip_rule *rule; | |||||||||
| int error, i; | |||||||||
| bool same_subnet; | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| error = EPERM; | |||||||||
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 | |||||||||
| mtx_lock(&rule_mtx); | |||||||||
| for (rule = TAILQ_FIRST(&rule_head); | |||||||||
| rule != NULL; | |||||||||
| rule = TAILQ_NEXT(rule, r_entries)) { | |||||||||
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… | |||||||||
| /*Skip if current rule applies to different jail.*/ | |||||||||
| if (cred->cr_prison->pr_id != rule->jid) | |||||||||
| continue; | |||||||||
Done Inline Actionsif (.. (add space) bz: if (.. (add space) | |||||||||
| if (strcmp(rule->if_name, "\0") && | |||||||||
| strcmp(rule->if_name, ifp->if_xname)) | |||||||||
| continue; | |||||||||
Done Inline ActionsI think the comment can go? bz: I think the comment can go? | |||||||||
| switch (rule->af) { | |||||||||
| #ifdef INET | |||||||||
| case AF_INET: | |||||||||
| 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) | |||||||||
| continue; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| #ifdef INET6 | |||||||||
| case AF_INET6: | |||||||||
| if (rule->subnet_apply) { | |||||||||
| same_subnet=true; | |||||||||
Done Inline Actionsifdef INET bz: ifdef INET | |||||||||
| for ( i=0 ; i<16 ; i++ ) | |||||||||
| if (rule->addr.v6.s6_addr[i] != | |||||||||
| (ip_addr->v6.s6_addr[i] & | |||||||||
| rule->mask.v6.s6_addr[i])) { | |||||||||
| same_subnet=false; | |||||||||
Done Inline ActionsBlank line between variable declaration and code bz: Blank line between variable declaration and code | |||||||||
| break; | |||||||||
| } | |||||||||
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? | |||||||||
| if (!same_subnet) | |||||||||
| continue; | |||||||||
| } | |||||||||
| else | |||||||||
| if (bcmp(&rule->addr, ip_addr, | |||||||||
| sizeof(*ip_addr))) | |||||||||
| continue; | |||||||||
| break; | |||||||||
| #endif | |||||||||
| default:/*dead block, should I keep it?*/ | |||||||||
| error = EINVAL; | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| } | |||||||||
| if (rule->allow) | |||||||||
| error = 0; | |||||||||
| else | |||||||||
Done Inline Actionsblank line again bz: blank line again | |||||||||
| error = EPERM; | |||||||||
| } | |||||||||
| mtx_unlock(&rule_mtx); | |||||||||
| return (error); | |||||||||
| } | |||||||||
| /* Feature request: Can we make this a sysctl policy as well defaulting | |||||||||
| * to jails only, but if changed also applying 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 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… | |||||||||
| ip4_addr.v4 = *ia; | |||||||||
Done Inline Actionsifdef INET6 bz: ifdef INET6 | |||||||||
| /*function only when requested by a jail*/ | |||||||||
| if (!jailed(cred)) | |||||||||
| return 0; | |||||||||
| /*check 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 scope id*/ | |||||||||
| /*function only when requested by a jail*/ | |||||||||
| if (!jailed(cred)) | |||||||||
| return 0; | |||||||||
| /*check 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)