Re: [systemd-devel] [PATCH] smack: introduce new SmackLabelExec option

2014-11-09 Thread WaLyong Cho
On 11/08/2014 01:36 AM, Lennart Poettering wrote:
 On Fri, 07.11.14 15:43, WaLyong Cho (walyong@samsung.com) wrote:
 
 On 11/07/2014 09:35 AM, Lennart Poettering wrote:
 On Fri, 07.11.14 04:17, WaLyong Cho (walyong@gmail.com) wrote:

 SMACK64
Used to make access control decisions. In almost all cases
the label given to a new filesystem object will be the label
of the process that created it.
 SMACK64EXEC
The Smack label of a process that execs a program file with
this attribute set will run with this attribute's value.

 I am sorry, but I cannot parse this.

 The smack label  will run with this attribute's value? smack
 labels run? That sentence makes no sense to me at all...

 Again, what kind of objects are SMACK64 and SMACK64EXEC applied to?
 files? processes?

 Sorry, I'm also confused.
 OK, I asked some about this to my security friend.
 And I add Casey Schaufler who the Author of smack.
 I hope my below explain it not wrong. But if not, please point out.


 Quoting the Wikipedia:
 In practice, a subject is usually a process or thread; objects are 
 constructs such as files, directories, TCP/UDP ports, shared memory 
 segments, IO devices etc.


 In case of SMACK, subject is SMACK64EXEC and object is SMACK64.
 Assume like this: /usr/bin/dbus-daemon has both label. SMACK64 is foo
 and SMACK64EXEC is bar. And current process (what will execute the
 /usr/bin/dbus-daemon) has foo label. Let's assume the current
 process
 
 So, here you are talking about *files* that have the SMACK64EXEC and
 SMACK64 type labels, while the *process* only as one generic label
 type.
 
 With your patch you want to introduce SmackLabelExec= now. It's a
 label applied to a *process* however, not to a *file*, right?
 
 This appears incompatible to me: I mean, if a process only has one
 single generic label, and doesn't distuingish between SMACK64 and
 SMACK64EXEC type labels, why would you call the option SmackLabelExec=
 and not the more generic SmackLabel=?

Previous mail just a explaination for the difference SMACK64 and
SMACK64EXEC. OK, well I think you also understood well about that. Now
time to explain my patch.
There was execution problems with User= option.

Precondition in systemd:
1. systemd is running as root uid.
2. systemd has _ SMACK attribute.

Precondition in SMACK side:
1. SMACK access permission checking is by-passed for root uid processes.

Problem case:
1. a system session systemd service has User= option.
2. The file on ExecStart= has some special access label and that can NOT
be accessed by _ label.

The problem occurred procedure:
1. systemd(pid 1) is running as root uid.
2. fork()-ed child systemd still has root uid. (be fork()-ed in
exec_spawn())
3. fork()-ed child systemd process do permission appying. (in the
exec_child())
4. If the target service has User= option the child systemd process will
call enforce_user().
5. After exit of enforce_user() the child systemd no more root uid.
6. Finally, the child systemd process try to execve the given process.
7. (At my case:) the execution was failed because of SMACK access error.

Cause:
The fork()-ed child systemd process has _ SMACK label what same with
parent. And after enforce_user() the child has no more root uid. So that
can not be by-passed.
(As I wrote as above problem case:) The given process file has some
special SMACK label and that can NOT be access-ed by _ attribute. So
filnally the child systemd process can NOT access the given ExecStart= file.


To resolve this, I tought two method.

The first is what I sent patch. Introduce new SMACK option and the given
label by new option is applied to child systemd process. Please, do
*NOT* confuse. The given label is not applied to will be executed
process on ExecStart. Just be applied to child systemd process to
successfully access the file on ExecStart=.
(The reason why I named the option name as SmackLabelExec, the given
label by the option, is applied to child systemd. That is not file. That
is currenlty running process. Not other reason.)

The second. This method do *NOT* need any new option. Before systemd is
calling enforce_user(), systemd still has root uid and can read SMACK64
of the file on ExecStart. So the child systemd process can set
/proc/$PID/attr/current of itself to same label what was red. Then
alwasy the child systemd process can access the given file on ExecStart=.

 
 This really doesn't make sense to me.
 
 I have no understanding of SMACK, and I am not sure I want to
 understand it, and I figure I'd merge the patch regardless which name
 you pick for the option, but this is a bit too blatantly contradictory
 for me to completely ignore.
 
 Let my simplify my questions:
 
 a) Why would you call a setting that controls a label is written into
/proc/$PID/attr/current SmackLabelExec= instead of just
SmackLabel=?

I don't care much this. But I menthioned above. The given label is
applied on child systemd. That is not file. That is running 

[systemd-devel] [PATCH] bus: fix null pointer dereference

2014-11-09 Thread Ronny Chevalier
CID#1237620
---
 src/libsystemd/sd-bus/bus-message.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/libsystemd/sd-bus/bus-message.c 
b/src/libsystemd/sd-bus/bus-message.c
index be36d9f..edadacf 100644
--- a/src/libsystemd/sd-bus/bus-message.c
+++ b/src/libsystemd/sd-bus/bus-message.c
@@ -2048,6 +2048,7 @@ static int bus_message_close_variant(sd_bus_message *m, 
struct bus_container *c)
 
 assert(m);
 assert(c);
+assert(c-signature);
 
 if (!BUS_MESSAGE_IS_GVARIANT(m))
 return 0;
@@ -2174,6 +2175,8 @@ _public_ int 
sd_bus_message_close_container(sd_bus_message *m) {
 if (c-enclosing != SD_BUS_TYPE_ARRAY)
 if (c-signature  c-signature[c-index] != 0)
 return -EINVAL;
+if (!c-signature  c-enclosing == SD_BUS_TYPE_VARIANT)
+return -EINVAL;
 
 m-n_containers--;
 
-- 
2.1.3

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


[systemd-devel] [PATCH] udev: fix TOCTOU when creating a directory

2014-11-09 Thread Ronny Chevalier
CID#979416
---
 src/udev/collect/collect.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/udev/collect/collect.c b/src/udev/collect/collect.c
index dc849bd..6cb10fe 100644
--- a/src/udev/collect/collect.c
+++ b/src/udev/collect/collect.c
@@ -86,12 +86,13 @@ static void usage(void)
  */
 static int prepare(char *dir, char *filename)
 {
-struct stat statbuf;
 char buf[512];
 int fd;
+int r;
 
-if (stat(dir, statbuf)  0)
-mkdir(dir, 0700);
+r = mkdir(dir, 0700);
+if (r  0  errno != EEXIST)
+return -errno;
 
 snprintf(buf, sizeof(buf), %s/%s, dir, filename);
 
-- 
2.1.3

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


[systemd-devel] [PATCH] shared: explicitly ignore the return value of wait_for_terminate

2014-11-09 Thread Ronny Chevalier
CID#1237532
CID#1237523
CID#1237522
---
 src/shared/pager.c| 2 +-
 src/shared/spawn-ask-password-agent.c | 2 +-
 src/shared/spawn-polkit-agent.c   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/shared/pager.c b/src/shared/pager.c
index 5479094..001927c 100644
--- a/src/shared/pager.c
+++ b/src/shared/pager.c
@@ -143,7 +143,7 @@ void pager_close(void) {
 /* Inform pager that we are done */
 fclose(stdout);
 kill(pager_pid, SIGCONT);
-wait_for_terminate(pager_pid, NULL);
+(void) wait_for_terminate(pager_pid, NULL);
 pager_pid = 0;
 }
 
diff --git a/src/shared/spawn-ask-password-agent.c 
b/src/shared/spawn-ask-password-agent.c
index c1a9c58..ff121f8 100644
--- a/src/shared/spawn-ask-password-agent.c
+++ b/src/shared/spawn-ask-password-agent.c
@@ -62,6 +62,6 @@ void ask_password_agent_close(void) {
 /* Inform agent that we are done */
 kill(agent_pid, SIGTERM);
 kill(agent_pid, SIGCONT);
-wait_for_terminate(agent_pid, NULL);
+(void) wait_for_terminate(agent_pid, NULL);
 agent_pid = 0;
 }
diff --git a/src/shared/spawn-polkit-agent.c b/src/shared/spawn-polkit-agent.c
index 29b01db..7a90ef8 100644
--- a/src/shared/spawn-polkit-agent.c
+++ b/src/shared/spawn-polkit-agent.c
@@ -82,7 +82,7 @@ void polkit_agent_close(void) {
 /* Inform agent that we are done */
 kill(agent_pid, SIGTERM);
 kill(agent_pid, SIGCONT);
-wait_for_terminate(agent_pid, NULL);
+(void) wait_for_terminate(agent_pid, NULL);
 agent_pid = 0;
 }
 
-- 
2.1.3

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH] smack: introduce new SmackLabelExec option

2014-11-09 Thread Casey Schaufler
On 11/9/2014 5:56 AM, WaLyong Cho wrote:
 On 11/08/2014 01:36 AM, Lennart Poettering wrote:
 On Fri, 07.11.14 15:43, WaLyong Cho (walyong@samsung.com) wrote:

 On 11/07/2014 09:35 AM, Lennart Poettering wrote:
 On Fri, 07.11.14 04:17, WaLyong Cho (walyong@gmail.com) wrote:

 SMACK64
   Used to make access control decisions. In almost all cases
   the label given to a new filesystem object will be the label
   of the process that created it.
 SMACK64EXEC
   The Smack label of a process that execs a program file with
   this attribute set will run with this attribute's value.
 I am sorry, but I cannot parse this.

 The smack label  will run with this attribute's value? smack
 labels run? That sentence makes no sense to me at all...

 Again, what kind of objects are SMACK64 and SMACK64EXEC applied to?
 files? processes?
 Sorry, I'm also confused.
 OK, I asked some about this to my security friend.
 And I add Casey Schaufler who the Author of smack.
 I hope my below explain it not wrong. But if not, please point out.


 Quoting the Wikipedia:
 In practice, a subject is usually a process or thread; objects are 
 constructs such as files, directories, TCP/UDP ports, shared memory 
 segments, IO devices etc.

 In case of SMACK, subject is SMACK64EXEC and object is SMACK64.
 Assume like this: /usr/bin/dbus-daemon has both label. SMACK64 is foo
 and SMACK64EXEC is bar. And current process (what will execute the
 /usr/bin/dbus-daemon) has foo label. Let's assume the current
 process
 So, here you are talking about *files* that have the SMACK64EXEC and
 SMACK64 type labels, while the *process* only as one generic label
 type.

 With your patch you want to introduce SmackLabelExec= now. It's a
 label applied to a *process* however, not to a *file*, right?

 This appears incompatible to me: I mean, if a process only has one
 single generic label, and doesn't distuingish between SMACK64 and
 SMACK64EXEC type labels, why would you call the option SmackLabelExec=
 and not the more generic SmackLabel=?
 Previous mail just a explaination for the difference SMACK64 and
 SMACK64EXEC. OK, well I think you also understood well about that. Now
 time to explain my patch.
 There was execution problems with User= option.

 Precondition in systemd:
 1. systemd is running as root uid.
 2. systemd has _ SMACK attribute.

On Tizen 3 we set the Smack label of systemd to System.

 Precondition in SMACK side:
 1. SMACK access permission checking is by-passed for root uid processes.

Yes. That's the Linux privilege model.

 Problem case:
 1. a system session systemd service has User= option.
 2. The file on ExecStart= has some special access label and that can NOT
 be accessed by _ label.

That's a configuration error. Set the SMACK64 attribute to _ on the
binary. Is there a security reason to hide the binary?

If you set the Smack label for the service before you set the UID
Bob's your uncle. This isn't a problem if you think about the order
you do things.

 The problem occurred procedure:
 1. systemd(pid 1) is running as root uid.
 2. fork()-ed child systemd still has root uid. (be fork()-ed in
 exec_spawn())
 3. fork()-ed child systemd process do permission appying. (in the
 exec_child())
 4. If the target service has User= option the child systemd process will
 call enforce_user().
 5. After exit of enforce_user() the child systemd no more root uid.
 6. Finally, the child systemd process try to execve the given process.
 7. (At my case:) the execution was failed because of SMACK access error.

This is one very good reason to specify the Smack label of the service
in the systemd configuration rather than using the SMACK64EXEC mechanism.
If you let systemd do the work, like it does for UIDs and capabilities,
this works seamlessly.

 Cause:
 The fork()-ed child systemd process has _ SMACK label what same with
 parent. And after enforce_user() the child has no more root uid. So that
 can not be by-passed.
 (As I wrote as above problem case:) The given process file has some
 special SMACK label and that can NOT be access-ed by _ attribute. So
 filnally the child systemd process can NOT access the given ExecStart= file.


 To resolve this, I tought two method.

 The first is what I sent patch. Introduce new SMACK option and the given
 label by new option is applied to child systemd process. Please, do
 *NOT* confuse. The given label is not applied to will be executed
 process on ExecStart. Just be applied to child systemd process to
 successfully access the file on ExecStart=.
 (The reason why I named the option name as SmackLabelExec, the given
 label by the option, is applied to child systemd. That is not file. That
 is currenlty running process. Not other reason.)

This is unnecessarily complicated. Just have systemd set the label
you want the service to run with and be done with it.


 The second. This method do *NOT* need any new option. Before systemd is
 calling enforce_user(), systemd still has root uid and can read 

Re: [systemd-devel] remote-fs ordering, iSCSI and _netdev

2014-11-09 Thread Ivan Shapovalov
On Tuesday 28 October 2014 at 06:41:32, Andrei Borzenkov wrote: 
 В Mon, 27 Oct 2014 14:10:47 -0700
 Chris Leech cle...@redhat.com пишет:
 
  
  At boot fstab-generator is picking up on the _netdev option in fstab,
  and the generated mount units are ordered against remote-fs properly.
  If I leave a filesystem mounted at shutdown, it will be unmounted before
  the iSCSI session is destroyed or the network is shut down and
  everything works as expected.
  
  But there are two cases that are problematic, adding entries to fstab at
  runtime and manually mounting without adding to fstab (while still using
  the _netdev option, some hint is needed).  The first case actually ends
  up being the second, with the possible work-around of always remembering
  to run a daemon-reload after editing fstab to run fstab-generator again.
 
 
 Even known network filesystems still have a problem. If network
 filesystem is mounted on boot, it pulls in network-online.target which
 (hopefully) serves as synchronization point on shutdown. If there is no
 network filesystem to mount at boot, network-online.target is not
 started. If you mount NFS manually later there is nothing to wait for
 on shutdown so network could be teared down before filesystem is
 unmounted.

Isn't this (unmount before teardown) achieved by After=network.target? That
target is passive, so it is pulled in by a provider, and all should work
even in case of manually mounted filesystems.

Am I wrong somewhere?

-- 
Ivan Shapovalov / intelfx /

signature.asc
Description: This is a digitally signed message part.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] remote-fs ordering, iSCSI and _netdev

2014-11-09 Thread Andrei Borzenkov
В Sun, 09 Nov 2014 20:15:56 +0300
Ivan Shapovalov intelfx...@gmail.com пишет:

 On Tuesday 28 October 2014 at 06:41:32, Andrei Borzenkov wrote:   
  В Mon, 27 Oct 2014 14:10:47 -0700
  Chris Leech cle...@redhat.com пишет:
  
   
   At boot fstab-generator is picking up on the _netdev option in fstab,
   and the generated mount units are ordered against remote-fs properly.
   If I leave a filesystem mounted at shutdown, it will be unmounted before
   the iSCSI session is destroyed or the network is shut down and
   everything works as expected.
   
   But there are two cases that are problematic, adding entries to fstab at
   runtime and manually mounting without adding to fstab (while still using
   the _netdev option, some hint is needed).  The first case actually ends
   up being the second, with the possible work-around of always remembering
   to run a daemon-reload after editing fstab to run fstab-generator again.
  
  
  Even known network filesystems still have a problem. If network
  filesystem is mounted on boot, it pulls in network-online.target which
  (hopefully) serves as synchronization point on shutdown. If there is no
  network filesystem to mount at boot, network-online.target is not
  started. If you mount NFS manually later there is nothing to wait for
  on shutdown so network could be teared down before filesystem is
  unmounted.
 
 Isn't this (unmount before teardown) achieved by After=network.target? That
 target is passive, so it is pulled in by a provider, and all should work
 even in case of manually mounted filesystems.
 
 Am I wrong somewhere?
 

Actually no. Now when I revisited the problem, it was misconfiguration
on openSUSE side and misunderstanding on mine :)



signature.asc
Description: PGP signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] Udev rules hardware database

2014-11-09 Thread Patrick Häcker
  IIRC there used to be a kernel bug that caused autosuspend
  to mostly not work on Linux, which they however blamed on crappy
  devices for a long time. After that kernel bug got fixed I think
  autosuspend works on most devices now, hence we only need a blacklist?
  
  I figure Greg has all the details on this, let's ask him (added to CC):
  
  Greg, say something! Is the autosuspend stuff something we can enable
  safely on most devices now? Do we need a blacklist? Or should we even
  go for a whitelist?
 
 I really don't know.  Some other operating system relies on a whitelist
 due to all of the horrible devices out there that can't handle suspend
 (keyboards and mice are notorious for being bad.) 

Thanks for your input. Do you know in which kernel version the above 
mentioned bug got fixed?
I just checked two mice, a keyboard and a bunch of internal devices with a 
3.17 kernel. Only one of the mice works completely reliable with activated 
power saving as input device – no problems with the internal devices. The 
funny thing is: That mouse is built from the same company as some other 
operating system – dog feeding does make sense.

 You might want to ask one of the distro people to see if they have ever
 turned it on by default and what happened if they tried that.
Unfortunately, I do not know anyone from a distribution who is in charge for 
that area. Do you? A short Google search didn't bring up any distribution 
which did that, but my search was probably incomplete.
But if my results mentioned above should be remotely representative, that 
might be disastrous, because for an average user it might be close to 
impossible to deactivate power savings without working input devices.

Kind regards
Patrick

signature.asc
Description: This is a digitally signed message part.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] Udev rules hardware database

2014-11-09 Thread Oliver Neukum
On Sun, 2014-11-09 at 22:39 +0100, Patrick Häcker wrote:
  
  I really don't know.  Some other operating system relies on a whitelist
  due to all of the horrible devices out there that can't handle suspend
  (keyboards and mice are notorious for being bad.) 
 
 Thanks for your input. Do you know in which kernel version the above 
 mentioned bug got fixed?
 I just checked two mice, a keyboard and a bunch of internal devices with a 
 3.17 kernel. Only one of the mice works completely reliable with activated 
 power saving as input device – no problems with the internal devices. The 
 funny thing is: That mouse is built from the same company as some other 
 operating system – dog feeding does make sense.

The problem with mice is the usual inability to trigger a remote wakeup
when the mouse is moved. They only do a remote wakeup when a button
is pressed. (The behavior is within spec. The USB HID spec is
deficient.)
I took two lessons from that

1. It makes no sense to use the same approach for all USB devices.
Some classes need a blacklist others do need a whitelist.

2. The kernel is not really the ideal place to decide when to activate
power management. Most mice are built for an operating model which
has more grades then Linux usually uses. We see our system either as
fully operational or suspended. Those mice are built for reducing power
consumption when a system becomes unused and the screen is blanked
or the screensaver comes up.

  You might want to ask one of the distro people to see if they have ever
  turned it on by default and what happened if they tried that.
 Unfortunately, I do not know anyone from a distribution who is in charge for 
 that area. Do you? A short Google search didn't bring up any distribution 
 which did that, but my search was probably incomplete.

Unlikely. You end up with systems that are practically unusable in the
worst and commonest case.

 But if my results mentioned above should be remotely representative, that 
 might be disastrous, because for an average user it might be close to 
 impossible to deactivate power savings without working input devices.

Exactly.

Regards
Oliver


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel