Stephen Browne wrote:
> Niall,
>
> thanks for the comments. That'll teach me to blindly port JohnF's code.
>
> I will change this to the piece of code I wrote for gnome-desktop.
>
> You are right for the case where the env var TRUSTED_SESSION is not
> defined trusted_session will remain null and the getenv call will happen
> multiple times. I have changed this to the following.
>
> static gboolean
> g_tsol_is_multi_label_session (void)
> {
> static char *trusted = -1;
>
> if (trusted < 0) {
> if (getenv ("TRUSTED_SESSION")) {
> trusted = 1;
> } else {
> trusted = 0;
> }
> }
>
> return trusted ? TRUE : FALSE;
> }
I would rather use something like this. IMO, it doesn't look sane to
initialize a pointer to -1.
static gboolean
g_tsol_is_multi_label_session (void)
{
static gboolean checked = FALSE;
static gboolean trusted = FALSE;
if (! checked) {
trusted = (getenv("TRUSTED_SESSION") != NULL);
checked = TRUE;
}
return trusted;
}
> This way the getenv is only ever called once. After that it has been
> initialised and the function will return the appropriate value.
>
> I don't see the need to conditionally compile this code, as you say
> there are no additional deps introduced.
>
> Since you mention pushing upstream I'd like to remind people that TJDS
> is very much Solaris specific right now. Once we have everything out on
> opensolaris.org I will be starting a discussion about how to push these
> changes back upstream. This may involve a partial redesign to
> accommodate SElinux and/or other security architectures that the GNOME
> project may be interested in.
>
> Thanks again,
>
> Stephen.
>
> On Wed, 2006-11-01 at 13:30, Niall Power wrote:
>> Hi Stephen,
>>
>> I have a couple of questions about this patch.
>>
>> The first is a minor nit:
>>
>> ++static gboolean
>> ++g_tsol_is_multi_label_session (void)
>> ++{
>> ++ static char *trusted_session = NULL;
>> ++
>> ++ if (!trusted_session)
>> ++ trusted_session = getenv("TRUSTED_SESSION");
>> ++
>> ++ if (!trusted_session)
>> ++ return FALSE;
>> ++
>> ++ return TRUE;
>> ++}
>> ++
>>
>> It's hard to figure out if this function is called multiple times
>> or just once, from looking at the diff. I see it called in
>> _g_module_open(). Is it called once or potentially many times?
>> If multiple times, then it costs a getenv() system call each time
>> _g_module_open() is invoked on a non trusted system. It seems like
>> a performance hit for the most common case.
>>
>> The second nit is that I don't see any conditional pre-processor
>> macros around this code or configure.in environment checks.
>> If we want to get this patch back upstream then we might want to
>> consider this. But since there are no trusted extensions specific
>> library dependencies introduced, maybe it's not such a big deal.
>>
>> Otherwise, looks good.
>>
>> Thanks,
>> Niall.
>>
>> On Wed, 2006-11-01 at 13:28 +0000, Stephen Browne wrote:
>>> Hi,
>>>
>>> I thought I'd start you off with one of our more simple Trusted
>>> Extensions patches. :)
>>>
>>> Please take a look at the following NEW patch for glib.
>>>
>>> This patch is to improve the security of the module loading aspect of
>>> glib so that trusted code can not be extended by arbitrary modules and
>>> left open to the case where rogue code can be inserted into a trusted
>>> component.
>>>
>>> In a trusted multi-label session, indicated by the env var
>>> TRUSTED_SESSION, glib will restrict the locations that it will accept
>>> modules from by first asking the runtime linker for the information it
>>> has been configured with and falling back to some default file system
>>> locations.
>>>
>>> Thanks,
>>>
>>> Stephen.
>
--
Greetings, alo.
