[PATCH] Uids should not be allowed to set to negative

[PATCH] Uids should not be allowed to set to negative

[Cai Xianchao]

Hello Steve,

When I tried to set uid to negative, no error message was outputed and
the return value was 0. In the rule list, the value of uid was also
negative. Negative uid does not exist and the negative user can't be
added. So, I think uids can't be set to negative.

It is also strange that gid can't be set to negative, while uid can.

I did as follows:
#auditctl -a exit,always -F uid=-1
#auditctl -l
LIST_RULES: exit,always uid=-1 (0xffffffff) syscall=all

Signed-off-by: Cai Xianchao <caixianchao@cn.fujistu.com>
---
diff --git a/deprecated.c b/deprecated.c
index 6bf42dd..94954a5 100644
--- a/deprecated.c
+++ b/deprecated.c
@@ -257,10 +257,6 @@ int audit_rule_fieldpair(struct audit_rule *rule, const char *pair, int flags)
 			if (isdigit((char)*(v))) 
 				rule->values[rule->field_count] = 
 					strtol(v, NULL, 0);
-			else if (vlen >= 2 && *(v)=='-' && 
-						(isdigit((char)*(v+1)))) 
-				rule->values[rule->field_count] = 
-					strtol(v, NULL, 0);
 			else {
 				if (name_to_uid(v, 
 					&rule->values[rule->field_count])) {
diff --git a/libaudit.c b/libaudit.c
index 7d48d78..85adbc5 100644
--- a/libaudit.c
+++ b/libaudit.c
@@ -850,10 +850,6 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			if (isdigit((char)*(v))) 
 				rule->values[rule->field_count] = 
 					strtol(v, NULL, 0);
-			else if (vlen >= 2 && *(v)=='-' && 
-						(isdigit((char)*(v+1)))) 
-				rule->values[rule->field_count] = 
-					strtol(v, NULL, 0);
 			else {
 				if (audit_name_to_uid(v, 
 					&rule->values[rule->field_count])) {

Re: [PATCH] Uids should not be allowed to set to negative

[Eric Paris]

On Fri, 2008-08-08 at 18:09 +0800, Cai Xianchao wrote:
> Hello Steve,
> 
> When I tried to set uid to negative, no error message was outputed and
> the return value was 0. In the rule list, the value of uid was also
> negative. Negative uid does not exist and the negative user can't be
> added. So, I think uids can't be set to negative.
> 
> It is also strange that gid can't be set to negative, while uid can.

Its not the same code that matches uid and auid is it?  auid can
reasonably be negative for anything that wasn't done from a login shell.
just want to make sure you don't lose that ability.

-Eric

> 
> I did as follows:
> #auditctl -a exit,always -F uid=-1
> #auditctl -l
> LIST_RULES: exit,always uid=-1 (0xffffffff) syscall=all
> 
> Signed-off-by: Cai Xianchao <caixianchao@cn.fujistu.com>
> ---
> diff --git a/deprecated.c b/deprecated.c
> index 6bf42dd..94954a5 100644
> --- a/deprecated.c
> +++ b/deprecated.c
> @@ -257,10 +257,6 @@ int audit_rule_fieldpair(struct audit_rule *rule, const char *pair, int flags)
>  			if (isdigit((char)*(v))) 
>  				rule->values[rule->field_count] = 
>  					strtol(v, NULL, 0);
> -			else if (vlen >= 2 && *(v)=='-' && 
> -						(isdigit((char)*(v+1)))) 
> -				rule->values[rule->field_count] = 
> -					strtol(v, NULL, 0);
>  			else {
>  				if (name_to_uid(v, 
>  					&rule->values[rule->field_count])) {
> diff --git a/libaudit.c b/libaudit.c
> index 7d48d78..85adbc5 100644
> --- a/libaudit.c
> +++ b/libaudit.c
> @@ -850,10 +850,6 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
>  			if (isdigit((char)*(v))) 
>  				rule->values[rule->field_count] = 
>  					strtol(v, NULL, 0);
> -			else if (vlen >= 2 && *(v)=='-' && 
> -						(isdigit((char)*(v+1)))) 
> -				rule->values[rule->field_count] = 
> -					strtol(v, NULL, 0);
>  			else {
>  				if (audit_name_to_uid(v, 
>  					&rule->values[rule->field_count])) {
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] Uids should not be allowed to set to negative

[Steve Grubb]

On Friday 08 August 2008 09:25:09 Eric Paris wrote:
> > It is also strange that gid can't be set to negative, while uid can.
>
> Its not the same code that matches uid and auid is it?  auid can
> reasonably be negative for anything that wasn't done from a login shell.
> just want to make sure you don't lose that ability.

That's true. But unfortunately, we have to give the uid as the unsigned value 
or we lose a bit in the conversion and it doesn't match. On second thought, 
maybe we can't do negative uids from user space because of that conversion to 
unsigned inside the rule matching engine.

-Steve

Re: [PATCH] Uids should not be allowed to set to negative

[Steve Grubb]

On Friday 08 August 2008 06:09:59 Cai Xianchao wrote:
> When I tried to set uid to negative, no error message was outputed and
> the return value was 0. In the rule list, the value of uid was also
> negative. Negative uid does not exist and the negative user can't be
> added. So, I think uids can't be set to negative.

There really is uid -1 and uid -2. We run into this problem with faillog all 
the time. I think they are nobody and nfsnobody respectively. They used by 
NFS.

-Steve

Re: [PATCH] Uids should not be allowed to set to negative

[Steve Grubb]

On Friday 08 August 2008 06:09:59 Cai Xianchao wrote:
> When I tried to set uid to negative, no error message was outputed and
> the return value was 0. In the rule list, the value of uid was also
> negative. Negative uid does not exist and the negative user can't be
> added. So, I think uids can't be set to negative.

Applied. Thanks for the patch !

-Steve