User id and group id integration in NPF #45

Emmankoko · 2025-03-17T10:35:12Z

we can set our uids or gids as numbers,(1001), names(Kojo), or a variable that holds a number of name($my_id)

pass all user 1000
pass all user Kojo
pass all user $myuser(containing Kojo or 1001)

same applies with group id.
NB: keyword for gid is usergroup
eg. pass all usergroup wheel(we cannot use group because group already exist)

we can set our uids or gids as numbers,(1001), names(Kojo), or a variable that holds a number of name($my_id) pass all user 1000 pass all user Kojo pass all user $myuser(containing Kojo or 1001) same applies with group id. NB: keyword for gid is usergroup eg. pass all usergroup wheel(we cannot use group because group already exist)

…tructure

… owner of that socket.

so we perform a comparison with the user on our rule and the user for the process involved, if it matches, proceed to block or pass. if it doesn't match, reveres from pass to block. if we pass on user Christos, it has to block on user Martin. iif we block on user Martin, it passes on user Christos. (pass->block)just rip of pass bit fields from return flag and insert block field (block->pass)same here. just rip block bit fields and insert pass field

zoulasc · 2025-03-17T13:20:56Z

usr.sbin/npf/npfctl/npf_parse.y

+uid_item
+	: uid
+	{
+		if (($$ = calloc(1, sizeof(struct r_uid))) == NULL)


write a method that takes 3 arguments instead of repeating the code.

zoulasc · 2025-03-17T13:23:53Z

usr.sbin/npf/npfctl/npf_data.c

+		struct passwd	*pw;
+
+		if ((pw = getpwnam(user)) == NULL) {
+			return 1;


Don't use 1 as an error, what is the difference between unknown and error?

zoulasc · 2025-03-17T13:24:06Z

usr.sbin/npf/npfctl/npf_data.c

+npfctl_parse_user(const char *user)
+{
+	uint32_t uid;
+	if (!(strcmp(user, "unknown")))


no parens around strcmp

zoulasc · 2025-03-17T13:24:30Z

sys/net/npf/npf_ruleset.c

+		return -1;
+
+	matched = 0;
+	if ( rl->uid)


zoulasc · 2025-03-17T13:26:30Z

usr.sbin/npf/npfctl/npf_parse.y

+
+group_id
+	: /* empty */ { $$ = NULL; }
+	| USERGROUP gids	{ $$ = $2; }


Are you calling this "USERGROUP" instead of "GROUP" because group is already used?

Emmankoko · 2025-03-17T13:28:02Z

Yes!

On 17 Mar 2025, at 1:26 PM, zoulasc ***@***.***> wrote: @zoulasc commented on this pull request. In usr.sbin/npf/npfctl/npf_parse.y <#45 (comment)>: > + $$ = npfvar_expand_number(vp); + break; + case -1: + yyerror("undefined variable '%s'", $1); + break; + default: + yyerror("wrong variable '%s' type '%s' for user id", + $1, npfvar_type(type)); + break; + } + } + ; + +group_id + : /* empty */ { $$ = NULL; } + | USERGROUP gids { $$ = $2; } Are you calling this "USERGROUP" instead of "GROUP" because group is already used? — Reply to this email directly, view it on GitHub <#45 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANU23YNGAMNKLUPNQDFKEGD2U3ERZAVCNFSM6AAAAABZFEDJMGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMOJQGQ4TINRVGM>. You are receiving this because you authored the thread.

Emmanuel

functions implementing for setting the uid and gid structure. group keyword used gids whitespace fixed no parens on strcmp

zoulasc · 2025-03-25T19:07:04Z

sys/net/npf/npf_socket.c

+static int
+npf_match(uint8_t op, uint32_t rid1, uint32_t rid2, uint32_t id_lp)
+{
+	switch (op) {


no parens in return.

zoulasc · 2025-03-25T19:07:44Z

lib/libnpf/npf.c

+}
+
+int
+npf_rule_setrugrp(nl_rule_t *rl, struct r_gid *gid)


why ugrp instead of group?

zoulasc · 2025-03-25T19:08:09Z

lib/libnpf/npf.c

+int
+npf_rule_setruser(nl_rule_t *rl, struct r_uid *uid)
+{
+	int error = 0;


why use error? delete the var and return directly.

zoulasc · 2025-03-25T19:09:22Z

sys/net/npf/npf_ctl.c

+		const uint64_t *uid = nvlist_get_number_array(req, "r_user", &uitems);
+		KASSERT(uitems == 3);
+
+		r_uid.uid[0] = (uint32_t)uid[0];


these should be (uid_t) not (uint32_t).

but... you are using so much uint32_t and uid_t/gid_t interchangeably, so why don't you just call it id in the structure, make it uint32_t and then merge all the duplicated code. You can put in a comment why (both uid_t and gid_t and uint32_t and using uint32_t saves a lot of code duplication).

zoulasc · 2025-03-25T19:18:03Z

sys/net/npf/npf.h

@@ -59,6 +59,16 @@ typedef union {
 	uint32_t		word32[4];
 } npf_addr_t;

+struct r_uid {


If you merge the two structs call it r_id { uint32_t id[2]; uint8_t op; };
Then you can merge most of the code.

zoulasc · 2025-03-26T20:06:40Z

sys/net/npf/npf_socket.c

+int
+npf_socket_lookup_uid(npf_cache_t *npc, int dir, uint32_t *uid)
+{
+	struct inpcbtable	*tb = NULL;


tb is NULL, why put it in a variable instead of passing NULL?

zoulasc · 2025-03-26T20:07:08Z

sys/net/npf/npf_socket.c

+
+	if (npf_iscached(npc, NPC_IP4)) {
+		so = npf_ip_socket(npc, tb, dir);
+


remove blank line

zoulasc · 2025-03-26T20:07:45Z

sys/net/npf/npf_socket.c

+
+	} else if (npf_iscached(npc, NPC_IP6)) {
+		so = npf_ip6_socket(npc, tb, dir);
+	}


else return -1; then you don't need to over-initialize so = NULL;

zoulasc · 2025-03-26T20:08:18Z

sys/net/npf/npf_socket.c

+}
+
+int
+npf_socket_lookup_gid(npf_cache_t *npc, int dir, uint32_t *gid)


this is duplicated for uid and gid merge them.

zoulasc · 2025-03-27T12:37:38Z

sys/net/npf/npf_ruleset.c

@@ -118,6 +118,9 @@ struct npf_rule {
 	LIST_ENTRY(npf_rule)	r_aentry;
 	nvlist_t *		r_info;
 	size_t			r_info_len;
+
+	rid_t *uid;


I don't know about storing pointers here and needing to allocate and free them seems to be a lot of work for very little benefit since the structures are small. Perhaps just store the structures and add NPF_OP_INVALID as the first element of the enum to mean 0 == unused?

I agree. too small to be allocating for it not optimal.

zoulasc · 2025-03-28T16:01:51Z

sys/net/npf/npf_ruleset.c

+/* again, use a single rule getid function both uid and gids */
+static rid_t
+npf_rule_getrid(const nvlist_t *req, const char *name)
+{


Why all this machinery? get rid of npf_rule_getrid and put all of it in npf_rule_setrid and operate on the pointer directly.

zoulasc · 2025-03-28T16:02:38Z

sys/net/npf/npf_ruleset.c

+}
+
+void
+npf_rule_setuid(const nvlist_t *req, npf_rule_t *rl, const char *name)


and these two are overkill too. Just do it directly.

I don't think It will be possible to do it directly here means I would have to call it in npf_ctl.c where it extracts the nvpair. I would have to pass rl->uid as argument to the function and it won't work because rl is an opaque type in npf_ruleset TU and hence cannot be dereferenced out of the ruleset TU

Also, don't memcpy() structures -- just assign them.

zoulasc · 2025-03-28T16:04:01Z

sys/net/npf/npf_ruleset.c

+* if socket id doesn't match any of rule ids, reverse action
+*/
+int
+npf_rule_match_user(npf_rule_t *rl, npf_cache_t *npc, int dir)


these two can be merged too, by passing the function npf_socket_lookup_uid/gid as a parameter.

zoulasc · 2025-03-28T16:04:52Z

usr.sbin/npf/npfctl/npf_parse.y

 	double		fpnum;
 	npfvar_t *	var;
 	addr_port_t	addrport;
 	filt_opts_t	filtopts;
 	opt_proto_t	optproto;
 	rule_group_t	rulegroup;
+	rid_t 	*uid;


those don't have to be pointers either. makes the code simpler.

Emmankoko added 6 commits March 17, 2025 10:25

set the rule user and group in libnpf(nvlist) to be sent to the kernel

d38d3b8

retrive uid and gid structure userland nvlist if set

305a3f8

set the received uid and gid structures into the global opaque rule s…

54516c4

…tructure

implement socket in the communication lookup and fetch the user/group…

29b7ac3

… owner of that socket.

zoulasc reviewed Mar 17, 2025

View reviewed changes

sys/net/npf/npf_ruleset.c Outdated

return -1;

matched = 0;

if ( rl->uid)

Copy link

Contributor

zoulasc Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

zoulasc reviewed Mar 17, 2025

View reviewed changes

Emmankoko added 2 commits March 17, 2025 21:38

requested changes

b4225e8

functions implementing for setting the uid and gid structure. group keyword used gids whitespace fixed no parens on strcmp

make static, usergoup=>group changes

b71ac11

Emmankoko force-pushed the uid_rev branch from c46af93 to b71ac11 Compare March 18, 2025 19:09

zoulasc reviewed Mar 25, 2025

View reviewed changes

lib/libnpf/npf.c Outdated

}

int

npf_rule_setrugrp(nl_rule_t *rl, struct r_gid *gid)

Copy link

Contributor

zoulasc Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ugrp instead of group?

zoulasc reviewed Mar 25, 2025

View reviewed changes

Emmankoko added 3 commits March 26, 2025 19:01

remove parens from return statements in id match

6ddc377

handle duplications in uid and gid code

541d8db

consistently use rid_t instead of struct rid: typedefed

1a53ebf

Emmankoko force-pushed the uid_rev branch from ac0aa16 to 1a53ebf Compare March 26, 2025 19:49

zoulasc reviewed Mar 26, 2025

View reviewed changes

sys/net/npf/npf_socket.c Outdated

if (npf_iscached(npc, NPC_IP4)) {

so = npf_ip_socket(npc, tb, dir);

Copy link

Contributor

zoulasc Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove blank line

zoulasc reviewed Mar 26, 2025

View reviewed changes

handle duplications in socket lookup and idmatch

c689702

zoulasc reviewed Mar 27, 2025

View reviewed changes

handle uid and gid by value in rule

03dba58

zoulasc reviewed Mar 28, 2025

View reviewed changes

Emmankoko closed this Mar 29, 2025

Emmankoko reopened this Mar 29, 2025

Emmankoko closed this Mar 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User id and group id integration in NPF #45

User id and group id integration in NPF #45

Emmankoko commented Mar 17, 2025

zoulasc Mar 17, 2025

zoulasc Mar 17, 2025

zoulasc Mar 17, 2025

zoulasc Mar 17, 2025

zoulasc Mar 17, 2025

Emmankoko commented Mar 17, 2025 via email

zoulasc Mar 25, 2025

zoulasc Mar 25, 2025

zoulasc Mar 25, 2025

zoulasc Mar 25, 2025

zoulasc Mar 25, 2025

zoulasc Mar 25, 2025

zoulasc Mar 26, 2025

zoulasc Mar 26, 2025

zoulasc Mar 26, 2025

zoulasc Mar 26, 2025

zoulasc Mar 27, 2025

Emmankoko Mar 28, 2025

zoulasc Mar 28, 2025 •

edited

Loading

zoulasc Mar 28, 2025 •

edited

Loading

Emmankoko Mar 28, 2025

robohack Mar 29, 2025

zoulasc Mar 28, 2025

zoulasc Mar 28, 2025


		if (npf_iscached(npc, NPC_IP4)) {
		so = npf_ip_socket(npc, tb, dir);

User id and group id integration in NPF #45

User id and group id integration in NPF #45

Conversation

Emmankoko commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emmankoko commented Mar 17, 2025 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoulasc Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

zoulasc Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoulasc Mar 28, 2025 •

edited

Loading

zoulasc Mar 28, 2025 •

edited

Loading