Bug 167093 – [URL Parser] ASSERTION FAILED: url == m_string in WebCore::URL::URL

• Status: NEW
• Component: WebCore Misc.

[Milan Crha]

I'm running git master of the WebKitGTK+ at commit 7f939a1123616ab4c72e467a48476db943669343 and when I change to a certain message in the evolution, then the UI process crashes on this assertion:

ASSERTION FAILED: url == m_string
....webkit.master/Source/WebCore/platform/URL.cpp(431) : WebCore::URL::URL(WebCore::ParsedURLStringTag, const WTF::String&)

(gdb) bt
#0  0x00007fc58a702fdb in waitpid () at /lib64/libpthread.so.0
#1  0x00007fc589060a32 in g_spawn_sync (working_directory=working_directory@entry=0x0, argv=<optimized out>, envp=envp@entry=0x0, flags=flags@entry=G_SPAWN_SEARCH_PATH, child_setup=child_setup@entry=0x0, user_data=user_data@entry=0x0, standard_output=0x0, standard_error=0x0, exit_status=0x0, error=0x7fff107869b8) at gspawn.c:410
#2  0x00007fc589060e56 in g_spawn_command_line_sync (command_line=<optimized out>, standard_output=0x0, standard_error=0x0, exit_status=0x0, error=0x7fff107869b8) at gspawn.c:727
#3  0x00007fc5621e4703 in run_bug_buddy (appname=0xe5dd70 "evolution", pid=9925) at gnome-segvhanlder.c:245
#4  0x00007fc5621e45bf in bugbuddy_segv_handle (signum=11) at gnome-segvhanlder.c:196
#5  0x00007fc58a7035c0 in <signal handler called> () at /lib64/libpthread.so.0
#6  0x00007fc579d96f8b in WTFCrash () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
#7  0x00007fc583b79df0 in WebCore::URL::URL(WebCore::ParsedURLStringTag, WTF::String const&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fc5825b7287 in IPC::ArgumentCoder<WebCore::URL>::decode(IPC::Decoder&, WebCore::URL&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#9  0x00007fc5825be987 in std::enable_if<!std::is_enum<WebCore::URL>::value, bool>::type IPC::Decoder::decode<WebCore::URL>(WebCore::URL&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#10 0x00007fc5825c1412 in bool WebCore::ResourceRequestBase::decodeBase<IPC::Decoder>(IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#11 0x00007fc582a70d16 in bool WebCore::ResourceRequest::decodeWithPlatformData<IPC::Decoder>(IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#12 0x00007fc582a70279 in IPC::ArgumentCoder<WebCore::ResourceRequest>::decodePlatformData(IPC::Decoder&, WebCore::ResourceRequest&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
---Type <return> to continue, or q <return> to quit---
#13 0x00007fc5825b5c4e in IPC::ArgumentCoder<WebCore::ResourceRequest>::decode(IPC::Decoder&, WebCore::ResourceRequest&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#14 0x00007fc582540259 in std::enable_if<!std::is_enum<WebCore::ResourceRequest>::value, bool>::type IPC::Decoder::decode<WebCore::ResourceRequest>(WebCore::ResourceRequest&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#15 0x00007fc582604216 in API::URLRequest::decode(IPC::Decoder&, WTF::RefPtr<API::Object>&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#16 0x00007fc5825a78d3 in WebKit::UserData::decode(IPC::Decoder&, WTF::RefPtr<API::Object>&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#17 0x00007fc5825a68c8 in WebKit::UserData::decode(IPC::Decoder&, WTF::RefPtr<API::Object>&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#18 0x00007fc5825a5b2b in WebKit::UserData::decode(IPC::Decoder&, WebKit::UserData&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#19 0x00007fc5825956f6 in IPC::ArgumentCoder<WebKit::UserData>::decode(IPC::Decoder&, WebKit::UserData&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#20 0x00007fc58259569f in std::enable_if<!std::is_enum<WebKit::UserData>::value, bool>::type IPC::Decoder::decode<WebKit::UserData>(WebKit::UserData&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#21 0x00007fc582c8f2ef in IPC::TupleCoder<1ul, WTF::String, WebKit::UserData>::decode(IPC::Decoder&, std::tuple<WTF::String, WebKit::UserData>&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#22 0x00007fc582c8f2bf in IPC::TupleCoder<2ul, WTF::String, WebKit::UserData>::decode(IPC::Decoder&, std::tuple<WTF::String, WebKit::UserData>&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#23 0x00007fc582c8f0c8 in IPC::ArgumentCoder<std::tuple<WTF::String, WebKit::UserData> >::decode(IPC::Decoder&, std::tuple<WTF::String, WebKit::UserData>&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#24 0x00007fc582c8f049 in std::enable_if<!std::is_enum<std::tuple<WTF::String, WebKit::UserData> >::value, bool>::type IPC::Decoder::decode<std::tuple<WTF::String, WebKit::UserData> >(std::tuple<WTF::String, WebKit::UserData>&) ()
    at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#25 0x00007fc582ccbba9 in void IPC::handleMessage<Messages::WebProcessPool::HandleMessage, WebKit::WebProcessPool, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&)>(IPC::Connection&, IPC::D---Type <return> to continue, or q <return> to quit---
ecoder&, WebKit::WebProcessPool*, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&)) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#26 0x00007fc582ccb8eb in WebKit::WebProcessPool::didReceiveMessage(IPC::Connection&, IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#27 0x00007fc5825770d5 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#28 0x00007fc5826ec6a7 in WebKit::WebProcessPool::dispatchMessage(IPC::Connection&, IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#29 0x00007fc582707e7f in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#30 0x00007fc58255c0dc in IPC::Connection::dispatchMessage(IPC::Decoder&) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#31 0x00007fc58255c246 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#32 0x00007fc58255c438 in IPC::Connection::dispatchOneMessage() () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#33 0x00007fc58255bf7c in IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)::{lambda()#1}::operator()() () at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#34 0x00007fc582562b50 in WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)::{lambda()#1}>::call() ()
    at /build/test-wk2/lib/libwebkit2gtk-4.0.so.37
#35 0x00007fc579da1cc5 in WTF::Function<void ()>::operator()() const () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
#36 0x00007fc579db6040 in WTF::RunLoop::performWork() () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
#37 0x00007fc579dfd386 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::operator()(void*) const () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
---Type <return> to continue, or q <return> to quit---
#38 0x00007fc579dfd3aa in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
#39 0x00007fc579dfd326 in WTF::{lambda(_GSource*, int (*)(void*), void*)#1}::operator()(_GSource*, int (*)(void*), void*) const () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
#40 0x00007fc579dfd355 in WTF::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) () at /build/test-wk2/lib/libjavascriptcoregtk-4.0.so.18
#41 0x00007fc589018e42 in g_main_dispatch (context=0xe8e200) at gmain.c:3203
#42 0x00007fc589018e42 in g_main_context_dispatch (context=context@entry=0xe8e200) at gmain.c:3856
#43 0x00007fc5890191c0 in g_main_context_iterate (context=0xe8e200, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3929
#44 0x00007fc5890194e2 in g_main_loop_run (loop=0x3dd0b80) at gmain.c:4125
#45 0x00007fc589af07e5 in gtk_main () at gtkmain.c:1301

[Milan Crha]

These are the strings:

url:'gtk-stock://go-next?size=4'
str:'gtk-stock://go-next/?size=4'

[Michael Catanzaro]

This must be a URL parser bug. Alex, do you know how to fix it?

[Alex Christensen]

What is sending this message, and why does it think that 'gtk-stock://go-next?size=4' is a properly canonicalized URL?  It is not.  That is the problem.  The old URL::parse had a mode that would assume it was given a properly canonicalized URL when you use ParsedURLString, but that assumption is no longer respected.  All Strings given as input are canonicalized if needed.  Before enabling the new URLParser, this was not being canonicalized properly and now it is safely canonicalizing everything.  The solution to this problem is either to make sure we get a properly canonicalized URL or stop using ParsedURLString.

[Milan Crha]

The Evolution code constructs an HTML document which it passes to the WebKitWebView which contains such URLs. Thus it comes from a "user input". The Evolution never pretended to provide canonicalized URLs.

I'm not sure if it answers your questions.

[Michael Catanzaro]

It sounds like URLParser is expecting ResourceRequest to canonicalize URIs, while ResourceRequest expects URLParser to do that...?

[Alex Christensen]

There is somewhere where there is a URL constructor that uses ParsedURLString that should not because the String it is given is not a pre-canonicalized URL.

[Frédéric Wang (:fredw)]

(In reply to Alex Christensen from comment #6)
> There is somewhere where there is a URL constructor that uses
> ParsedURLString that should not because the String it is given is not a
> pre-canonicalized URL.

So from the back trace provided in comment 0, it seems this likely happens here: 

template<class Decoder>
ALWAYS_INLINE bool ResourceRequestBase::decodeBase(Decoder& decoder)
{
...
    String firstPartyForCookies;
    if (!decoder.decode(firstPartyForCookies))
        return false;
    m_firstPartyForCookies = URL(ParsedURLString, firstPartyForCookies);

I guess a workaround would be to put the url in canonical form:

URLParser parser(firstPartyForCookies);
m_firstPartyForCookies = parser.result();

But is it the proper place to do such a conversion?

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Frédéric Wang (:fredw) from comment #7)
> (In reply to Alex Christensen from comment #6)
> > There is somewhere where there is a URL constructor that uses
> > ParsedURLString that should not because the String it is given is not a
> > pre-canonicalized URL.
> 
> So from the back trace provided in comment 0, it seems this likely happens
> here: 
> 
> template<class Decoder>
> ALWAYS_INLINE bool ResourceRequestBase::decodeBase(Decoder& decoder)
> {
> ...
>     String firstPartyForCookies;
>     if (!decoder.decode(firstPartyForCookies))
>         return false;
>     m_firstPartyForCookies = URL(ParsedURLString, firstPartyForCookies);
> 
> I guess a workaround would be to put the url in canonical form:
> 
> URLParser parser(firstPartyForCookies);
> m_firstPartyForCookies = parser.result();
> 
> But is it the proper place to do such a conversion?

I guess the question is who's encoding this string in the first place, and does *it* think it has a properly canonicalized URL?

[Michael Catanzaro]

(In reply to Ms2ger from comment #8)
> I guess the question is who's encoding this string in the first place, and
> does *it* think it has a properly canonicalized URL?

We would need a full backtrace to know. The problem is somewhere in the web process, which should canonicalize the URI before sending it to the UI process (right, Alex?), but it's hard to know where the problem is because the backtrace does not show the message type. A backtrace taken with 'bt full' would show the message type, which would allow us some chance at figuring out where the underlying problem is.

That said... there is also a second bug here, which is that the untrusted web process should not be able to crash the UI process by sending a noncanonicalized URI (or any form of garbage). Since ResourceRequestBase::decodeBase is obviously message decoding code, it must assume that it has received invalid or malicious input, and not assert if input is unexpected. This implies that using ParsedURLString anywhere in a message decoder is a security bug. We should audit for this.

(In reply to Frédéric Wang (:fredw) from comment #7)
> I guess a workaround would be to put the url in canonical form:
> 
> URLParser parser(firstPartyForCookies);
> m_firstPartyForCookies = parser.result();
> 
> But is it the proper place to do such a conversion?

I believe yes, using URLParser in the decoder is proper and necessary. But it is not sufficient. There is someplace else where it is needed. Milan, I don't suppose you are still able to reproduce this issue? We'd really need a full backtrace to be able to fix this fully. Otherwise, we'll be able to fix the UI process crash by turning it into a message decode failure, but that's still a serious bug.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9)
> This implies that using ParsedURLString anywhere in a
> message decoder is a security bug.

Well, it's actually only a debug assert, so it won't be enabled in release builds. I guess Milan was testing a debug build at the time he reported this crash. Anyway, it's still a decoder bug that should be fixed.

[Milan Crha]

I can try to reproduce with the latest checkout and debug build, it'll only take a bit of time. I'll let you know when I know more.

[Alex Christensen]

I'd be ok with removing that ParsedURLString tag in ResourceRequestBase::decodeBase.  It doesn't actually do anything any more.

[Milan Crha]

Hrm, I cannot find the message right now, thus I cannot trigger the assertion. Pity I didn't write here a clue which message it was.