bug#865: 23.0.60; The directory is unsafe today

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

server-ensure-safe-dir complained today during server-start. Examining
the values in server-ensure-safe-dir I found the following:

  (default-file-modes) => 511 (#o777, #x1ff)
  (user-uid) => 30675 (#o73723, #x77d3)

  dir's value is
  "c:/Documents and Settings/my-userid/Application Data/.emacs.d/server"

  drwxrwxrwx  1 Administrators Domain Users 0 03-07 11:14 server

  attrs's value is
  (t 1 544 513
     (18621 8536)
     (18385 5511)
     (17762 50477)
     0 "drwxrwxrwx" nil
     (44800 0 . 20258)
     (32879 . 13859))


The part that where server-unsure-safe-dir barks is

  (eql (nth 2 attrs) (user-uid))

This check is maybe something that should be skipped on w32? As you can
see from the dired output above it will get "Domain Users" in my case.



(This is from my patched version, but there is no patches in w32.c or
server.el.)

In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-09-02 on (patched)
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --cflags -Ic:/g/include'

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Lennart Borgman (gmail) wrote:
>   drwxrwxrwx  1 Administrators Domain Users 0 03-07 11:14 server
>
> The part that where server-unsure-safe-dir barks is
>
>   (eql (nth 2 attrs) (user-uid))
>
> This check is maybe something that should be skipped on w32? As you can
> see from the dired output above it will get "Domain Users" in my case.
>   

I think it will get "Administrators" in fact, but it is still wrong. 
This is caused by a misfeature of Windows, that if the creator of a file 
is a member of the local "Administrators" group, then ownership is 
assigned to "Administrators" instead of the individual user. I think a 
similar problem will occur on FAT32 systems where ownership is assigned 
to "Everyone".

Eli, is it possible to tell the difference between an individual user 
and a group when we get the owner from the system? If so, we could check 
if the current user is a member of that group, and if so report them as 
the owner rather than the whole group, to work around ownership tests 
like this in lisp code.

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Jason Rumney wrote:
> Lennart Borgman (gmail) wrote:
>>   drwxrwxrwx  1 Administrators Domain Users 0 03-07 11:14 server
>>
>> The part that where server-unsure-safe-dir barks is
>>
>>   (eql (nth 2 attrs) (user-uid))
>>
>> This check is maybe something that should be skipped on w32? As you can
>> see from the dired output above it will get "Domain Users" in my case.
>>   
> 
> I think it will get "Administrators" in fact, but it is still wrong.
> This is caused by a misfeature of Windows, that if the creator of a file
> is a member of the local "Administrators" group, then ownership is
> assigned to "Administrators" instead of the individual user. I think a
> similar problem will occur on FAT32 systems where ownership is assigned
> to "Everyone".
> 
> Eli, is it possible to tell the difference between an individual user
> and a group when we get the owner from the system? If so, we could check
> if the current user is a member of that group, and if so report them as
> the owner rather than the whole group, to work around ownership tests
> like this in lisp code.

I can see what you want to achieve, but why not just check with
file-writeable-p here? Is not that more correct?

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Tue, 02 Sep 2008 20:06:56 +0200
> From: "Lennart Borgman (gmail)" <lennart.borgman@gmail.com>
> Cc: emacs-pretest-bug@gnu.org, 865@emacsbugs.donarmstrong.com
> 
> why not just check with file-writeable-p here?

Don't forget that on Windows, almost every file is writable.  You
don't want everybody and their dog to be able to look at your server
directory, do you?

IOW, this test is not about being able to write to a directory, it's
about keeping your private data private.

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Eli Zaretskii wrote:
>> Date: Tue, 02 Sep 2008 20:06:56 +0200
>> From: "Lennart Borgman (gmail)" <lennart.borgman@gmail.com>
>> Cc: emacs-pretest-bug@gnu.org, 865@emacsbugs.donarmstrong.com
>>
>> why not just check with file-writeable-p here?
> 
> Don't forget that on Windows, almost every file is writable.  You
> don't want everybody and their dog to be able to look at your server
> directory, do you?
> 
> IOW, this test is not about being able to write to a directory, it's
> about keeping your private data private.

Yes, you are right it was a silly suggestion.

On the other hand the doc string is wrong. Isn't it impossible to be
sure that there are no race conditions? And the list with different
things can't be fullfilled.

On w32 the location is maybe more meaningful than the owner? I would
suggest checking for that instead on w32.

BTW there is a

  (ignore-errors (delete-file server-file))

in server-start. I do not like the ignore-errors there. Check if the
file is there first and if it is delete it.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Wed, 03 Sep 2008 01:16:05 +0800
> From: Jason Rumney <jasonr@gnu.org>
> Cc: emacs-pretest-bug@gnu.org
> 
> Lennart Borgman (gmail) wrote:
> >   drwxrwxrwx  1 Administrators Domain Users 0 03-07 11:14 server
> >
> > The part that where server-unsure-safe-dir barks is
> >
> >   (eql (nth 2 attrs) (user-uid))
> >
> > This check is maybe something that should be skipped on w32? As you can
> > see from the dired output above it will get "Domain Users" in my case.
> >   
> 
> I think it will get "Administrators" in fact

Yes, because `nth's argument counts from zero.

> Eli, is it possible to tell the difference between an individual user 
> and a group when we get the owner from the system?

If you mean to ask whether LookupAccountSid we use to get the owner or
group name by their SID can tell us if the name it returns is a group
rather than a user, then no, I don't think so, unfortunately.  AFAIK,
Windows doesn't really know that itself, it stores the user and group
names in the same place.  In particular, no user can have a SID that
is identical to a SID of some group.

However, Administrators and Everyone have universally fixed RID values
(544 and 0 respectively), so we could make special code for those two
values on Windows (the offending function already makes exceptions for
Windows anyway).

We can also list all the groups to which the user belongs, and check
if one of them is Administrators.

Btw, I think erroring out if what server-ensure-safe-dir wants to see
is not true is too draconian.  It may be better to ask, at least an an
optional behavior.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Tue, 02 Sep 2008 21:10:32 +0200
> From: "Lennart Borgman (gmail)" <lennart.borgman@gmail.com>
> CC: 865@emacsbugs.donarmstrong.com, jasonr@gnu.org
> 
> On the other hand the doc string is wrong.

I agree that the doc string "needs work".  It sounds like the function
started from something like what the doc string describes, but then
additional functionality was added to the code, and the doc string
never caught up.

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Eli Zaretskii wrote:
>> Date: Tue, 02 Sep 2008 21:10:32 +0200
>> From: "Lennart Borgman (gmail)" <lennart.borgman@gmail.com>
>> CC: 865@emacsbugs.donarmstrong.com, jasonr@gnu.org
>>
>> On the other hand the doc string is wrong.
> 
> I agree that the doc string "needs work".  It sounds like the function
> started from something like what the doc string describes, but then
> additional functionality was added to the code, and the doc string
> never caught up.

What I was trying to say was that the doc string seems to have started
from some ideas of how to solve the race condition problem. Those ideas
just does not fit very well here.

Checking the owner for example may fit much better on a file system that
is not using ACL-lists (are they dead on *nix?).

The ideas to use may have to be different on different systems.

So I think it is better to start with the ideas again. I guess the ideas
that someone tried to expressed in the doc strings may fit *nix. It that
is the case then let the doc string say that.

For w32 other criterias should be used in my opinion.

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Tue, Sep 2, 2008 at 21:10, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> BTW there is a
>
>  (ignore-errors (delete-file server-file))
>
> in server-start. I do not like the ignore-errors there. Check if the
> file is there first and if it is delete it.

How are you going to guarantee that the file isn't deleted by another
process between the check and the delete-file call?

  Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Tue, Sep 2, 2008 at 21:10, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> BTW there is a
>>
>>  (ignore-errors (delete-file server-file))
>>
>> in server-start. I do not like the ignore-errors there. Check if the
>> file is there first and if it is delete it.
> 
> How are you going to guarantee that the file isn't deleted by another
> process between the check and the delete-file call?


Forgot that discussion. Yes, but why not check the error returned instead?

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 00:40, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> Forgot that discussion. Yes, but why not check the error returned instead?

Check for what?

   Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 00:40, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> Forgot that discussion. Yes, but why not check the error returned instead?
> 
> Check for what?


Hm, I see that you can just get the rather general `file-error' error.

Maybe it is better to just test if the file really was deleted then.

The reason I want to test this is that errors in these parts often are
confusing and hard to find. There could be something that prevents the
file from beeing deleted.

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:01, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> Maybe it is better to just test if the file really was deleted then.

And, again, how would you guarantee that the file isn't created by
another process between the delete-file call and the check?

> The reason I want to test this is that errors in these parts often are
> confusing and hard to find. There could be something that prevents the
> file from beeing deleted.

The server file is deleted to make room for the new server file. If
writing it fails, you'll notice.

Are you chasing a real error, or the possibility of one?

   Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:01, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> Maybe it is better to just test if the file really was deleted then.
> 
> And, again, how would you guarantee that the file isn't created by
> another process between the delete-file call and the check?
> 
>> The reason I want to test this is that errors in these parts often are
>> confusing and hard to find. There could be something that prevents the
>> file from beeing deleted.
> 
> The server file is deleted to make room for the new server file. If
> writing it fails, you'll notice.

Yes, but is it not easier to understand what is happening and catch the
message right after the delete-file?

> Are you chasing a real error, or the possibility of one?

A possible one.

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:11, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> Yes, but is it not easier to understand what is happening and catch the
> message right after the delete-file?

You still haven't shown how you would do that without a race condition.

   Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:11, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> Yes, but is it not easier to understand what is happening and catch the
>> message right after the delete-file?
> 
> You still haven't shown how you would do that without a race condition.


I will not do that ... ;-)

I got your point, but I still think it is better to try to catch the
error there.

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:19, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> I got your point, but I still think it is better to try to catch the
> error there.

Why? Deleting the old server-file is irrelevant, it's just cleanup.
Failing to do it does not affect the Emacs server.

Failing to write a new server file *is* a problem: you won't be able
to communicate with emacsclient, or it will use erroneous or malicious
data.

The relevant operations there are `(make-network-process ... :service
server-file ...)' and `(with-temp-file server-file ...)'; these are
the ones that should signal an error if something fails. And they do.

 Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:19, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> I got your point, but I still think it is better to try to catch the
>> error there.
> 
> Why? Deleting the old server-file is irrelevant, it's just cleanup.
> Failing to do it does not affect the Emacs server.
> 
> Failing to write a new server file *is* a problem: you won't be able
> to communicate with emacsclient, or it will use erroneous or malicious
> data.

So you mean there is no more problem if the old file was not deleted?
Why delete it at all in that case?

> The relevant operations there are `(make-network-process ... :service
> server-file ...)' and `(with-temp-file server-file ...)'; these are
> the ones that should signal an error if something fails. And they do.
> 
>  Juanma
> 

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:27, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> So you mean there is no more problem if the old file was not deleted?

I didn't say that. But the problem is not that you deleted it or not,
but whether that affects creating the new server-file or opening the
new local socket.

> Why delete it at all in that case?

Cleanup is a good prophilactic measure, but it is not always required:

 (with-temp-file "~/temp.dat" (insert "Hi!"))
 (with-temp-file "~/temp.dat" (insert "Bye!"))

 Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:27, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> So you mean there is no more problem if the old file was not deleted?
> 
> I didn't say that. But the problem is not that you deleted it or not,
> but whether that affects creating the new server-file or opening the
> new local socket.
> 
>> Why delete it at all in that case?
> 
> Cleanup is a good prophilactic measure, but it is not always required:

But in this case the dirt is back just in few lines below ...

>  (with-temp-file "~/temp.dat" (insert "Hi!"))
>  (with-temp-file "~/temp.dat" (insert "Bye!"))
> 
>  Juanma
> 

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:41, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> But in this case the dirt is back just in few lines below ...

I miss your point.

   Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:41, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> But in this case the dirt is back just in few lines below ...
> 
> I miss your point.


You are doing the cleanup just before you expect the file to be created
again. Not that cleanup use to last very long but the life time of this
one is very short.

The only reason I can see to delete the file is that it matters when the
network process creates it again. And in that case it might be good to
check if it really was deleted.

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Eli Zaretskii wrote:
> If you mean to ask whether LookupAccountSid we use to get the owner or
> group name by their SID can tell us if the name it returns is a group
> rather than a user, then no, I don't think so, unfortunately.

There is a variable in get_name_and_id called `ignore', which seems to 
have the required information.
I'll try to come up with a patch later today.

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:50, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> And in that case it might be good to
> check if it really was deleted.

How?

   Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:50, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> And in that case it might be good to
>> check if it really was deleted.
> 
> How?


Just check if it exists immediately after the delete. As you have
pointed out some race condition (which one is unclear to me) can exist
so that it exists there again.

However I did not say that we should raise an error in that case. It
might be legitimate. So we can perhaps instead check file modification
time and see if it is a new file. In that case we could just give a
message (which might be good for hunting errors).

If file modification time says it is the same old file then it is an error.

bug#865: 23.0.60; The directory is unsafe today

[Juanma Barranquero]

On Wed, Sep 3, 2008 at 01:58, Lennart Borgman (gmail)
<lennart.borgman@gmail.com> wrote:

> Just check if it exists immediately after the delete. As you have
> pointed out some race condition (which one is unclear to me) can exist
> so that it exists there again.
>
> However I did not say that we should raise an error in that case. It
> might be legitimate. So we can perhaps instead check file modification
> time and see if it is a new file. In that case we could just give a
> message (which might be good for hunting errors).
>
> If file modification time says it is the same old file then it is an error.

You want to do a lot of work for nothing.

   Juanma

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Juanma Barranquero wrote:
> On Wed, Sep 3, 2008 at 01:58, Lennart Borgman (gmail)
> <lennart.borgman@gmail.com> wrote:
> 
>> Just check if it exists immediately after the delete. As you have
>> pointed out some race condition (which one is unclear to me) can exist
>> so that it exists there again.
>>
>> However I did not say that we should raise an error in that case. It
>> might be legitimate. So we can perhaps instead check file modification
>> time and see if it is a new file. In that case we could just give a
>> message (which might be good for hunting errors).
>>
>> If file modification time says it is the same old file then it is an error.
> 
> You want to do a lot of work for nothing.


Hm, I do not like doing cleaning without any reason.

I think you might be missing that the situation can be complicated when
ACL lists are involved. But I guess it is not common.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Wed, 03 Sep 2008 07:50:45 +0800
> From: Jason Rumney <jasonr@gnu.org>
> CC: 865@emacsbugs.donarmstrong.com, lennart.borgman@gmail.com, 
>  emacs-pretest-bug@gnu.org
> 
> Eli Zaretskii wrote:
> > If you mean to ask whether LookupAccountSid we use to get the owner or
> > group name by their SID can tell us if the name it returns is a group
> > rather than a user, then no, I don't think so, unfortunately.
> 
> There is a variable in get_name_and_id called `ignore', which seems to 
> have the required information.

Thanks.

I never found any sufficiently clear documentation of the meaning of
that parameter.  Its value seems almost random.  If you can find an
explanation or give one, please do.  Otherwise, I don't like to rely
on a variable whose behavior we don't understand.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

> I can see what you want to achieve, but why not just check with
> file-writeable-p here? Is not that more correct?

The purpose of the check is not to make sure we can make the server
work, but that it is safe to do so.  I.e. it doesn't check that we have
write access, but that nobody else does.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Stefan Monnier wrote:
>> I can see what you want to achieve, but why not just check with
>> file-writeable-p here? Is not that more correct?
> 
> The purpose of the check is not to make sure we can make the server
> work, but that it is safe to do so.  I.e. it doesn't check that we have
> write access, but that nobody else does.


Yes, as I said in my reply to Eli, I realized that. I suggested another
way to try to achieve that which I think is much better.

If you really want to make sure no one else can write you have to set up
the access rights for that. On w32 that means you should modify the ACL
list. The current code is far from that.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

> If you really want to make sure no one else can write you have to set up
> the access rights for that. On w32 that means you should modify the ACL
> list. The current code is far from that.

The current code "does just that" using file-modes.
Now, of course, depending on how file-modes are mapped to&from ACLs,
this may DTRT or not.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Wed, 03 Sep 2008 18:35:10 +0200
> From: "Lennart Borgman (gmail)" <lennart.borgman@gmail.com>
> Cc: emacs-pretest-bug@gnu.org, 865@emacsbugs.donarmstrong.com
> 
> If you really want to make sure no one else can write you have to set up
> the access rights for that. On w32 that means you should modify the ACL
> list. The current code is far from that.

Patches to do that are most welcome.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Wed, 03 Sep 2008 06:27:43 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-pretest-bug@gnu.org, 865@emacsbugs.donarmstrong.com
> 
> > There is a variable in get_name_and_id called `ignore', which seems to 
> > have the required information.
> 
> Thanks.
> 
> I never found any sufficiently clear documentation of the meaning of
> that parameter.  Its value seems almost random.  If you can find an
> explanation or give one, please do.

Sorry, I confused myself and thought about a different parameter.

Yes, the last argument of LookupAccountSid does tell what type of
account is that.  But I'm not sure that what you suggest is a good
strategy: the fact that the current user is a member of Administrators
does not yet mean we can report that user as the file's owner.

How about testing the user's primary group in addition to UID?  That
is, 

  (or (eql (nth 2 attrs) (user-uid))
      (and (eq system-type 'windows-nt) (eql (nth 2 attrs) (user-gid)))

(assuming we add a function user-gid)?

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Wed, 03 Sep 2008 14:05:43 -0400
> Cc: emacs-pretest-bug@gnu.org, 865@emacsbugs.donarmstrong.com
> 
> > If you really want to make sure no one else can write you have to set up
> > the access rights for that. On w32 that means you should modify the ACL
> > list. The current code is far from that.
> 
> The current code "does just that" using file-modes.
> Now, of course, depending on how file-modes are mapped to&from ACLs,
> this may DTRT or not.

The problem is not the mapping of ACLs into file modes, the problem is
the opposite mapping.  The current Emacs code assumes Posix semantics,
where a file created like this:

  (letf (((default-file-modes) ?\700)) (make-directory dir t))

will not be accessible by anyone else but the user who created it.
But on Windows, this does not do what it does on Posix, because the
Posix rwx model does not map well into ACLs.  If we want this to work
on platforms that support ACLs (including some varieties of GNU/Linux
systems), we need to use a more sophisticated abstraction.

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Eli Zaretskii wrote:

> Sorry, I confused myself and thought about a different parameter.
>
> Yes, the last argument of LookupAccountSid does tell what type of
> account is that.  But I'm not sure that what you suggest is a good
> strategy: the fact that the current user is a member of Administrators
> does not yet mean we can report that user as the file's owner.
>
> How about testing the user's primary group in addition to UID?  That
> is, 
>
>   (or (eql (nth 2 attrs) (user-uid))
>       (and (eq system-type 'windows-nt) (eql (nth 2 attrs) (user-gid)))
>   

I am trying to find a solution that will work without Lisp coders 
needing to be aware of the strange situation on Windows that files can 
be owned by a group.

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Jason Rumney wrote:
> Eli Zaretskii wrote:
> 
>> Sorry, I confused myself and thought about a different parameter.
>>
>> Yes, the last argument of LookupAccountSid does tell what type of
>> account is that.  But I'm not sure that what you suggest is a good
>> strategy: the fact that the current user is a member of Administrators
>> does not yet mean we can report that user as the file's owner.
>>
>> How about testing the user's primary group in addition to UID?  That
>> is,
>>   (or (eql (nth 2 attrs) (user-uid))
>>       (and (eq system-type 'windows-nt) (eql (nth 2 attrs) (user-gid)))
>>   
> 
> I am trying to find a solution that will work without Lisp coders
> needing to be aware of the strange situation on Windows that files can
> be owned by a group.


Then, since it at this times seems like we want to forget about ACL
lists, then perhaps implementing a function for changing owner+group,
like chown, would be best?

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Thu, 04 Sep 2008 07:44:49 +0800
> From: Jason Rumney <jasonr@gnu.org>
> CC: 865@emacsbugs.donarmstrong.com, emacs-pretest-bug@gnu.org
> 
> >   (or (eql (nth 2 attrs) (user-uid))
> >       (and (eq system-type 'windows-nt) (eql (nth 2 attrs) (user-gid)))
> >   
> 
> I am trying to find a solution that will work without Lisp coders 
> needing to be aware of the strange situation on Windows that files can 
> be owned by a group.

I think the only good solution in that venue would be to have a new
primitive for such tests, and implement it on Windows so that it will
cater to this special case.  Any other solution is likely to lie about
the file's security attributes.

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Eli Zaretskii wrote:
>> Date: Thu, 04 Sep 2008 07:44:49 +0800
>> From: Jason Rumney <jasonr@gnu.org>
>> CC: 865@emacsbugs.donarmstrong.com, emacs-pretest-bug@gnu.org
>>
>>     
>>>   (or (eql (nth 2 attrs) (user-uid))
>>>       (and (eq system-type 'windows-nt) (eql (nth 2 attrs) (user-gid)))
>>>   
>>>       
>> I am trying to find a solution that will work without Lisp coders 
>> needing to be aware of the strange situation on Windows that files can 
>> be owned by a group.
>>     
>
> I think the only good solution in that venue would be to have a new
> primitive for such tests, and implement it on Windows so that it will
> cater to this special case.  Any other solution is likely to lie about
> the file's security attributes.
>   

We are telling a lie already by attempting to map ACLs to POSIX 
ownership. If we are going to attempt this, then it is better to tell a 
lie that fits with the expectations of POSIX (ie that owners of files 
are individual users). A different API that tells the full story might 
be a good idea, but any new API is only going to be used by future 
developers who know about the API, it cannot fix problems in existing 
Lisp code that might make the same assumptions as server.el.

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Jason Rumney wrote:

> Eli Zaretskii wrote:
>>> From: Jason Rumney

>>>>   (or (eql (nth 2 attrs) (user-uid))
>>>>       (and (eq system-type 'windows-nt) (eql (nth 2 attrs) (user-gid)))

>>> I am trying to find a solution that will work without Lisp coders needing to
>>> be aware of the strange situation on Windows that files can be owned by a
>>> group.

>> I think the only good solution in that venue would be to have a new
>> primitive for such tests, and implement it on Windows so that it will
>> cater to this special case.  Any other solution is likely to lie about
>> the file's security attributes.

> We are telling a lie already by attempting to map ACLs to POSIX ownership. If we
> are going to attempt this, then it is better to tell a lie that fits with the
> expectations of POSIX (ie that owners of files are individual users).

Doesn't POSIX define both a user owner and group owner for each file?
Could a Windows file owned by a group be mapped to POSIX by showing the
Windows group as the POSIX group owner and a pseudo-user (e.g., nobody,
root, emacs) as the user owner?
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Thu, 04 Sep 2008 11:30:57 +0800
> From: Jason Rumney <jasonr@gnu.org>
> CC: 865@emacsbugs.donarmstrong.com, emacs-pretest-bug@gnu.org
> 
> > I think the only good solution in that venue would be to have a new
> > primitive for such tests, and implement it on Windows so that it will
> > cater to this special case.  Any other solution is likely to lie about
> > the file's security attributes.
> >   
> 
> We are telling a lie already by attempting to map ACLs to POSIX 
> ownership.

The current lie is relatively small: it is limited to the
interpretation of the group ID, while the owner of the file is
displayed and interpreted correctly.

> If we are going to attempt this, then it is better to tell a lie
> that fits with the expectations of POSIX (ie that owners of files
> are individual users).

The problem is, I don't see how we can do this reliably.  In this
particular case, Emacs compares the file's ownership with the UID of
the user running Emacs, so we could pretend the file is owned by that
user.  But in other cases, Emacs could compare the ownership to a UID
of some other user, and what will we do then?

That said, I won't necessarily object if you find a clever way out of
this that leaves existing Lisp code intact.

> A different API that tells the full story might be a good idea, but
> any new API is only going to be used by future developers who know
> about the API, it cannot fix problems in existing Lisp code that
> might make the same assumptions as server.el.

We are talking about Emacs 23, so it's not too late to find all those
places (only in the Emacs distro, of course) and fix them.  I just
looked, there aren't so many of them.  And server.el should have an
entirely different code to do what it wants on Windows, anyway.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Francis Litterio <flitterio@gmail.com>
> Date: Thu, 04 Sep 2008 11:56:39 -0400
> Cc: 
> 
> Could a Windows file owned by a group be mapped to POSIX by showing the
> Windows group as the POSIX group owner and a pseudo-user (e.g., nobody,
> root, emacs) as the user owner?

How will this help the following code work on Windows, when the owner
of the file is reported as the Administrators group (whose ID is 544),
and the UID of the user who created the file is different (because she
belongs to the local admins group)?

  (or (eql (nth 2 (file-attributes FILE)) (user-uid))
      (scream-bloody-murder))

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Eli Zaretskii wrote:

>> From: Francis Litterio <flitterio@gmail.com>
>> 
>> Could a Windows file owned by a group be mapped to POSIX by showing the
>> Windows group as the POSIX group owner and a pseudo-user (e.g., nobody,
>> root, emacs) as the user owner?
>
> How will this help the following code work on Windows, when the owner
> of the file is reported as the Administrators group (whose ID is 544),
> and the UID of the user who created the file is different (because she
> belongs to the local admins group)?
>
>   (or (eql (nth 2 (file-attributes FILE)) (user-uid))
>       (scream-bloody-murder))

My suggestion wouldn't help in this case, because it would cause (nth 2
(file-attributes ...)) to return a value that does not match any
existing user.

But if Windows says the file is owned by the Local Administrators group,
hasn't the information about which user created it been lost?  The file
is not truly owned by any single user.

Thus, (nth 2 (file-attributes ...)) can never return a meaningful
existing user ID for a file that is owned by a group.
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Francis Litterio wrote:
> Doesn't POSIX define both a user owner and group owner for each file?
> Could a Windows file owned by a group be mapped to POSIX by showing the
> Windows group as the POSIX group owner and a pseudo-user (e.g., nobody,
> root, emacs) as the user owner?
>   

Under POSIX, the user is the owner, and the group is there for assigning 
wider permissions for a file without opening it to the world. I've never 
heard it referred to as group owner, and it is different than the 
situation on Windows where an ACL may contain a group as the 
creator/owner of a file.

Making the owner `nobody' would not fix the problem that the current ACL 
mapping causes. I would surmise that the only use of these fields that 
lisp code has is to test if the current user owns a file. Lisp code does 
not know about the other users or groups that are configured on the 
system, so it isn't going to care about exactly who the other users are. 
Under Windows, where the owner can be a group, if the current user is a 
member of the group that owns a file, then they effectively own that 
file, and Emacs should report that to avoid problems with Lisp code that 
is trying to test ownership. In the case where they aren't a member of 
that group, I don't think it causes problems to report the group as the 
owner, since to Lisp code, any other owner means "not me".

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Eli Zaretskii wrote:

> The current lie is relatively small: it is limited to the
> interpretation of the group ID, while the owner of the file is
> displayed and interpreted correctly.
>
>   
>> If we are going to attempt this, then it is better to tell a lie
>> that fits with the expectations of POSIX (ie that owners of files
>> are individual users).
>>     
>
> The problem is, I don't see how we can do this reliably.  In this
> particular case, Emacs compares the file's ownership with the UID of
> the user running Emacs, so we could pretend the file is owned by that
> user.  But in other cases, Emacs could compare the ownership to a UID
> of some other user, and what will we do then?
>   

I can't imagine any Lisp code doing this, as it will not know which 
other users are on the system. The only use for checking the owner seems 
to be to check if the current user is the owner. Rather than a 
technically correct mapping of ownership to groups, I think it is better 
for users if the mapping reflects whether the current user has ownership 
(individually or through a group they are a member of) so that such 
binary tests can work.

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Jason Rumney wrote:
> Francis Litterio wrote:
>> Doesn't POSIX define both a user owner and group owner for each file?
>> Could a Windows file owned by a group be mapped to POSIX by showing the
>> Windows group as the POSIX group owner and a pseudo-user (e.g., nobody,
>> root, emacs) as the user owner?
>>   
> 
> Under POSIX, the user is the owner, and the group is there for assigning
> wider permissions for a file without opening it to the world. I've never
> heard it referred to as group owner, and it is different than the
> situation on Windows where an ACL may contain a group as the
> creator/owner of a file.

Is the owner really in the ACL? Is not owner a separate property on w32?

I do not remember how these are related any more.

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Jason Rumney wrote:
> Francis Litterio wrote:
>> Doesn't POSIX define both a user owner and group owner for each file?
>> Could a Windows file owned by a group be mapped to POSIX by showing the
>> Windows group as the POSIX group owner and a pseudo-user (e.g., nobody,
>> root, emacs) as the user owner?
>>   
> 
> Under POSIX, the user is the owner, and the group is there for assigning
> wider permissions for a file without opening it to the world. I've never
> heard it referred to as group owner, and it is different than the
> situation on Windows where an ACL may contain a group as the
> creator/owner of a file.


I searched a bit for a good explanation of owner and ACL on w32. This
page looks ok to me:

  http://setacl.sourceforge.net/html/doc-basics.html

Notice that access is controled by the ACLs, not the owner. The owner
has however the right to change the ACLs.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

>> If we are going to attempt this, then it is better to tell a lie
>> that fits with the expectations of POSIX (ie that owners of files
>> are individual users).

> The problem is, I don't see how we can do this reliably.  In this
> particular case, Emacs compares the file's ownership with the UID of
> the user running Emacs, so we could pretend the file is owned by that
> user.  But in other cases, Emacs could compare the ownership to a UID
> of some other user, and what will we do then?

I think the problem comes earlier: the (let ((default-file-modes ?\700))
should make sure that the directory created there is owned by the use
and not by some Administator group.

Of course, on FAT there's just nothing we can do and the
server-ensure-safe-dir functionality simply cannot be provided, so we
should then just skip the safety checks,


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Francis Litterio <flitterio@gmail.com>
> Date: Thu, 04 Sep 2008 14:08:36 -0400
> Cc: 
> 
> But if Windows says the file is owned by the Local Administrators group,
> hasn't the information about which user created it been lost?  The file
> is not truly owned by any single user.
> 
> Thus, (nth 2 (file-attributes ...)) can never return a meaningful
> existing user ID for a file that is owned by a group.

Well, that's the problem we are trying to solve...

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Fri, 05 Sep 2008 06:51:30 +0800
> From: Jason Rumney <jasonr@gnu.org>
> CC: 865@emacsbugs.donarmstrong.com, emacs-pretest-bug@gnu.org
> 
> > But in other cases, Emacs could compare the ownership to a UID
> > of some other user, and what will we do then?
> >   
> 
> I can't imagine any Lisp code doing this, as it will not know which 
> other users are on the system.

It's quite easy to enumerate users defined on a system, both on Unix
and Windows.  Also, the user ID could be recorded in a file.  See, for
example, vc-rcs.el, where it checks whether the current user is the
one who locked the file.  (We are lucky that it compares user _names_
rather than UIDs.)

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman]

On 9/5/08, Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Francis Litterio <flitterio@gmail.com>
> > Date: Thu, 04 Sep 2008 14:08:36 -0400
> > Cc:
> >
> > But if Windows says the file is owned by the Local Administrators group,
> > hasn't the information about which user created it been lost?  The file
> > is not truly owned by any single user.
> >
> > Thus, (nth 2 (file-attributes ...)) can never return a meaningful
> > existing user ID for a file that is owned by a group.
>
> Well, that's the problem we are trying to solve...

I think Stefan is suggesting what I suggested before: Change the owner.

But that does not necessarily help since it is the ACL lists that
tells who has access to the directory, not the owner.

Some comments on what is going on there so we can move forward to a
reasonable suggestion?

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Fri, 05 Sep 2008 06:46:35 +0800
> From: Jason Rumney <jasonr@gnu.org>
> Cc: 
> 
> Under Windows, where the owner can be a group, if the current user is a 
> member of the group that owns a file, then they effectively own that 
> file, and Emacs should report that to avoid problems with Lisp code that 
> is trying to test ownership.

How about solving this the other way around: if the current user is a
member of the Administators group, Emacs on Windows will return that
group's ID (544) as user-uid of such a user?

Specifically, my suggestion is the following API change:

   user-uid is a built-in function in `C source code'.
   (user-uid &optional real-id)

   Return the effective UID of the user who runs this Emacs session.
   Value is an integer or float, depending on the value.

   On MS-Windows, if the user belongs to the "Administrators" group,
   that group's ID is returned instead of the user's ID.  This is to
   match MS-Windows policy to attribute ownership of files created by
   such users to the Administrators group.  If REAL-ID is non-nil, return
   the real UID of the user even if it belongs to the Administrators
   group.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  Jason Rumney <jasonr@gnu.org>,  emacs-pretest-bug@gnu.org
> Date: Thu, 04 Sep 2008 23:11:10 -0400
> 
> I think the problem comes earlier: the (let ((default-file-modes ?\700))
> should make sure that the directory created there is owned by the use
> and not by some Administator group.

That's a different problem.  I don't see how it can be solved without
introducing a new primitive, which on Windows will DTRT.  (I think
GNU/Linux and Unix systems that support ACLs will need a similar
primitive, but I don't know enough about those to say for sure.)

There are a few other places in Emacs other than server.el that make
similar tests, for reasons other than making sure the file/directory
is private to the current user.  Here's the list:

 files.el:file-ownership-preserved-p
 eshell/em-ls.el:eshell-ls-applicable
 net/ange-ftp.el:ange-ftp-parse-netrc

(the last one is actually quite similar to server.el).

> Of course, on FAT there's just nothing we can do and the
> server-ensure-safe-dir functionality simply cannot be provided, so we
> should then just skip the safety checks,

On FAT, all files belong to a user called Everyone, who has a special
UID of 0, so I think all these checks will simply pass, or at least
they should.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

>> I think the problem comes earlier: the (let ((default-file-modes ?\700))
>> should make sure that the directory created there is owned by the use
>> and not by some Administator group.

> That's a different problem.  I don't see how it can be solved without
> introducing a new primitive, which on Windows will DTRT.  (I think
> GNU/Linux and Unix systems that support ACLs will need a similar
> primitive, but I don't know enough about those to say for sure.)

> There are a few other places in Emacs other than server.el that make
> similar tests, for reasons other than making sure the file/directory
> is private to the current user.  Here's the list:

>  files.el:file-ownership-preserved-p
>  eshell/em-ls.el:eshell-ls-applicable
>  net/ange-ftp.el:ange-ftp-parse-netrc

> (the last one is actually quite similar to server.el).

I think we need to consider the testing part and the file (or dir)
creation part separately.  And my previous messages pointed out that the
core problem (securitywise) is in the creation part, which is hence
unrelated to the above 3 other cases.

>> Of course, on FAT there's just nothing we can do and the
>> server-ensure-safe-dir functionality simply cannot be provided, so we
>> should then just skip the safety checks,

> On FAT, all files belong to a user called Everyone, who has a special
> UID of 0, so I think all these checks will simply pass, or at least
> they should.

No, they shouldn't, since the tests are meant to ensure that only the
current user has write access, whereas FAT cannot provide this behavior.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman]

On 9/5/08, Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: 865@emacsbugs.donarmstrong.com,  Jason Rumney <jasonr@gnu.org>,  emacs-pretest-bug@gnu.org
> > Date: Thu, 04 Sep 2008 23:11:10 -0400
> >
> > I think the problem comes earlier: the (let ((default-file-modes ?\700))
> > should make sure that the directory created there is owned by the use
> > and not by some Administator group.
>
> That's a different problem.  I don't see how it can be solved without
> introducing a new primitive, which on Windows will DTRT.  (I think
> GNU/Linux and Unix systems that support ACLs will need a similar
> primitive, but I don't know enough about those to say for sure.)

I thought that perhaps a workaround would be to changed the owner so I tested.

I did a test on the directory where I had trouble. I did something like

  (set-file-modes "serv2" ?\654)

CACLS still reports the same ACL entries.

I thought that maybe was because the owner is the group
Administrators. However changing the owner to me and trying
set-file-modes again gave the same result.

Am I perhaps not using set-file-modes correctly? (I get no errors.)

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Eli Zaretskii wrote:

>> From: Jason Rumney

>> Under Windows, where the owner can be a group, if the current user is a 
>> member of the group that owns a file, then they effectively own that 
>> file, and Emacs should report that to avoid problems with Lisp code that 
>> is trying to test ownership.
>
> How about solving this the other way around: if the current user is a
> member of the Administators group, Emacs on Windows will return that
> group's ID (544) as user-uid of such a user?

If this solution is implemented, keep in mind that there is a Group
Policy setting called "System objects: default owner for objects created
by members of Administrators group" which can be set to either
"Administrators group" or "Object creator".  If it is set to the latter
value, then newly-created files are owned by the user not the group.

And on a domain-based machine, this can be a domain policy that can't be
changed even by a member of the machine's Local Administrators group.
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Fri, 5 Sep 2008 15:18:55 +0200
> From: "Lennart Borgman" <lennart.borgman@gmail.com>
> Cc: "Stefan Monnier" <monnier@iro.umontreal.ca>, emacs-pretest-bug@gnu.org
> 
> I did a test on the directory where I had trouble. I did something like
> 
>   (set-file-modes "serv2" ?\654)
> 
> CACLS still reports the same ACL entries.
> 
> I thought that maybe was because the owner is the group
> Administrators. However changing the owner to me and trying
> set-file-modes again gave the same result.
> 
> Am I perhaps not using set-file-modes correctly? (I get no errors.)

set-file-modes is only very crudely emulated on Windows.  In a
nutshell, the underlying primitive _chmod only considers the high 3
bits of the argument you pass to set-file-modes.

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Lennart Borgman wrote:
> I thought that perhaps a workaround would be to changed the owner so I tested.
>
> I did a test on the directory where I had trouble. I did something like
>
>   (set-file-modes "serv2" ?\654)
>   

set-file-modes is just a wrapper for the chmod system function. On 
Windows, the chmod system function ignores all fields in the bitmask 
except the owner read and write flags, which it maps to the DOS readonly 
flag on files.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  jasonr@gnu.org,  emacs-pretest-bug@gnu.org
> Date: Fri, 05 Sep 2008 09:16:51 -0400
> 
> I think we need to consider the testing part and the file (or dir)
> creation part separately.

100% agreement.

> And my previous messages pointed out that the core problem
> (securitywise) is in the creation part, which is hence unrelated to
> the above 3 other cases.

Are you okay with adding a primitive for solving the creation part,
and will such a primitive be allowed into the repository even though
we are in feature freeze?

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman]

On 9/5/08, Jason Rumney <jasonr@gnu.org> wrote:
> Lennart Borgman wrote:
> > I thought that perhaps a workaround would be to changed the owner so I
> tested.
> >
> > I did a test on the directory where I had trouble. I did something like
> >
> >  (set-file-modes "serv2" ?\654)
> >
> >
>
> set-file-modes is just a wrapper for the chmod system function. On Windows,
> the chmod system function ignores all fields in the bitmask except the owner
> read and write flags, which it maps to the DOS readonly flag on files.

Thanks. But then we can not make very much on w32, can we?

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

> set-file-modes is only very crudely emulated on Windows.  In a
> nutshell, the underlying primitive _chmod only considers the high 3
> bits of the argument you pass to set-file-modes.

So I guess that's pretty much the source of the problem (modulo the
fact that we set default-file-modes rather than calling set-file-modes).


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Francis Litterio <flitterio@gmail.com>
> Date: Fri, 05 Sep 2008 10:23:27 -0400
> Cc: 
> 
> If this solution is implemented, keep in mind that there is a Group
> Policy setting called "System objects: default owner for objects created
> by members of Administrators group" which can be set to either
> "Administrators group" or "Object creator".  If it is set to the latter
> value, then newly-created files are owned by the user not the group.

Thanks.  Do you know which API can be used to find out
programmatically whether this setting is one or the other?

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> Date: Fri, 5 Sep 2008 17:16:46 +0200
> From: "Lennart Borgman" <lennart.borgman@gmail.com>
> Cc: 865@emacsbugs.donarmstrong.com, "Eli Zaretskii" <eliz@gnu.org>, 
> 	emacs-pretest-bug@gnu.org
> 
> On 9/5/08, Jason Rumney <jasonr@gnu.org> wrote:
> > Lennart Borgman wrote:
> > > I thought that perhaps a workaround would be to changed the owner so I
> > tested.
> > >
> > > I did a test on the directory where I had trouble. I did something like
> > >
> > >  (set-file-modes "serv2" ?\654)
> > >
> > >
> >
> > set-file-modes is just a wrapper for the chmod system function. On Windows,
> > the chmod system function ignores all fields in the bitmask except the owner
> > read and write flags, which it maps to the DOS readonly flag on files.
> 
> Thanks. But then we can not make very much on w32, can we?

Of course, we can.  We just need to use native NT security APIs, not
the emulated Posix ones.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Lennart Borgman <lennart.borgman@gmail.com>,
>         865@emacsbugs.donarmstrong.com, emacs-pretest-bug@gnu.org
> Date: Fri, 05 Sep 2008 11:34:58 -0400
> 
> > set-file-modes is only very crudely emulated on Windows.  In a
> > nutshell, the underlying primitive _chmod only considers the high 3
> > bits of the argument you pass to set-file-modes.
> 
> So I guess that's pretty much the source of the problem

The source of the problem is that Posix rwxrwxrwx mode bits don't map
well to Windows ACL-based file security.  Even if I would sit down to
rewrite _chmod to preserve Posix semantics, I'd have difficulty doing
that because there's no clear distinction between "group" and "world",
and because the set of Windows access bits is larger than just rwx.
(Cygwin jumps through the hoops to make Posix semantics happen, but
even they require that the user's group be defined for this to work,
and generally that you play by Posix rules.)

Instead of assuming Posix semantics, we should explicitly have a
primitive that creates files that can only be accessed by their
creator; on Posix platforms the implementation could do what we do in
server.el now.

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Eli Zaretskii wrote:

>> From: Francis Litterio

>> If this solution is implemented, keep in mind that there is a Group
>> Policy setting called "System objects: default owner for objects created
>> by members of Administrators group" which can be set to either
>> "Administrators group" or "Object creator".  If it is set to the latter
>> value, then newly-created files are owned by the user not the group.
>
> Thanks.  Do you know which API can be used to find out
> programmatically whether this setting is one or the other?

You had to ask. :) Some hours of Googling and reading bad MS
documentation reveals that Windows sets the following registry value to
0 or 1 to reflect that particular policy:

  Key: HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Lsa
  Value: NoDefaultAdminOwner

If it is 0, then new files created by members of Local Administrators
are owned by the group, otherwise they are owned by the user.

Some caveats:

1. I think that changing this value may have no effect on a machine in a
   domain with restrictive domain policies, but that doesn't affect
   Emacs, which would only read it.

2. This value might not exist in the registry on Windows versions prior
   to Windows XP.  That may be a bigger issue for Emacs.

Hope this helps.
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

>> I think we need to consider the testing part and the file (or dir)
>> creation part separately.

> 100% agreement.

>> And my previous messages pointed out that the core problem
>> (securitywise) is in the creation part, which is hence unrelated to
>> the above 3 other cases.

> Are you okay with adding a primitive for solving the creation part,
> and will such a primitive be allowed into the repository even though
> we are in feature freeze?

I'm not sure what it would look like so it's hard for me to say.
But I'd argue that having the umask (aka default-file-modes) set to
#o700 could be used as a tell-tale sign, so it sounds to me like it
might be doable by adding w32 C code without any C-level changes.

Of course, that might just be a reflection of my naive misunderstanding
of the w32 API.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Francis Litterio wrote:
> Eli Zaretskii wrote:
> 
>>> From: Francis Litterio
> 
>>> If this solution is implemented, keep in mind that there is a Group
>>> Policy setting called "System objects: default owner for objects created
>>> by members of Administrators group" which can be set to either
>>> "Administrators group" or "Object creator".  If it is set to the latter
>>> value, then newly-created files are owned by the user not the group.
>> Thanks.  Do you know which API can be used to find out
>> programmatically whether this setting is one or the other?
> 
> You had to ask. :) Some hours of Googling and reading bad MS
> documentation reveals that Windows sets the following registry value to
> 0 or 1 to reflect that particular policy:
> 
>   Key: HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Lsa
>   Value: NoDefaultAdminOwner
> 
> If it is 0, then new files created by members of Local Administrators
> are owned by the group, otherwise they are owned by the user.

Is this the recommended way to check it? Quite often I have seen people
on Internet claiming that you should look for a registry value when the
documentation from MS clearly say you should use an API instead.

> Some caveats:
> 
> 1. I think that changing this value may have no effect on a machine in a
>    domain with restrictive domain policies, but that doesn't affect
>    Emacs, which would only read it.
> 
> 2. This value might not exist in the registry on Windows versions prior
>    to Windows XP.  That may be a bigger issue for Emacs.
> 
> Hope this helps.
> --
> Fran
> 
> 
> 
> 
> 
> 

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Lennart Borgman (gmail) wrote:

> Francis Litterio wrote:
>> Eli Zaretskii wrote:
>>
>>> Thanks.  Do you know which API can be used to find out
>>> programmatically whether this setting is one or the other?
>> 
>> You had to ask. :) Some hours of Googling and reading bad MS
>> documentation reveals that Windows sets the following registry value to
>> 0 or 1 to reflect that particular policy:
>> 
>>   Key: HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Lsa
>>   Value: NoDefaultAdminOwner
>> 
>> If it is 0, then new files created by members of Local Administrators
>> are owned by the group, otherwise they are owned by the user.
>
> Is this the recommended way to check it? Quite often I have seen people
> on Internet claiming that you should look for a registry value when the
> documentation from MS clearly say you should use an API instead.

The documentation from MS on how to query policy settings is poorly
organized.  There is a function named GetGPOList() documented at:

  http://msdn.microsoft.com/en-us/library/aa373520(VS.85).aspx

but it isn't clear to me how one extracts a specific policy setting from
the returned GPO list.  There's a structure named GROUP_POLICY_OBJECT
documented at:

  http://msdn.microsoft.com/en-us/library/aa374173(VS.85).aspx

but it's not clear to me how that structure represents policy settings.

There is a different (newer?) API called RSOP (Resulting Set of
Policies) that's part of the WMI (Windows Management Infrastructure)
API, but it doesn't appear to be directly callable from C code.  It's
documented at:

  http://msdn.microsoft.com/en-us/library/aa375082(VS.85).aspx

The overall Group Policy architecture is documented at:

  http://msdn.microsoft.com/en-us/library/aa373783(VS.85).aspx

where it is implied that group policies only exist on a domain, which
leads me to wonder if any of the above APIs work on non-domain
machines.

Confusing.
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  jasonr@gnu.org,  emacs-pretest-bug@gnu.org
> Date: Fri, 05 Sep 2008 17:36:40 -0400
> 
> But I'd argue that having the umask (aka default-file-modes) set to
> #o700 could be used as a tell-tale sign, so it sounds to me like it
> might be doable by adding w32 C code without any C-level changes.

So you are saying we should assume that when umask has its two lower
mode bits set to zero, the intent is to create a private file
accessible only by the user who runs Emacs?  I don't like such
assumptions, but if I'm the only one, so be it.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Francis Litterio <flitterio@gmail.com>
> Date: Fri, 05 Sep 2008 16:29:04 -0400
> Cc: 
> 
> > Thanks.  Do you know which API can be used to find out
> > programmatically whether this setting is one or the other?
> 
> You had to ask. :) Some hours of Googling and reading bad MS
> documentation reveals that Windows sets the following registry value to
> 0 or 1 to reflect that particular policy:

Thanks a lot for the info and the footwork.  I should have said more
clearly that I only meant to ask you about what you already knew;
sorry about that.

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Eli Zaretskii wrote:

>> From: Stefan Monnier

>> But I'd argue that having the umask (aka default-file-modes) set to
>> #o700 could be used as a tell-tale sign, so it sounds to me like it
>> might be doable by adding w32 C code without any C-level changes.
>
> So you are saying we should assume that when umask has its two lower
> mode bits set to zero, the intent is to create a private file
> accessible only by the user who runs Emacs?  I don't like such
> assumptions, but if I'm the only one, so be it.

Overloading the semantics of a subset of the bits in the umask seems
prone to confusion.  Why not create a new w32-... variable to encode
those semantics?
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Francis Litterio wrote:
> Eli Zaretskii wrote:
> 
>>> From: Stefan Monnier
> 
>>> But I'd argue that having the umask (aka default-file-modes) set to
>>> #o700 could be used as a tell-tale sign, so it sounds to me like it
>>> might be doable by adding w32 C code without any C-level changes.
>> So you are saying we should assume that when umask has its two lower
>> mode bits set to zero, the intent is to create a private file
>> accessible only by the user who runs Emacs?  I don't like such
>> assumptions, but if I'm the only one, so be it.
> 
> Overloading the semantics of a subset of the bits in the umask seems
> prone to confusion.  Why not create a new w32-... variable to encode
> those semantics?


Unfortunately they are already overloaded on w32. I think the best
remedy would be to just remove that on w32. New primitives are needed if
we really want to handle security from within Emacs.

I am not sure it is good to do that, but if you really want to handle
security it must of course be carefully done.

For the current problem a work around using a special function in
server-ensure-safe-dir for OS:es that uses ACLs for security control
would be the best in my opinion.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

>> But I'd argue that having the umask (aka default-file-modes) set to
>> #o700 could be used as a tell-tale sign, so it sounds to me like it
>> might be doable by adding w32 C code without any C-level changes.
                                                   ^^^
                                                  Elisp

> So you are saying we should assume that when umask has its two lower
> mode bits set to zero, the intent is to create a private file
> accessible only by the user who runs Emacs?  I don't like such
> assumptions, but if I'm the only one, so be it.

It's not an assumption: it's what default-file-modes says.
If it's #oX00, it means that nobody else than the user who creates it
should have any access rights to it.  The X may specify additional
constraints so that even the creating user may not have read or write
(or exec) access, but these are less important.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  jasonr@gnu.org,  emacs-pretest-bug@gnu.org
> Date: Sat, 06 Sep 2008 15:45:29 -0400
> 
> > So you are saying we should assume that when umask has its two lower
> > mode bits set to zero, the intent is to create a private file
> > accessible only by the user who runs Emacs?  I don't like such
> > assumptions, but if I'm the only one, so be it.
> 
> It's not an assumption: it's what default-file-modes says.

On Posix platforms, yes.  The MS-Windows access right set is richer
than just Read, Write, Execute.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

>> > So you are saying we should assume that when umask has its two lower
>> > mode bits set to zero, the intent is to create a private file
>> > accessible only by the user who runs Emacs?  I don't like such
>> > assumptions, but if I'm the only one, so be it.
>> It's not an assumption: it's what default-file-modes says.
> On Posix platforms, yes.

No, it's not platform dependent: it just so happens that it matches the
Posix semantics, so the implementation is trivial on Posix platforms,
but the meaning of this Elisp code is the same whether or not it's
running on a Posix platform.

> The MS-Windows access right set is richer than just Read,
> Write, Execute.

That doesn't really matter, the point is that nobody but the creating
user should have any right.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Francis Litterio]

Stefan Monnier wrote:

>> The MS-Windows access right set is richer than just Read,
>> Write, Execute.
>
> That doesn't really matter, the point is that nobody but the creating
> user should have any right.

Would it be acceptable for Emacs on Windows to change the ownership of
any file it creates to be the user running Emacs, and then set the ACLs
such that the creating user is the only user with rights to the file?
--
Fran

bug#865: 23.0.60; The directory is unsafe today

[Lennart Borgman (gmail)]

Francis Litterio wrote:
> Stefan Monnier wrote:
> 
>>> The MS-Windows access right set is richer than just Read,
>>> Write, Execute.
>> That doesn't really matter, the point is that nobody but the creating
>> user should have any right.
> 
> Would it be acceptable for Emacs on Windows to change the ownership of
> any file it creates to be the user running Emacs, and then set the ACLs
> such that the creating user is the only user with rights to the file?


I think that is what Stefan means.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  jasonr@gnu.org,  emacs-pretest-bug@gnu.org
> Date: Sat, 06 Sep 2008 23:43:52 -0400
> 
> > The MS-Windows access right set is richer than just Read,
> > Write, Execute.
> 
> That doesn't really matter, the point is that nobody but the creating
> user should have any right.

This is impossible on Windows, AFAIK.  There are special flags to the
syscall that opens a file or directory that can bypass any denied
rights to enter a directory or open a file.  (These flags allegedly
exist so that system backup and restore programs could DTRT without
running as a privileged user.)

So, if we are to emulate this special value of mode-bits on Windows,
we need to decide exactly what rights we need to deny.

Or maybe a better implementation for Windows would be to create
encrypted files.  Or maybe something else entirely; that's why I was
asking questions about the semantics of this.

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Francis Litterio <flitterio@gmail.com>
> Date: Sun, 07 Sep 2008 11:44:50 -0400
> Cc: 
> 
> Would it be acceptable for Emacs on Windows to change the ownership of
> any file it creates to be the user running Emacs, and then set the ACLs
> such that the creating user is the only user with rights to the file?

Do you really mean to do that for _any_ file created by Emacs?  That's
hardly appropriate: do we really want that no one else could access
the files we create with Emacs?  Not even the Unix version does that,
unless default-file-modes say we should.

This discussion was only about the directory used by the Emacs server,
and any files in that directory.  I don't think anyone meant we should
do this for every file Emacs creates.

If you meant to suggest a way of doing this just for that single
directory, then yes, this is what we were discussing.  If you can
propose code to do that, please do.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

> This is impossible on Windows, AFAIK.  There are special flags to the
> syscall that opens a file or directory that can bypass any denied
> rights to enter a directory or open a file.  (These flags allegedly
> exist so that system backup and restore programs could DTRT without
> running as a privileged user.)

Are you saying that anybody can read any file (or dir) simply by using
those extra flags when they open those files and dirs?  So there's no
possible privacy between users on the same machine?  If so, we may just
stop to worry about server-ensure-safe-dir under w32 since there's
simply no way for it to be safe (short of encrypting it, which implies
a fairly different UI).


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Jason Rumney]

Stefan Monnier wrote:
>> This is impossible on Windows, AFAIK.  There are special flags to the
>> syscall that opens a file or directory that can bypass any denied
>> rights to enter a directory or open a file.  (These flags allegedly
>> exist so that system backup and restore programs could DTRT without
>> running as a privileged user.)
>>     
>
> Are you saying that anybody can read any file (or dir) simply by using
> those extra flags when they open those files and dirs?  So there's no
> possible privacy between users on the same machine?  If so, we may just
> stop to worry about server-ensure-safe-dir under w32 since there's
> simply no way for it to be safe (short of encrypting it, which implies
> a fairly different UI).
>   

I don't know the full details of NTFS security, but there is a special 
group called "Backup Users", which I would expect to be linked with the 
use of these flags somehow.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

> I don't know the full details of NTFS security, but there is a special group
> called "Backup Users", which I would expect to be linked with the use of
> these flags somehow.

Then it's OK.  Unix has similar features, but they don't matter for
server.el's purpose.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  jasonr@gnu.org,  emacs-pretest-bug@gnu.org
> Date: Sun, 07 Sep 2008 23:33:28 -0400
> 
> > This is impossible on Windows, AFAIK.  There are special flags to the
> > syscall that opens a file or directory that can bypass any denied
> > rights to enter a directory or open a file.  (These flags allegedly
> > exist so that system backup and restore programs could DTRT without
> > running as a privileged user.)
> 
> Are you saying that anybody can read any file (or dir) simply by using
> those extra flags when they open those files and dirs?  So there's no
> possible privacy between users on the same machine?  If so, we may just
> stop to worry about server-ensure-safe-dir under w32 since there's
> simply no way for it to be safe (short of encrypting it, which implies
> a fairly different UI).

Not exactly: most programs don't use these special flags, and some of
them seem to require special privileges, although I'm not quite sure
who can gain those privileges.  (A small test program confirmed that I
can gain them, even though I'm not in the Administrators group.)

See:

  http://msdn.microsoft.com/en-us/library/aa364399(VS.85).aspx

for more details.

But I don't think we should dismiss the privacy issue just because it
can be bypassed by an ill meaning program: the same can happen on
Unix, given a program that deliberately gains root access.  "Normal"
programs don't use those special access flags and privileges, and so
cannot access files in a private directory.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

> Not exactly: most programs don't use these special flags, and some of
> them seem to require special privileges, although I'm not quite sure
> who can gain those privileges.  (A small test program confirmed that I
> can gain them, even though I'm not in the Administrators group.)

> See:

>   http://msdn.microsoft.com/en-us/library/aa364399(VS.85).aspx

> for more details.

> But I don't think we should dismiss the privacy issue just because it
> can be bypassed by an ill meaning program: the same can happen on
> Unix, given a program that deliberately gains root access.  "Normal"
> programs don't use those special access flags and privileges, and so
> cannot access files in a private directory.

Huh?  Those programs that can deliberately gain root access are kept
under very tight control.  For a normal user to be able to read
arbitrary files on the system is considered as a major security hole on
unixy systems (even if she has to go through contortions to do that).


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 865@emacsbugs.donarmstrong.com,  jasonr@gnu.org,  emacs-pretest-bug@gnu.org
> Date: Tue, 09 Sep 2008 10:37:57 -0400
> 
> > But I don't think we should dismiss the privacy issue just because it
> > can be bypassed by an ill meaning program: the same can happen on
> > Unix, given a program that deliberately gains root access.  "Normal"
> > programs don't use those special access flags and privileges, and so
> > cannot access files in a private directory.
> 
> Huh?  Those programs that can deliberately gain root access are kept
> under very tight control.  For a normal user to be able to read
> arbitrary files on the system is considered as a major security hole on
> unixy systems (even if she has to go through contortions to do that).

I'm not going to argue about merits and demerits of Unix vs Windows
wrt security.  My point was that using a private directory in
server.el is important on Windows even if you think its security level
is lower than that of Unix systems.  And I hoped that you'd provide
some guidance for implementing this on Windows.

bug#865: 23.0.60; The directory is unsafe today

[Stefan Monnier]

>> > But I don't think we should dismiss the privacy issue just because it
>> > can be bypassed by an ill meaning program: the same can happen on
>> > Unix, given a program that deliberately gains root access.  "Normal"
>> > programs don't use those special access flags and privileges, and so
>> > cannot access files in a private directory.
>> 
>> Huh?  Those programs that can deliberately gain root access are kept
>> under very tight control.  For a normal user to be able to read
>> arbitrary files on the system is considered as a major security hole on
>> unixy systems (even if she has to go through contortions to do that).

> I'm not going to argue about merits and demerits of Unix vs Windows
> wrt security.  My point was that using a private directory in
> server.el is important on Windows even if you think its security level
> is lower than that of Unix systems.

I don't think it is, actually, so we violently agree.

> And I hoped that you'd provide some guidance for implementing this
> on Windows.

As mentioned, ideally the Emacs C code should notice when
default-file-modes is #o700 that the files&dirs should be created
"private", whatever that means in the w32 world.
Even better would be if each individual file-modes bits were interpreted,
but handling #o700 is all we really need for now.


        Stefan

bug#865: 23.0.60; The directory is unsafe today

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

"Lennart Borgman (gmail)" <lennart.borgman@gmail.com> writes:

> server-ensure-safe-dir complained today during server-start. Examining
> the values in server-ensure-safe-dir I found the following:

I don't know how to solve this bug, but I recently handled a similar bug
report from a macOS user[1] (still open, it's intermittent so very slow
to track down) and I found the error message rather unhelpful.  I'll
push the following to emacs-26 in a few days if there are no objections.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 3088 bytes --]

From b8e526e3861e64115f1458b2e53c2c0a838eb25d Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 27 Aug 2017 23:09:32 -0400
Subject: [PATCH] Make "unsafe directory" error message more informative
 (Bug#865)

* lisp/server.el (server-ensure-safe-dir): Produce a description for
each "unsafe" condition.
---
 lisp/server.el | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 8aafa1c257..33800a9868 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -525,30 +525,35 @@ server-ensure-safe-dir
     ;; Check that it's safe for use.
     (let* ((uid (nth 2 attrs))
 	   (w32 (eq system-type 'windows-nt))
-	   (safe (cond
-		  ((not (eq t (car attrs))) nil)  ; is a dir?
-		  ((and w32 (zerop uid))	  ; on FAT32?
-		   (display-warning
-		    'server
-		    (format-message "\
+           (unsafe (cond
+                    ((not (eq t (car attrs)))
+                     (format "it is a %s" (if (stringp (car attrs))
+                                              "symlink" "file")))
+                    ((and w32 (zerop uid)) ; on FAT32?
+                     (display-warning
+                      'server
+                      (format-message "\
 Using `%s' to store Emacs-server authentication files.
 Directories on FAT32 filesystems are NOT secure against tampering.
 See variable `server-auth-dir' for details."
-			    (file-name-as-directory dir))
-		    :warning)
-		   t)
-		  ((and (/= uid (user-uid))	  ; is the dir ours?
-			(or (not w32)
-			    ;; Files created on Windows by Administrator
-			    ;; (RID=500) have the Administrators (RID=544)
-			    ;; group recorded as the owner.
-			    (/= uid 544) (/= (user-uid) 500)))
-		   nil)
-		  (w32 t)			  ; on NTFS?
-		  (t				  ; else, check permissions
-		   (zerop (logand ?\077 (file-modes dir)))))))
-      (unless safe
-	(error "The directory `%s' is unsafe" dir)))))
+                                      (file-name-as-directory dir))
+                      :warning)
+                     nil)
+                    ((and (/= uid (user-uid)) ; is the dir ours?
+                          (or (not w32)
+                              ;; Files created on Windows by Administrator
+                              ;; (RID=500) have the Administrators (RID=544)
+                              ;; group recorded as the owner.
+                              (/= uid 544) (/= (user-uid) 500)))
+                     (format "it is not owned by you (owner = %s (%d))"
+                             (user-full-name (user-uid)) (user-uid)))
+                    (w32 nil)           ; on NTFS?
+                    ((/= 0 (logand ?\077 (file-modes dir)))
+                     (format "it is accessible by others (%03o)"
+                             (file-modes dir)))
+                    (t nil))))
+      (when unsafe
+        (error "`%s' is not a safe directory because %s" dir unsafe)))))
 
 (defun server-generate-key ()
   "Generate and return a random authentication key.
-- 
2.11.0


[-- Attachment #3: Type: text/plain, Size: 49 bytes --]


[1]: https://github.com/magit/magit/issues/3148

bug#865: 23.0.60; The directory is unsafe today

[Noam Postavsky]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> +                     (format "it is not owned by you (owner = %s (%d))"
> +                             (user-full-name (user-uid)) (user-uid)))

Oops, I put the wrong uid there.  Fixed in [1: d719ea6ad5].

> +      (when unsafe
> +        (error "`%s' is not a safe directory because %s" dir unsafe)))))

I also added aa call to expand-file-name here, as I recently saw a
case[2] where there was some confusion about which directory
"~/.emacs.d/server" refered to.

[1: d719ea6ad5]: 2017-10-21 15:19:14 -0400
  Another fix for unsafe directory error message (Bug#865)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d719ea6ad5419bba2b376384c370dc2744dc718f

[2]: https://github.com/magit/with-editor/issues/41