Bug 191360

Created attachment 354090 [details] Patch

Comment on attachment 354090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354090&action=review > Source/WebCore/ChangeLog:3 > + [GTK][WPE] Bubblewrap launcher should not depend on memfd It's a little unclear to me why this is desirable given that kernel 3.17 is positively ancient now, but OK. > Source/WebCore/platform/glib/FileSystemGlib.cpp:42 > +#if HAVE(LINUX_MEMFD_H) Here's what really confuses me. A grep of the entire codebase shows no other uses of LINUX_MEMFD_H. And your patch doesn't appear to add it. So how does this check work? Is it always false? It's not the ideal check, either. The original code is designed to use memfd even if memfd.h doesn't exist, as long as the kernel is new enough. I guess it's perfectly fine to change this, since we have fallback in place, but it's not clear to me that the change here was intentional. > Source/WebCore/platform/glib/FileSystemGlib.cpp:43 > +#include <linux/memfd.h> Looks like this is unused? You only use sycall() directly and don't seem to use anything declared in this header. > Source/WebCore/platform/glib/FileSystemGlib.cpp:47 > + > + Too much space here > Source/WebCore/platform/glib/FileSystemGlib.cpp:477 > + fileDescriptor = syscall(__NR_memfd_create, name, 0); Since you're inside an #if HAVE(LINUX_MEMFD_H) guard, you know linux/memfd.h is available, and can use it instead of syscall(). We were using syscall() to ensure this works when memfd.h is not available. > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:-94 > - if (fcntl(fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL) == -1) { > - g_warning("Failed to seal memfd: %s", g_strerror(errno)); > - close(fd); > - return -1; > - } Well we lost the fd sealing code. I'm not sure how important this is, or why it's there in the first place. Are other processes somehow able to modify the fd and mess with the args passed to bwap without the sealing? Patrick, can you comment on this? > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:47 > -static int > -argsToFd(const Vector<CString>& args, const char *name) > +static RefPtr<SharedMemory> argsToFd(const Vector<CString>& args, const char *name) Oops. Also fix: const char* name > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:708 > + auto flatpakInfoMemory = createFlatpakInfo(); > + std::optional<int> flatpakInfoFd; > + if (flatpakInfoMemory) { How about: if (auto flatpakInfoMemory = createFlatpakInfo()) {

Comment on attachment 354090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354090&action=review >> Source/WebCore/ChangeLog:3 >> + [GTK][WPE] Bubblewrap launcher should not depend on memfd > > It's a little unclear to me why this is desirable given that kernel 3.17 is positively ancient now, but OK. The code si simpler using SharedMemory, IMO and SharedMemory now uses memfd when available. >> Source/WebCore/platform/glib/FileSystemGlib.cpp:42 >> +#if HAVE(LINUX_MEMFD_H) > > Here's what really confuses me. A grep of the entire codebase shows no other uses of LINUX_MEMFD_H. And your patch doesn't appear to add it. So how does this check work? Is it always false? > > It's not the ideal check, either. The original code is designed to use memfd even if memfd.h doesn't exist, as long as the kernel is new enough. I guess it's perfectly fine to change this, since we have fallback in place, but it's not clear to me that the change here was intentional. LINUX_MEMFD_H was added in r237922. Since we have a fallback now, I think it's simpler to only use it if the header is present. >> Source/WebCore/platform/glib/FileSystemGlib.cpp:43 >> +#include <linux/memfd.h> > > Looks like this is unused? You only use sycall() directly and don't seem to use anything declared in this header. Right >> Source/WebCore/platform/glib/FileSystemGlib.cpp:477 >> + fileDescriptor = syscall(__NR_memfd_create, name, 0); > > Since you're inside an #if HAVE(LINUX_MEMFD_H) guard, you know linux/memfd.h is available, and can use it instead of syscall(). We were using syscall() to ensure this works when memfd.h is not available. Can't we have newer glibc with old kernel? that's what ENOSYS is detecting, isn't it? >> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:-94 >> - } > > Well we lost the fd sealing code. I'm not sure how important this is, or why it's there in the first place. Are other processes somehow able to modify the fd and mess with the args passed to bwap without the sealing? Patrick, can you comment on this? I don't think we need it at all, seals only work with memfd and other processes don't assume the fd was created with memfd AFAIK. Ideally we could fix SharedMemoty::createHandle to use the protection parameter, but I don't know how to do it.

Comment on attachment 354090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354090&action=review OK, I was confused because I had forgotten about r237922 and was looking at the old version of SharedMemoryUnix.cpp! >>> Source/WebCore/platform/glib/FileSystemGlib.cpp:477 >>> + fileDescriptor = syscall(__NR_memfd_create, name, 0); >> >> Since you're inside an #if HAVE(LINUX_MEMFD_H) guard, you know linux/memfd.h is available, and can use it instead of syscall(). We were using syscall() to ensure this works when memfd.h is not available. > > Can't we have newer glibc with old kernel? that's what ENOSYS is detecting, isn't it? If you're checking for linux/memfd.h, you should use it. Right now you check to make sure the header exists, and include the header, but don't actually use anything from the header. So that doesn't make sense. r237922 has the same problem, which I missed when I reviewed it. So I would replace this syscall() and the syscall() in SharedMemoryUnix.cpp with calls to memfd_create(). Then you can get rid of the #include <sys/syscall.h>. Alternatively, you could take the opposite route, get rid of the checks for linux/memfd.h, and just rely on syscall(), like Patrick's original code was doing. Either way is fine IMO. But checking for the header and including it and not using it isn't fine!

(In reply to Carlos Garcia Campos from comment #3) > Can't we have newer glibc with old kernel? that's what ENOSYS is detecting, > isn't it? I guess, but that's pedantic, and if anyone ever runs such a configuration, then I think failure is reasonable.

Losing sealing shouldn't be a concern for our usage.

Outside of existing comments looks fine and works here.

Actually regarding F_SEAL_WRITE, I guess that is the definition of what `SharedMemory::Protection::ReadOnly` should do right?

(In reply to Patrick Griffis from comment #8) > Actually regarding F_SEAL_WRITE, I guess that is the definition of what > `SharedMemory::Protection::ReadOnly` should do right? It could be, but I don't think we can do that anyway. It's true that we don't normally create more than one handle for the same shared memory, but the API allows that. We can only seal an fd created by memfd, but not the duplicated one used by the handler (AFAIK). So we would need to seal before dup, making the shared memory read only even if a following createHandle uses a readwrite protection, because seals can't be removed. We could add a seal method to shared memory, to be called after data have been written, but I don't think it's worth it.

(In reply to Carlos Garcia Campos from comment #9) > (In reply to Patrick Griffis from comment #8) > > Actually regarding F_SEAL_WRITE, I guess that is the definition of what > > `SharedMemory::Protection::ReadOnly` should do right? > > It could be, but I don't think we can do that anyway. It's true that we > don't normally create more than one handle for the same shared memory, but > the API allows that. We can only seal an fd created by memfd, but not the > duplicated one used by the handler (AFAIK). So we would need to seal before > dup, making the shared memory read only even if a following createHandle > uses a readwrite protection, because seals can't be removed. We could add a > seal method to shared memory, to be called after data have been written, but > I don't think it's worth it. Well the resulting fd *must* be read-only otherwise its a sandbox escape.

(In reply to Patrick Griffis from comment #10) > Well the resulting fd *must* be read-only otherwise its a sandbox escape. So then the sealing is important! But I don't understand. How can you escape the sandbox? The trusted UI process creates the fd, stuffs arguments into it, launches the bwrap process, and then bwrap reads them from the fd before launching the untrusted process. Right? I don't see why it has to be read-only.

(In reply to Michael Catanzaro from comment #11) > But I don't understand. How can you escape the sandbox? The trusted UI > process creates the fd, stuffs arguments into it, launches the bwrap > process, and then bwrap reads them from the fd before launching the > untrusted process. Right? I don't see why it has to be read-only. Well I've not tested it yet, but `/.flatpak-info` is read at various points during runtime and what it contains determines what `xdg-desktop-portal` exposes. So it does need to be read-only.

Ok so `bubblewrap` itself does make it read-only so its actually totally fine.

It does sound like a bug that the ReadOnly permission is ignored though.

I didn't realize this was used for /.flatpak-info. (We should really probably fix any portals that are depending on that, since it creates a very risky assumption that WebKit is Flatpak... I can imagine portals breaking us in the future with that assumption.)

(In reply to Michael Catanzaro from comment #15) > (We should really probably fix any portals that are depending on that, since > it creates a very risky assumption that WebKit is Flatpak... I can imagine > portals breaking us in the future with that assumption.) Re can reinvent a new format that is effectively identical to flatpak-info but I'm not sure its really needed. I verified that only a single field is required by the portals `Name`, mclasen knows we use it for this, and I see every commit that lands upstream. I think it will be alright.

(In reply to Patrick Griffis from comment #14) > It does sound like a bug that the ReadOnly permission is ignored though. https://bugs.webkit.org/show_bug.cgi?id=131542

Comment on attachment 354090 [details] Patch It seems we agree on the modifications that are needed here. r- for now.