[PATCH] D93881: [llvm-objcopy] preserve file ownership when overwritten

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:140
+    fs::file_status Stat;
+    std::error_code EC = fs::status(Path, Stat);
+    if (!EC)
----------------
jcai19 wrote:
> rupprecht wrote:
> > jcai19 wrote:
> > > rupprecht wrote:
> > > > rupprecht wrote:
> > > > > WDYT about doing a stat on the newly-generated tempfile so we can skip the redundant fchown if possible? (i.e. the common case of running as a non-root user)
> > > > IIUC, this has to be `fstat` on the input file in order to work securely. Otherwise, there is a race between reading the file (for copying the object from), and deciding what to fchown the new file to.
> > > > WDYT about doing a stat on the newly-generated tempfile so we can skip the redundant fchown if possible? (i.e. the common case of running as a non-root user)
> > > 
> > > I'm not sure I followed here.  How can we set the ownership of the tempfile to the ownership of the output file without inquiring it? And how do we skip fchown in that case?
> > > 
> > > > (i.e. the common case of running as a non-root user)
> > > Right, I will limit this to only affect root user, i.e. when Stat.getUser() is 0. This should also avoid some of the issues with "sudo -u" issue mentioned by @MaskRay.
> > > 
> > > > IIUC, this has to be fstat on the input file in order to work securely. Otherwise, there is a race between reading the file (for copying the object from), and deciding what to fchown the new file to.
> > > 
> > > I think fs::status by default calls stat (https://llvm.org/doxygen/Unix_2Path_8inc_source.html#l00739), which should have the same effect with fast according to man page: "fstat() is identical to stat(), except that the file about which information is to be retrieved is specified by the file descriptor fd."
> > The suggestion to use `fstat` is here: https://sourceware.org/bugzilla/show_bug.cgi?id=26945#c2. Although as mentioned, it may not be exploitable.
> Thanks for the link, but if I understand you correctly, it seems to me that we only have path of the output file, so I have to call stat instead, which is the same as fstat except it takes the path instead of the file descriptor. 
I do realize that `fstat` is just `stat` but applied to a file descriptor. But the point is that relying on the stat properties to not change is a race if you use path names instead of file descriptors.

The idea is to do:
1) int input_fd = fopen("input-file.o")  // llvm-objcopy copies sections etc. from this fd
2) stat input_stat = fstat(input_fd)
3) fchown(output_fd, input_stat.uid, input_stat.gid)

As opposed to:
1) int input_fd = fopen("input-file.o")  // llvm-objcopy copies sections etc. from this fd
2) Attacker changes "input-file.o"
3) stat input_stat = stat("input-file.o") // stat coming from the wrong file
4) fchown(output_fd, input_stat.uid, input_stat.gid) // wrong chown applied


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93881/new/

https://reviews.llvm.org/D93881