(In reply to s.wend from comment #0)
> Now we can recognize the wrong file permissions.
> # ls -alsh /etc/NetworkManager/system-connections
> 4,0K drwxr-xr-x+ 2 root root 4,0K Aug 10 10:47 .
> 4,0K drwxr-xr-x+ 8 root root 4,0K Jul 25 07:28 ..
> 4,0K -rw-r--r--  1 root root  447 Aug 10 10:47 wlantest-beta
> 
> Actually I'am not able to track down the problem and hope you have an idea.

I don't see anything wrong in the logs. What happens if you run:

 (umask 066; touch /etc/NetworkManager/system-connections/test-connection)

as root, which are the permissions of the new file?

Hi Benjamino,

I did so and the file mods didn't change at all.

root@geraet:/etc/NetworkManager/system-connections# ll
insgesamt 20
drwxr-xr-x+ 2 root root 4096 Aug 11 08:13 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
-rw-r--r--  1 root root  447 Aug 10 10:47 wlantest-beta
root@geraet:/etc/NetworkManager/system-connections# touch test_no_umask
root@geraet:/etc/NetworkManager/system-connections# ll
insgesamt 20
drwxr-xr-x+ 2 root root 4096 Aug 11 08:13 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
-rw-r--r--  1 root root  447 Aug 10 10:47 wlantest-beta
-rw-r--r--  1 root root    0 Aug 11 08:13 test_no_umask
root@geraet:/etc/NetworkManager/system-connections# umask 066; touch test_066_umask 
root@geraet:/etc/NetworkManager/system-connections# ll
insgesamt 20
drwxr-xr-x+ 2 root root 4096 Aug 11 08:13 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
-rw-r--r--  1 root root  447 Aug 10 10:47 wlantest-beta
-rw-r--r--  1 root root    0 Aug 11 08:13 test_066_umask
-rw-r--r--  1 root root    0 Aug 11 08:13 test_no_umask

I tried to find umask definitions inside profile or bashrc stuff without any hit.

(In reply to s.wend from comment #2)
> root@geraet:/etc/NetworkManager/system-connections# ll
> insgesamt 20
> drwxr-xr-x+ 2 root root 4096 Aug 11 08:13 ./
> drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../

The '+' after permissions indicates that there is an ACL set for the directory, which probably is changing the default permissions for new files. What does 'getfacl /etc/NetworkManager/system-connections' return?

# getfacl /etc/NetworkManager/system-connections
# file: etc/NetworkManager/system-connections
# owner: root
# group: root
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:other::r-x

The ACL is been ignored, because the touched files got a 644 mod. I would expect 755 cause of the ACL.

Can you try again removing the ACL? Which purpose does it serve?

Strange behavior! I removed the apperently unused ACL (apperently unused because of comment #4) and now it worked out.

# pwd
/etc/NetworkManager/system-connections
# ll
insgesamt 8
drwxr-xr-x+ 2 root root 4096 Aug 12 13:59 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
# setfacl -b .
# ll
insgesamt 8
drwxr-xr-x  2 root root 4096 Aug 12 13:59 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
# touch test_no_specific_umask
# ll
insgesamt 12
drwxr-xr-x  2 root root 4096 Aug 12 14:02 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
-rw-r--r--  1 root root    0 Aug 12 14:02 test_no_specific_umask
# umask 066; touch and_now_with_066
# ll
insgesamt 12
drwxr-xr-x  2 root root 4096 Aug 12 14:02 ./
drwxr-xr-x+ 8 root root 4096 Jul 25 07:28 ../
-rw-------  1 root root    0 Aug 12 14:02 and_now_with_066
-rw-r--r--  1 root root    0 Aug 12 14:02 test_no_specific_umask


Actually I can't get it.

The ACL is installed by the Ubuntu Unity Frontend as a matter of dependency.

We have a lot of 14.04.5 LTS systems installed which also have ACL for /etc/NetworkManager/system-connection and it works without any kind of trouble.

Hey,

I could figure out the problem via strace especially for files.

We used 14.04 LTS before, even with ACLs, without any kind of problems. The right problem emerged with 16.04.

So we compared the straces while creating a new connection in both version (14.04 and 16.04) and found in 16.04 a missing chmod!



#STRACE

#16.04 LTS

########## network-manager
network-manager:
  Installed:           1.2.0-0ubuntu0.16.04.3

access("/etc/NetworkManager/system-connections/vpn_test_connection", F_OK) = -1 ENOENT (No such file or directory)
open("/etc/NetworkManager/system-connections/vpn_test_connection.ETK7LY", O_RDWR|O_CREAT|O_EXCL, 0666) = 21
lstat("/etc", {st_mode=S_IFDIR|0755, st_size=12288, ...}) = 0 
lstat("/etc/NetworkManager", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 
lstat("/etc/NetworkManager/system-connections", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 
lstat("/etc/NetworkManager/system-connections/vpn_test_connection.ETK7LY", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0 
lstat("/etc/NetworkManager/system-connections/vpn_test_connection", 0x7ffe030f4470) = -1 ENOENT (No such file or directory)
rename("/etc/NetworkManager/system-connections/vpn_test_connection.ETK7LY", "/etc/NetworkManager/system-connections/vpn_test_connection") = 0 
lstat("/etc", {st_mode=S_IFDIR|0755, st_size=12288, ...}) = 0 
lstat("/etc/NetworkManager", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 
lstat("/etc/NetworkManager/system-connections", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 
lstat("/etc/NetworkManager/system-connections/vpn_test_connection", {st_mode=S_IFREG|0644, st_size=651, ...}) = 0 
chown("/etc/NetworkManager/system-connections/vpn_test_connection", 0, 0) = 0 

# ll /etc/NetworkManager/system-connections
-rw-r--r--  1 root root   651 Aug 15 08:23 vpn_test_connection



#14.04 LTS

########## network-manager
network-manager:
  Installed:           0.9.8.8-0ubuntu7.3

access("/etc/NetworkManager/system-connections/vpn_test_connection",F_OK) = -1 ENOENT (No such file or directory)
open("/etc/NetworkManager/system-connections/vpn_test_connection.QSY9LY", O_RDWR|O_CREAT|O_EXCL, 0666) = 20
lstat("/etc", {st_mode=S_IFDIR|0755, st_size=12288, ...}) = 0
lstat("/etc/NetworkManager", {st_mode=S_IFDIR|0755, st_size=4096, ...})= 0
lstat("/etc/NetworkManager/system-connections", {st_mode=S_IFDIR|0755,st_size=4096, ...}) = 0
lstat("/etc/NetworkManager/system-connections/vpn_test_connection.QSY9LY", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
lstat("/etc/NetworkManager/system-connections/vpn_test_connection",0x7ffd0f0f3860) = -1 ENOENT (No such file or directory)
rename("/etc/NetworkManager/system-connections/vpn_test_connection.QSY9LY", "/etc/NetworkManager/system-connections/vpn_test_connection") = 0
lstat("/etc", {st_mode=S_IFDIR|0755, st_size=12288, ...}) = 0
lstat("/etc/NetworkManager", {st_mode=S_IFDIR|0755, st_size=4096, ...})= 0
lstat("/etc/NetworkManager/system-connections", {st_mode=S_IFDIR|0755,st_size=4096, ...}) = 0
lstat("/etc/NetworkManager/system-connections/vpn_test_connection",{st_mode=S_IFREG|0644, st_size=498, ...}) = 0
chown("/etc/NetworkManager/system-connections/vpn_test_connection",0, 0) = 0
chmod("/etc/NetworkManager/system-connections/vpn_test_connection",0600) = 0

# ll /etc/NetworkManager/system-connections
-rw-------  1 root root   651 Aug 15 08:23 vpn_test_connection

Could be this commit:

# /tmp/NetworkManager$ git checkout 60b7ed3
Note: checking out '60b7ed3'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD ist jetzt bei 60b7ed3... ifcfg,keyfile: fix temporary file races (CVE-2016-0764)

(In reply to s.wend from comment #8)
> Could be this commit:
>
> # /tmp/NetworkManager$ git checkout 60b7ed3
> Note: checking out '60b7ed3'.

> HEAD ist jetzt bei 60b7ed3... ifcfg,keyfile: fix temporary file races
> (CVE-2016-0764)

Yes, that's probably the cause of your issue since now permissions are restricted using umask() upon file creation with g_file_set_contents(), to guarantee atomicity; but the umask is ignored if there is any ACL on the parent directory.

The ACL has:

 default:group::r-x
 default:other::r-x

and files are created with:

 rw-r--r--

which is the intersection between the ACL default and the 666 mode used by g_file_set_contents().

I'm still convinced an ACL on the NM connections directory is a bad
idea; do you have any insight what is its purpose?

But since this technically is a regression, I think we should fix it.

Created attachment 334230 [details] [review]
[PATCH] core: introduce and use nm_utils_file_set_contents()

How about the attached patch?

(In reply to Beniamino Galvani from comment #10)
> Created attachment 334230 [details] [review] [review]
> [PATCH] core: introduce and use nm_utils_file_set_contents()
> 
> How about the attached patch?

Looks good to me.


could we name "errno_sv" instead "errsv"? I don't know what "sv" stands for (saved value?), but that is what is used virtually everywhere else.


systemd's loop_write() does:

                if (_unlikely_(nbytes > 0 && k == 0)) /* Can't really happen */
                        return -EIO;

                assert((size_t) k <= nbytes);

maybe we should do that too?



what is the significance of the lstat() in:
+    if (   lstat (filename, &statbuf) == 0
+        && statbuf.st_size > 0
+        && fsync (fd) != 0) {
looks interesting, if you know it, please add a comment why.




This should also replace ifcfg-rh's write_secret_file().


And be extended to preserve SELinux context https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/settings/nm-settings.c?id=cbbc8620a1442d1801e8ef2d0fd27a38fbadc4f8#n1703

Created attachment 334243 [details] [review]
[PATCH v2] core: introduce and use nm_utils_file_set_contents()

(In reply to Thomas Haller from comment #11)
> could we name "errno_sv" instead "errsv"? I don't know what "sv" stands for
> (saved value?), but that is what is used virtually everywhere else.

Ok.

> systemd's loop_write() does:
>
>                 if (_unlikely_(nbytes > 0 && k == 0)) /* Can't really happen
> */
>                         return -EIO;

GLib doesn't have it and I think it's safe to assume it'll never happen.

> what is the significance of the lstat() in:
> +    if (   lstat (filename, &statbuf) == 0
> +        && statbuf.st_size > 0
> +        && fsync (fd) != 0) {
> looks interesting, if you know it, please add a comment why.

Now I've copied the comment from GLib.

> This should also replace ifcfg-rh's write_secret_file().

Done.

> And be extended to preserve SELinux context
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/settings/
> nm-settings.c?id=cbbc8620a1442d1801e8ef2d0fd27a38fbadc4f8#n1703

As discussed on IRC, g_file_set_contents() creates the file with the
SELinux label set by setfscreatecon(), so no change is necessary.

I have added some assertions in v2 and removed the call to umask().

v2 lgtm

I would prefer if we avoided reimplementing g_file_set_contents() if possible.

At least until we learn what purpose does that ACL serve.

(In reply to Lubomir Rintel from comment #14)
> I would prefer if we avoided reimplementing g_file_set_contents() if
> possible.
> 
> At least until we learn what purpose does that ACL serve.

we already re-implemented it in write_secret_file() and write_cert_key_file().

-- Beniamino, could we also replace write_cert_key_file()?

Also, it's nicer to have a file-set-contents functions that accepts a "mode" argument, instead of doing the saved_umask-and-restore thing repeatedly. I'd be anyway in favor of such a wrapper.

Created attachment 334482 [details] [review]
[PATCH v3] core: introduce and use nm_utils_file_set_contents()

(In reply to Lubomir Rintel from comment #14)
> I would prefer if we avoided reimplementing g_file_set_contents() if
> possible.
> 
> At least until we learn what purpose does that ACL serve.

Ideally we should create the file passing the correct mode; creating
the file with 0666 and using umask to limit the permissions as we
currently do is not completely equivalent as it doesn't work in
presence of ACLs. The use of ACLs on NM configuration directories is
questionable, but people like to do strange things :).

Also, we had already a couple of equivalent functions in the tree, and it
seems useful to consolidate them into a single utility function
(thanks Thomas for spotting the other occurrence).

lgtm

Two comments:

1) let's file a bug upstream in glib saying we'd like g_file_set_contents() but one that allows us to specify permissions, because this race/mask issue is a real problem.  The answer might come back to "Just use GFile" or something, but at least we tried.

2) can we re-implement this with GFile instead, so it's a bit more modern?  I took a quick look and it may actually be more complicated and maybe impossible with GFile, but that also seems like a gap in glib that should be filed in a bug or something.  eg, g_file_new_tmp() doesn't take permissions.

If the GLIb recommendation is to set umask() before calling file creation functions, and this approach is incompatible with ACLs, then clearly there's an issue upstream in GLib that at least needs a bug filed.

That all said, the patch is a nice cleanup; I like explicitly specifying the permissions instead of playing games with umask ourselves.

(In reply to Dan Williams from comment #18)
> Two comments:
> 
> 1) let's file a bug upstream in glib saying we'd like g_file_set_contents()
> but one that allows us to specify permissions, because this race/mask issue
> is a real problem.  The answer might come back to "Just use GFile" or
> something, but at least we tried.

I've filed bug 771476, let's see what comes out.

> 2) can we re-implement this with GFile instead, so it's a bit more modern? 
> I took a quick look and it may actually be more complicated and maybe
> impossible with GFile, but that also seems like a gap in glib that should be
> filed in a bug or something.  eg, g_file_new_tmp() doesn't take permissions.

Ok, I'll look into it.

Merged patch v3; keeping the bz open for the improvements mentioned in comment 18.

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=21358edc549123a754e508f2b317c811516e138a

GFile seems needlessly complicated. Using the filedescriptor directly and Posix/C API (or their thin glib wrappers) is as simple as it gets.

I am doubtful of such "improvements", but I can be proven wrong.

In response to comment 21 and comment 18, if anybody wants to implement that, please go ahead. I am personally not convinced the more object-oriented API is actually nicer, but well.

But we don't need an open BZ for this. Closing this BZ as FIXED.

*** Bug 785769 has been marked as a duplicate of this bug. ***