[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 --------------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <sys/stat.h>
#include <fcntl.h>

#define MY_TEST_DIR_NAME "my_test_dir"
#define MY_TEST_UID_1 42
#define MY_TEST_UID_2 43
#define MY_TEST_GROUP 2222

static void testDir(int flags)
{
    system("ls -ld " MY_TEST_DIR_NAME);

    /*
     * Open the directory.
     */
    int fd = open(MY_TEST_DIR_NAME, flags | O_NOFOLLOW);
    if (fd < 0) {
        fprintf(stderr, "Couldn't open directory (%s)\n", strerror(errno));
        return;
    }
    system("ls -ld " MY_TEST_DIR_NAME);

    /*
     * Give it away.
     */
    if (fchown(fd, MY_TEST_UID_1, MY_TEST_GROUP) != 0) {
        fprintf(stderr, "Can't do first change of ownership (%s)\n", 
                strerror(errno));
        close(fd);
        return;
    }
    system("ls -ld " MY_TEST_DIR_NAME);

    /*
     * Give it away again.
     */
    if (fchown(fd, MY_TEST_UID_2, MY_TEST_GROUP) != 0) {
        fprintf(stderr, "Can't do second change of ownership (%s)\n", 
                strerror(errno));
        close(fd);
        return;
    }
    system("ls -ld " MY_TEST_DIR_NAME);

    printf("All OK!\n");
    close(fd);
}

int main(int argc, char ** argv)
{
    /*
     * Parse the args.
     */
    if (argc != 3) {
        fprintf(stderr, "Usage: %s [O_RDONLY|O_WRONLY] mode\n", argv[0]);
        exit(1);
    }

    int flags;
    if (strcmp(argv[1], "O_RDONLY") == 0) {
        flags = O_RDONLY;
    } else if (strcmp(argv[1], "O_WRONLY") == 0) {
        flags = O_WRONLY;
    } else {
        fprintf(stderr, "Argument must be O_RDONLY or O_WRONLY\n");
        exit(1);
    }

    int mode = (int)strtol(argv[2], NULL, 0);

    /*
     * Will only work as root.
     */
    if (geteuid() != 0) {
        fprintf(stderr, "Must run test as root.\n");
        exit(1);
    }

    /*
     * Create a directory.
     */
    if (mkdir(MY_TEST_DIR_NAME, mode) < 0) {
        fprintf(stderr, "Can't create directory(%s)\n", strerror(errno));
        exit(1);
    }

    testDir(flags);

    /*
     * Delete the directory.
     */
    if (rmdir(MY_TEST_DIR_NAME) < 0) {
        fprintf(stderr, "Can't delete directory(%s)\n", strerror(errno));
        exit(1);
    }

    exit(0);
}