[sugar] Initial Security Patches

[Michael Stone]

Marco, Simon, 

First, thanks very much for the detailed and helpful reviews. Your
insight and experience are greatly appreciated.

> thanks for the patches, some comments follow...
> 
> +        if os.path.exists('/etc/olpc-security'):
> 
> The way we are usually doing this is to initialize the proxy in a
> try/except block and fallback if the service is not available. Are we
> using /etc/olpc-security here because we want to ensure we never run
> without a Rainbow service on the XO?

We proposed using olpc-security in general because it's an easy test
that required minimal restructuring of the existing code. However, I
think I'm fine with unconditionally searching for a Rainbow service and
using it if found. (I'll discuss it further with Noah, who is the other
main contributor to this patch.)

> +            system_bus = dbus.SystemBus()
> +            rainbow = system_bus.get_object('org.laptop.security.Rainbow', '/')
> +            self.rainbow = dbus.Interface(rainbow,
> +                               dbus_interface='org.laptop.security.Rainbow')
> 
> Please use self._rainbow here, I don't see a reason the proxy should be public.

Agreed.

>      @dbus.service.signal(_DBUS_OWNER_IFACE, signature="s")
>      def CurrentActivityChanged(self, activity_id):
> -        pass
> +        if os.path.exists('/etc/olpc-security'):
> +            self.rainbow.activity_changed(activity_id)
> 
> The os.path.exists('/etc/olpc-security') is a little ugly, let's at
> least avoid doing it when it's not necessary. I think we can just make
> self._rainbow None when '/etc/olpc-security' doesn't exist above and
> just "if self._rainbow" here.

This would be a fine improvement.

> Is there any reason the dbus call is not async here?

I'll assume this question refers to the snippet above where a
synchronous call is made to Rainbow. 

I have mixed feelings about the overall design here. This was the
smallest change we could make in order to extract the requisite
information and, at the moment, the container rate-limits that we are
tickling in response to this hook firing are not security-critical.

However, regardless of whether this call is synchronous or asynchronous,
I imagine on casual contemplation that that there's at least one race
between Sugar and Rainbow in this extended interaction.

Thus, so long as we make no security-critical manipulations based on the
timing of this event with respect to the attempt to foreground an
activity, I don't really care if the method call is synchronous or
asynchronous and I'm not too concerned about the races.

My conclusion is that we should probably paste a fat #RACE: warning in
the appropriate Rainbow code so that if we eventually revise the
security semantics of the operation of foregrounding an activity, we
know where to revisit the Sugar code and that we should perhaps indicate
that the semantics of this interaction may be of long-term security
relevance in the relevant Sugar code.


> +        if not os.path.exists('/etc/olpc-security'):
> +            self._factory.create(self._activity_handle.get_dict(),
> +                                 timeout=120 * 1000,
> +                                 reply_handler=self._no_reply_handler,
> +                                 error_handler=self._create_error_handler)

> I don't want the behavior with and without bitfrost to be so
> different. If we are going for separate processes we should get rid of
> the sugar factory completely and just run separate processes also in
> the non-bitfrost case. So we either just launch the activity
> executable from Sugar (which is what I assume bitfrost is doing) or we
> make bitfrost run even without vserver.

The capsule description of the current rainbow launching process is:

1) Rainbow receives a message from Sugar indicating that a new activity
instance is desired.

2) Rainbow forks, builds an environment including a filesystem (which we
call an "island"), a VServer security context, (and eventually some
firewall rules and rate limits) in which to run the new instance.

3) Rainbow runs the exec-string specified in the activity's bundle metadata.

3b) For the purposes of having testable software today, Rainbow sends
the create message to the resulting factory process.


> Also before landing this we need to fix activities which must run with
> a single process (web-activity for example) to implement their own
> single process mechanism.

I'm not sure I understand how you want to synchronize this. The purpose
of the 'olpc-security' flag file (or of your proposal to try to find the
Rainbow service and to revert to normal sugar launching if that fails)
is to allow us to safely merge the final patches that we produce from this
review process while retaining backward compatibility with existing
activities.

If you're saying that you want to make one large hard break for Trial-3,
then we can do that, but you should know that it's not what we were
intending and that it will require more synchronization between everyone
involved to achieve. (Note: I'm not saying it's a bad idea, just trying
to communicate what we had intended, which was to give activity authors
a more gentle, discretionary introduction to the changes introduced by
containerization.)

Thanks again,

Michael