This is the mail archive of the cygwin-patches@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: ntsec patch 1: uid==gid, chmod, alloc_sd, is_grp_member


On Thu, Nov 14, 2002 at 10:04:54PM -0500, Pierre A. Humblet wrote:
> Great! Here are my patches. I think they are as we agreed on.
> 
> Pierre

Sorry, I still have some problems:

>  	}
>      }
>    *attribute &= ~(S_IRWXU | S_IRWXG | S_IRWXO | S_ISVTX | S_ISGID | S_ISUID);
> +  if (owner_sid && group_sid && EqualSid (owner_sid, group_sid))
> +    {
> +      allow &= ~(S_IRGRP | S_IWGRP | S_IXGRP);
> +      allow |= (((allow & S_IRUSR) ? S_IRGRP : 0)
> +		| ((allow & S_IWUSR) ? S_IWGRP : 0)
> +		| ((allow & S_IXUSR) ? S_IXGRP : 0));
> +    }
>    *attribute |= allow;
> -  *attribute &= ~deny;
>    return;
>  }
> 

I found that this change had an unpredictable result.  sshd refused to
start:

  Bad owner or mode for /var/empty

Looking for the modes.  Before that change:

  drwxr-xr-x    2 SYSTEM   SYSTEM          0 Jul  7 11:39 /var/empty

With that patch:

  drwxrwxr-x    2 SYSTEM   SYSTEM          0 Jul  7 11:39 /var/empty

I'm not against the patch since it reflects the permissions obviously
better than the old version.  However, how many people are running
sshd now with the above /var/empty settings.  Urgh.

I only see two choices:
- Let it as it is now.
- Force all people with a running sshd installation to chmod 544
  /var/empty.

> @@ -1664,18 +1666,25 @@ alloc_sd (__uid32_t uid, __gid32_t gid,
>        if (attribute & S_ISVTX)
>  	null_allow |= FILE_READ_DATA;
>      }
> -
> -  /* Construct deny attributes for owner and group. */
> -  DWORD owner_deny = 0;
> -  if (is_grp_member (uid, gid))
> -    owner_deny = ~owner_allow & (group_allow | other_allow);

Erm... that's not exactly what we agreed upon...

> +
> +  /* Add owner and group permissions if SIDs are equal
> +     and construct deny attributes for group and owner. */
> +  DWORD group_deny;
> +  if (owner_sid == group_sid)
> +    {
> +      owner_allow |= group_allow;
> +      group_allow = group_deny = 0L;
> +    }

And this change will produce the above sshd problem for any new user
calling ssh-host-config.

>  	  if (!AddAce (acl, ACL_REVISION,
>  		       ace->Header.AceType == ACCESS_DENIED_ACE_TYPE ?
> -		       (owner_deny ? 1 : 0) : MAXDWORD,
> +		       ace->Mask & owner_allow ? owner_off + 1 : owner_off++
> +		       : MAXDWORD,
>  		       (LPVOID) ace, ace->Header.AceSize))

After applying the patch to my local sandbox I found that I'm still
having problems here.  While I see the advantage for emulating POSIX
permissions closer, I also see that the probability is pretty high
that all unrelated deny ACEs will be placed after the owner_allow
(which probably has most bits set).  This doesn't really support the
wish to produce ACLs in canonical order.  So far, only the group_deny
could possibly but unlikely be placed after the owner_allow...

Corinna


-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Developer                                mailto:cygwin@cygwin.com
Red Hat, Inc.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]