[llvm-objcopy] Drop S_ISUID and S_ISGID bits

[MaskRay]

Summary

This addresses a vulnerability introduced in D62718.

chmod u+s,g+s,o+x a
sudo llvm-strip a
// a should not have set-user-ID or set-group-ID bits

No test because it is not testable on all file systems.

[manojgupta]

This change make further deviation from GNU tools. Lets wait for resolution of D93881. This will not be needed if D93881 is merged.

[MaskRay]

This address llvm-strip exe and llvm-strip exe -o out.

https://sourceware.org/pipermail/binutils/2021-February/115282.html binutils is move to conditional in-place overwrite. I can sign up the work for llvm-objcopy.

[MaskRay]

In D96292#2549683, @manojgupta wrote:

This change make further deviation from GNU tools. Lets wait for resolution of D93881. This will not be needed if D93881 is merged.

This change actually makes our behavior consistent with current GNU strip/objcopy. GNU strip/objcopy does not copy S_ISUID and S_ISGID bits.

chmod u+s,g+s,o+x a
sudo llvm-strip a -o b

Currently b has S_ISUID and S_ISGID bits.

[manojgupta]

In D96292#2563966, @MaskRay wrote:

In D96292#2549683, @manojgupta wrote:

This change make further deviation from GNU tools. Lets wait for resolution of D93881. This will not be needed if D93881 is merged.

This change actually makes our behavior consistent with current GNU strip/objcopy. GNU strip/objcopy does not copy S_ISUID and S_ISGID bits.

chmod u+s,g+s,o+x a
sudo llvm-strip a -o b

Currently b has S_ISUID and S_ISGID bits.

Thanks @MaskRay for the example.
I agree that for the case of Output != Input, S_ISUID and S_ISGID bits should not be copied. The bits should be kept only when Output == Input .

[MaskRay]

I investigated the old and new binutils behaviors a bit. I think we need to do D97253: If input=output, preserve umask bits, otherwise drop S_ISUID/S_ISGID bits

chmod u+s,g+s,o+x a
sudo llvm-strip a -o b

is still a case that we want to change to match cp

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

static Error restoreStatOnFile(StringRef Filename,
   if (std::error_code EC = sys::fs::status(FD, OStat))
     return createFileError(Filename, EC);
   if (OStat.type() == sys::fs::file_type::regular_file)
 #ifdef _WIN32
     if (auto EC = sys::fs::setPermissions(
             Filename, static_cast<sys::fs::perms>(Stat.permissions() &
                                                   ~sys::fs::getUmask())))
 #else
+    // Drop umask, S_ISUID and S_ISGID bits.
     if (auto EC = sys::fs::setPermissions(
             FD, static_cast<sys::fs::perms>(Stat.permissions() &
-                                            ~sys::fs::getUmask())))
+                                            ~(sys::fs::getUmask() | 06000))))
 #endif
       return createFileError(Filename, EC);
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(Filename, EC);
 
   return Error::success();
 }