[Pulp-dev] Proposal: Changing in 3.1 that Artifact.save() will hard-link/copy, not move

[Brian Bouterse]

On Sun, Jan 12, 2020 at 3:47 PM Simon Baatz <gmbnomis at gmail.com> wrote:

> On Tue, Jan 07, 2020 at 04:45:54PM -0500, Brian Bouterse wrote:
> >    We had two bugs filed recently [0][1] which suggest that when using
> the
> >    default backend for Pulp, i.e. pulpcore.app.models.storage.FileSystem
> >    Pulp should not be "moving" files. This is the default behavior Django
> >    gives us, and it destroys data when sync'ed from file:/// for example
> >    [1].
> >    I propose that with 3.1 we fix this bug by switching
> >    pulpcore.app.models.storage.FileSystem to leave files in place and
> >    either hard-link (same filesystem) or copy (different filesystem).
> >    This will require files to be cleaned up where before the were "moved"
> >    so they didn't need cleanup. This will include worker temp
> directories,
> >    uploaded files, uploaded files w/ the chunked API, and downloaded
> files
> >    during sync. I believe this is all straightforward, but an important
> >    side-effect of this change to identify. Plugin writers that manage
> >    files would also need to handle this, but can mostly rely on pulpcore
> >    cleaning up the worker temp directories and user-uploaded files.
> >    What do you think about this? Do you have concerns? Is there something
> >    better we can do?
>
> Yes, I have concerns:
>
> - Aren't there files that are readable for the worker ("pulp" user), but
>   may be not for the user doing the import?  The user could try to
>   import them.  "file:///etc/pulp/settings.py" comes to mind as a
>   juicy target (Pulp pun intended).
>
Thank you for bringing this up, and even with a pun!

>
>   Can we ensure that this can't happen? And which part of Pulp must
>   ensure this?
>
I am also concerned about this. I filed it as a new story here:
https://pulp.plan.io/issues/5974 I put a design on there that defaults to
no access for file:/// urls and then allows settings to declare approved
paths. I also put some questions in the comments. What do you think?


> - In general, we must not trust paths supplied by the user; things
>   like missing path normalization and path comparisons using
>   startswith() make me nervous (
> https://access.redhat.com/security/cve/cve-2018-10917
>   comes to mind).
>
> I agree, my hope is that 5947 should address the security concerns, but
let me know. If you and @daviddavis can ack it I'd like to add it to the
sprint for 3.1.

The paths checked in the fixing of https://pulp.plan.io/issues/5870 are
administrator provided settings and affect if the file is moved or copied
(not its access). So in addition, we need the safety 5947 provides. Thank
you again for bringing this up.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20200113/797fc612/attachment.htm>