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

[Manoj Gupta via Phabricator via llvm-commits]

manojgupta 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)
----------------
rupprecht wrote:
> 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
yes, I think this should be doable to avoid the call to stat completely here since objcopy already has the Stat information for the input file.
Jian, please see my comment for llvm-objcopy.cpp file.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:275
 static Error executeObjcopy(CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.InputFilename != "-") {
----------------
Since original file Stat is available, please re-use this instead of calling stat again on original file later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881