Bug 211131 – [WPE][GTK] Paths should be canonicalized before calling bwrap

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jack Hill]

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>.

[apteryx]

Created attachment 452983 [details]
Patch

[Adrian Perez]

(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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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. ;)

[Patrick Griffis]

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*`.

[apteryx]

Created attachment 453003 [details]
Patch

[apteryx]

(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!

[Michael Catanzaro]

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.

[apteryx]

Created attachment 453026 [details]
Patch

[apteryx]

> 
> > 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!

[Michael Catanzaro]

(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.

[apteryx]

(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!

[EWS]

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].

[Philippe Normand]

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?

[Michael Catanzaro]

If it's not a symlink then it gets bound at the bottom of bindIfExists, after the call to bindSymlinksRealPath.