Bad coding practices in moonshot-ui likely to mask significant problems

[Sam Hartman]

The moonshot-ui build generates way too many warnings both at the vala and C compilation phases.

I'm concerned that these warnings are likely to mask memory management or other problems that may eventually compromise user security.

In addition, there are several design decisions in the code that significantly reduce the robustness of the identity selector below that which I'd consider viable for production security software.
Here is what I noticed. In general, I think this code needs some attention.

* Operations involving the keyring store are synchronous. So the UI thread and event loop are blocked while the secret service responds. I believe that the secret service can present UI if it needs to unlock a keyring, so this interaction may take a while.

* On every operation, the UI deletes all identity cards from the keystore and then later adds them back rather than updating only the changed ID cards. This is needlessly complex, but worse is not robust in several ways. There is no locking against other users of the keyring. I think that we do a relatively good job of making sure that we only end up with one identity selector even in headless environments, so this is probably only a concern against other keyring users. More seriously, if there is an error or if the code fails to deserialize some identity card, that entire card will be lost.

* There are a number of warnings about vala constructs being used inappropriately

* I'll specifically call out warnings about inappropriate types for properties in the IdCard class; these seem like potential memory management problems

* The code was ported to use gtk+-3.0. However it uses a huge ton of deprecated parts of GTK+-3.0. First, these items may be removed at some point. Also the warnings make it hard to see more serious warnings. But these items were probably deprecated for a reason and it's quite possible the code is masking problems with how it uses GTK.

[Alejandro Perez (alejandro-perez-mendez)]

> The moonshot-ui build generates way too many warnings both at the vala
> and C compilation phases.
>
> I'm concerned that these warnings are likely to mask memory management
> or other problems that may eventually compromise user security.
>
> In addition, there are several design decisions in the code that significantly reduce the robustness of the identity selector below that which I'd consider viable for production security software.
> Here is what I noticed. In general, I think this code needs some attention.

Agreed. We made some changes already, but these were not covered yet.

>
> * Operations involving the keyring store are synchronous. So the UI
> thread and event loop are blocked while the secret service responds. I
> believe that the secret service can present UI if it needs to unlock a
> keyring, so this interaction may take a while.

In this case, I'm not so sure this is actually that important. The UI is
a slim layer over the secret service. Hence, there is little else to do
when waiting for the Keyring to provide an outcome (specially if we do
not cache the entire credential list on memory). I think having
synchronous calls allows for better robustness, as the user knows, for
instance, when an IdentityCard has actually been written to the
SecretService.
I think that this item could be considered optional.

>
> * On every operation, the UI deletes all identity cards from the
> keystore and then later adds them back rather than updating only the
> changed ID cards. This is needlessly complex, but worse is not robust
> in several ways. There is no locking against other users of the
> keyring. I think that we do a relatively good job of making sure that
> we only end up with one identity selector even in headless environments,
> so this is probably only a concern against other keyring users. More
> seriously, if there is an error or if the code fails to deserialize some
> identity card, that entire card will be lost.

That is obviously wrong and must be solved. Does your patch for
libsecret provide a fix for this one?

>
> * There are a number of warnings about vala constructs being used
> inappropriately
>
> * I'll specifically call out warnings about inappropriate types for
> properties in the IdCard class; these seem like potential memory
> management problems
>
> * The code was ported to use gtk+-3.0. However it uses a huge ton of
> deprecated parts of GTK+-3.0. First, these items may be removed at some
> point. Also the warnings make it hard to see more serious warnings.
> But these items were probably deprecated for a reason and it's quite
> possible the code is masking problems with how it uses GTK.

I will definitely take a look to those and try to fix them. I have a
pending item related to check gtk3 support.

>
> ** Affects: moonshot-ui
> Importance: Undecided
> Status: New
>

[Alejandro Perez (alejandro-perez-mendez)]

This commit avoid writing the entire keyring for each action. Instead, every Keyring subclass stores a map between the provided IdCard and the internal reference the Keyring uses. That way, it is possible to directly remove the card, even when display_name or NAI changed.

https://github.com/janetuk/moonshot-ui/commit/58245fbdd04f76717f5b9f6ad0154f40d521bc61