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

[WaLyong Cho]

On 11/08/2014 01:36 AM, Lennart Poettering wrote:
> On Fri, 07.11.14 15:43, WaLyong Cho (walyong.cho at samsung.com) wrote:
> 
>> On 11/07/2014 09:35 AM, Lennart Poettering wrote:
>>> On Fri, 07.11.14 04:17, WaLyong Cho (walyong.cho at 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 process. So
I thought SmackLabelExec more appropriate.

> 
> b) If SmackLabelExec= is appropriate (which it might well be, after
>    all I really don't grok this), and SmackLabel= is a misnomer that
>    would suggest that something different would happen than actually
>    assumed, then what would an option by the name SmackLabel= for a
>    service unit do differntly from one called SmackLabelExec?

I think the option name make very very us confuse. Even if we decide to
any option that will make misunderstanding. Because the given label name
is *NOT* applied to executed process. Just only be applied to child
systemd process.

> 
> For both SELinux and AppArmor we now have simple options:
> SELinuxContext= and AppArmorProfile=. They only come in one flavour,
> and apply a label/profile to the process being executed and that's
> it. Why would SMACK be more complicated there so that SmackLabel= and
> SmackLabelExec= would even be a distinction?
> 
> Lennart
>