Before setting up worker sandboxes with Bubblewrap, the paths should be canonicalized by calling realpath(). Bubblewrap does not handle paths with absolute symlinks <https://github.com/containers/bubblewrap/issues/195>, so this avoids that problem. The error message presented by bwrap in such a case is, "bwrap: Can't create file at /path/to/symbolic/link: No such file or directory". This problem was observed on GNU Guix with WebKitGTK 2.28.1 <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=40837>.
Created attachment 452983 [details] Patch
(In reply to apteryx from comment #1) > Created attachment 452983 [details] > Patch The patch looks reasonable to me, but I am a bit shy to approve it because there are other reviewers with more knowledge about of the sandbox code. If Michael or Patrick could rubber-stamp this, that would be perfect.
Comment on attachment 452983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452983&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:334 > +static void bindSymlinksRealPath(Vector<CString>& args, const char* path, const char* bindOption = "--ro-bind") > +{ > + WTF::String realPath = FileSystem::realPath(path); > + if (path == realPath) { > + const char* rpath = realPath.utf8().data(); > + args.appendVector(Vector<CString>({ bindOption, rpath, rpath })); > + } > +} I'm a little confused here. What I expected: if the path is a symlink, bind its target instead so the operation doesn't fail. What you have here: if the path is a symlink (path != realPath), ignore it. Sure, the operation will not fail if you skip it, but won't you still wind up with a broken sandbox? > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:352 > + bindSymlinksRealPath(args, path, bindType); > + // As /etc is exposed wholesale, do not layer extraneous bind Style nit: leave a blank line here.
Comment on attachment 452983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452983&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:330 > + if (path == realPath) { Seems this was supposed to be !=, which indicates that another round of testing is warranted here. ;)
Comment on attachment 452983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452983&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:355 > + if (!WTF::String(path).startsWith("/etc/")) No point in making a new object. `g_str_has_prefix()` takes a `char*`.
Created attachment 453003 [details] Patch
(In reply to Michael Catanzaro from comment #3) > Comment on attachment 452983 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452983&action=review > > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:334 > > +static void bindSymlinksRealPath(Vector<CString>& args, const char* path, const char* bindOption = "--ro-bind") > > +{ > > + WTF::String realPath = FileSystem::realPath(path); > > + if (path == realPath) { > > + const char* rpath = realPath.utf8().data(); > > + args.appendVector(Vector<CString>({ bindOption, rpath, rpath })); > > + } > > +} > > I'm a little confused here. > > What I expected: if the path is a symlink, bind its target instead so the > operation doesn't fail. > > What you have here: if the path is a symlink (path != realPath), ignore it. > Sure, the operation will not fail if you skip it, but won't you still wind > up with a broken sandbox? Hmm, you are right. Fixed. > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:352 > > + bindSymlinksRealPath(args, path, bindType); > > + // As /etc is exposed wholesale, do not layer extraneous bind > > Style nit: leave a blank line here. Fixed. Thank you!
Comment on attachment 453003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453003&action=review OK, almost there... > Source/WebKit/ChangeLog:3 > + Canonicalize source paths passed to bindIfExists in BubbleWrap launcher This line should match the title of the bug: [WPE][GTK] Paths should be canonicalized before calling bwrap > Source/WebKit/ChangeLog:6 > + Reviewed by Michael Catanzaro and Adrian Perez. Please don't fill this out yourself unless you've actually received r+. Just leave it saying NOBODY (OOPS!). Confusing, I know.... > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:172 > + const char* rpath = realPath.utf8().data(); > + args.appendVector(Vector<CString>({ bindOption, rpath, rpath })); This is a use-after-free again. The temporary utf8() is out of scope, so rpath is dangling here. You must own the memory somehow. Probably the easiest way is to change rpath to be a CString: CString rpath = realPath.utf8(); args.appendVector(Vector<CString>({ bindOption, rpath.data(), rpath.data() })); I think that would work.
Created attachment 453026 [details] Patch
> > > Source/WebKit/ChangeLog:3 > > + Canonicalize source paths passed to bindIfExists in BubbleWrap launcher > > This line should match the title of the bug: > > > [WPE][GTK] Paths should be canonicalized before calling bwrap Fixed. > > Source/WebKit/ChangeLog:6 > > + Reviewed by Michael Catanzaro and Adrian Perez. > > Please don't fill this out yourself unless you've actually received r+. Just > leave it saying NOBODY (OOPS!). Confusing, I know.... OK! I wasn't sure. Fixed. > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:172 > > + const char* rpath = realPath.utf8().data(); > > + args.appendVector(Vector<CString>({ bindOption, rpath, rpath })); > > This is a use-after-free again. The temporary utf8() is out of scope, so > rpath is dangling here. You must own the memory somehow. Probably the > easiest way is to change rpath to be a CString: > > CString rpath = realPath.utf8(); > args.appendVector(Vector<CString>({ bindOption, rpath.data(), rpath.data() > })); > > I think that would work. Tricky! I'm still not sure I fully understand; but I've used your suggestion. Thank you!
(In reply to apteryx from comment #10) > Tricky! I'm still not sure I fully understand; but I've used your > suggestion. It's really simple. rpath.utf8() is a CString. rpath.utf8().data() is a pointer to the char* data owned by the CString. It's valid for the lifetime of rpath.utf8(). Once rpath.utf8() is destroyed, it's a dangling pointer to memory that is no longer valid. And in your previous patch, it was destroyed right away, because it was a temporary.
(In reply to Michael Catanzaro from comment #11) > (In reply to apteryx from comment #10) > > Tricky! I'm still not sure I fully understand; but I've used your > > suggestion. > > It's really simple. rpath.utf8() is a CString. rpath.utf8().data() is a > pointer to the char* data owned by the CString. It's valid for the lifetime > of rpath.utf8(). Once rpath.utf8() is destroyed, it's a dangling pointer to > memory that is no longer valid. And in your previous patch, it was destroyed > right away, because it was a temporary. Thank you for having explained in more details... it's clear now!
Committed r290401 (247713@main): <https://commits.webkit.org/247713@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453026 [details].
Comment on attachment 453026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453026&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:173 > + if (path != realPath) { > + CString rpath = realPath.utf8(); > + args.appendVector(Vector<CString>({ bindOption, rpath.data(), rpath.data() })); > + } I don't understand this, path and realPath will be equal if path has no symlink, so path won't be bound at all in the sandbox. Was that really the intent here?
If it's not a symlink then it gets bound at the bottom of bindIfExists, after the call to bindSymlinksRealPath.