GNOME.org
 

Re: [BuildStream] Protect against plugin modifications of artifacts

Reply in line.

On 21/06/2020 07:36, Tristan Van Berkom wrote:

Hi all,

TLDR;

    This is a motion to destroy any and all possible public APIs from
    the virtual directory API[0] and any related APIs, which mistakenly
    give plugin authors the ability to mutate the content and attributes
    of files within the sandbox or artifacts, or to import files from
    outside the sandbox which are not otherwise imported by Sources.

    This is not a proposal for anything new, this is a course correction
    to ensure that filesystem permutations can only ever happen within
    the sandbox, as they were only ever allowed to happen in the
    sandbox.

In this email I will try to piece together a story of how things went
awry in this regard.



[...]
I am not familiar with how we got here but I am interested in how to write good plugins so I have replied to that section. 




What to do for manifests and oci plugins
========================================
Go back to the drawing board.

For collect manifest, I strongly suggest revisiting the mailing list
discussions[3] from 2018 and instead of working around the issue with a
quick hack, start getting to work on the root cause of this issue which
is that BuildStream cannot export originating source URLs in a standard
format (in any case, collect_manifest.py has no realistic future and
needs to be rewritten due to the aforementioned issue[6])

For the `oci` plugin, I am not sure what details caused it to become
dependent on this exploitation of the sandbox, but certainly there are
other ways to do this without having plugin python manipulating file
content in the sandbox.

   * The plugin could export public data to the sandbox via environment
     variables, allowing the sandbox to execute scripts which generate
     whatever is needed.

   * BuildStream could be given a feature to *stage* public data in a
     sandbox relative location for elements to process.

     While this would be borderline similar to the existing exploits,
     at least it would be maintained in the core once, where we could
     take special care to ensure that staged public data is always
     sorted stably and content is never changed in the future.
There are already a number of plugins that put files in the sandbox but that do it slightly differently. They run commands that then get executed in the sand box but the effect is the same, files get added that did not come from a source plugin.
It is very common to use cat and some variable to place files in the sandbox that do not come from a "source plugin", see x86image [4] which Tristan often mentions. I appreciate that by creating the file in this way, the action is recorded in the config but the config can be manipulated by the plugin. I have written rather convoluted plugins to try and make the x86image much more generic. This made assembling the cat command very complex and ultimately lead to the plugin not being successful.
To me the issue is, do changes in the artifact have changes in the cache key. It is the authors job to make sure that this is true with tools such as get_unique_key.
A author could cause changes to a artifact by changing the way they use the `blessed` api eg `layout_add`.
Using cat is actually rather tricky and I think we could make much better plugins by just adding files to the sandbox directly. The x86image is very ridged and so i have had mixed success with a plugin wrapped around genimage, freedesktop-sdk use this approach but not the plugin a lot but as there configuration files are added with cat they end up having to have loads of very different elements.
I think a good thing would be for use to have a flexible image creation plugin that has a nice way to create configs and assemble the sandbox. I was planing to use the API that Tristan is planning to outlaw to do this as I think it is a the best way. As noted above plugins anthers who are very familiar with bst2 internals have chose to do this for plugins like OCI.
Looking at the new version of some of the plugins you have talked about [7] [8] I am confused by the problem. The new api has all file manipulation in nice context handlers that control how the new data is added in to the cas. I can see that maybe we could change when you can do this so that the new files can be baked in to the cache key. Maybe at configure time, but I think that would add time to bst show.
As discussed on IRC adding a cleaner API for changing permissions etc would be nice but I don't really see why having to include a build dep to bring chown/chmod in to a sandbox and coupling this with some config is so much better. Or why we can not found a way to make sure that tweaking the sandbox can not be done safely. 



But in short, the authors of these plugins should be doing the work to
get their features supported properly upstream, and BuildStream 2.0
should not be held hostage to provide alternative support for a feature
which was never a feature in the first place.
BST should not be held hostage to any one of course but given that I have found and helped to fix 8 bugs in the last 2 mounts from trying to get bst2 to work with freedesktop-sdk. Several of these bugs have been HUGE eg the handling of variables being broken or bst not being able to pull a project the size of freedesktop-sdk it seems that freedesktop-sdk and buildstream working together is giving both projects a lot of benefit.
It would seem rather prudent to me to check that bst2 can be used with some real projects before the release. Freedesktop-sdk seems like the most obvious candidate as everyone one can see it and work on it. But I don't expect us to need to block on freedesktop-sdk specifically, if we have a alternative approach to validate bst before release? 




So, what are your thoughts ?

Best regards,
     -Tristan


[0]: https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/storage/directory.py
[1]: https://gitlab.com/BuildStream/buildstream/-/issues/235
[2]: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/issues/55
[3]: https://mail.gnome.org/archives/buildstream-list/2018-August/msg00013.html
[4]: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/issues/376
[5]: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/merge_requests/735
[6]: https://gitlab.com/BuildStream/bst-plugins-experimental/-/issues/2
[7]: https://gitlab.com/BuildStream/bst-plugins-experimental/-/blob/master/src/bst_plugins_experimental/elements/x86image.yaml#L48 [8]: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/blob/dwinship/bst2-fv/use-virtual-directories-in-plugins/plugins/elements/url_manifest.py#L174 [9]: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/blob/dwinship/bst2-fv/use-virtual-directories-in-plugins/plugins/elements/collect_initial_scripts.py#L50
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]