[Toybox] [PATCH] Correct Setting of uid and gid when extracting CPIO archive

[Mike Moreton]

Hi Rob,

> 2) I don't understand this bit:
On the Linux I'm running O_WRONLY failed ("Is a directory") and O_RDONLY worked, and I admit I didn't dig too deeply...

I don't have any problem opening a directory with mode 0000 O_RDONLY as root - I've attached a test program that can be run with:

cc -o testit -Wall testit.c && sudo ./testit O_RDONLY 0000

Could it be this was only a problem before the getpid/geteuid change?

> I try to keep the code within 80 chars. Code style thing. This goes way off the right edge without rewrapping.
Sorry - other stuff I'm working on uses longer lines - I'll try to remember!

> I generally put the local variable tests _before_ the tests that potentially generate system calls, because && preserves side effects so the left side does get evaluated even if the right will always fail.
Sounds eminently sensible!

Mike.


-----Original Message-----
From: Rob Landley [mailto:rob at landley.net] 
Sent: 06 December 2015 22:04
To: Mike Moreton <Mike.Moreton at frontier-silicon.com>; toybox at lists.landley.net
Subject: Re: [Toybox] [PATCH] Correct Setting of uid and gid when extracting CPIO archive

On 12/02/2015 07:39 AM, Mike Moreton wrote:
> Hi, I've fixed a couple of bugs in cpio.

Thanks, I applied both patches. Several good fixes here in the "I don't know how this ever worked before" category, but:

1) git am says it doesn't have a valid email address. (I fixed that up to get it to apply.)

2) I don't understand this bit:

-      if (!S_ISREG(mode) && !S_ISLNK(mode) && !getpid()) {
-        int fd = open(name, O_WRONLY|O_NOFOLLOW);
+      if (!S_ISREG(mode) && !S_ISLNK(mode) && !geteuid()) {
+        int fd = open(name, O_RDONLY|O_NOFOLLOW);

The purpose of the filehandle is to call fchown() on it. Can we change ownership through a read only filehandle? (The kernel not only allows this now but will continue to allow this in future?)

The O_WRONLY was also there because you can create a node or directory without the read bit set, in which case attempting to open it for read access fails.

I'd really like to do this via O_PATH, but trying to gave me errno EBADF. And it turns out that the bionic workaround of doing:

  fd = open("file", O_PATH)
  sprintf(buf, "/proc/self/fd/%d");
  fd2 = open(buf, O_RDONLY);

Does the proper permissions checks on the second open (good!) which means that using O_PATH to try to reliably affect the metadata of chmod
000 files STILL DOESN'T WORK. Grrr. I expect I have to create directories and nodes chmod u+w and then chmod them again to the final version later. I can presumably add some extra tests to make this only happen when necessary...

Minor comments on second patch:

I try to keep the code within 80 chars. Code style thing. This goes way off the right edge without rewrapping.

I generally put the local variable tests _before_ the tests that potentially generate system calls, because && preserves side effects so the left side does get evaluated even if the right will always fail.
(Fairly minor nitpick.)

Thanks,

Rob
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: testit.c
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20151207/580b311f/attachment.txt>