<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jan 12, 2020 at 3:47 PM Simon Baatz <<a href="mailto:gmbnomis@gmail.com">gmbnomis@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Jan 07, 2020 at 04:45:54PM -0500, Brian Bouterse wrote:<br>
>    We had two bugs filed recently [0][1] which suggest that when using the<br>
>    default backend for Pulp, i.e. pulpcore.app.models.storage.FileSystem<br>
>    Pulp should not be "moving" files. This is the default behavior Django<br>
>    gives us, and it destroys data when sync'ed from file:/// for example<br>
>    [1].<br>
>    I propose that with 3.1 we fix this bug by switching<br>
>    pulpcore.app.models.storage.FileSystem to leave files in place and<br>
>    either hard-link (same filesystem) or copy (different filesystem).<br>
>    This will require files to be cleaned up where before the were "moved"<br>
>    so they didn't need cleanup. This will include worker temp directories,<br>
>    uploaded files, uploaded files w/ the chunked API, and downloaded files<br>
>    during sync. I believe this is all straightforward, but an important<br>
>    side-effect of this change to identify. Plugin writers that manage<br>
>    files would also need to handle this, but can mostly rely on pulpcore<br>
>    cleaning up the worker temp directories and user-uploaded files.<br>
>    What do you think about this? Do you have concerns? Is there something<br>
>    better we can do?<br>
<br>
Yes, I have concerns:<br>
<br>
- Aren't there files that are readable for the worker ("pulp" user), but<br>
  may be not for the user doing the import?  The user could try to<br>
  import them.  "file:///etc/pulp/settings.py" comes to mind as a<br>
  juicy target (Pulp pun intended).<br></blockquote><div>Thank you for bringing this up, and even with a pun!<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
  Can we ensure that this can't happen? And which part of Pulp must<br>
  ensure this?<br></blockquote><div>I am also concerned about this. I filed it as a new story here: <a href="https://pulp.plan.io/issues/5974">https://pulp.plan.io/issues/5974</a> 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?<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- In general, we must not trust paths supplied by the user; things<br>
  like missing path normalization and path comparisons using<br>
  startswith() make me nervous (<a href="https://access.redhat.com/security/cve/cve-2018-10917" rel="noreferrer" target="_blank">https://access.redhat.com/security/cve/cve-2018-10917</a><br>
  comes to mind).<br>
<br></blockquote><div>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.</div><div><br></div><div>The paths checked in the fixing of <a href="https://pulp.plan.io/issues/5870">https://pulp.plan.io/issues/5870</a> 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.<br></div><div><br></div></div></div>