Summary: | [URL Parser] ASSERTION FAILED: url == m_string in WebCore::URL::URL | ||
---|---|---|---|
Product: | WebKit | Reporter: | Milan Crha <mcrha> |
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
Status: | NEW --- | ||
Severity: | Normal | CC: | achristensen, bugs-noreply, calvaris, fred.wang, mcatanzaro, Ms2ger, rwlbuis |
Priority: | P2 | ||
Version: | WebKit Local Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified |
Description
Milan Crha
2017-01-16 08:48:28 PST
These are the strings: url:'gtk-stock://go-next?size=4' str:'gtk-stock://go-next/?size=4' This must be a URL parser bug. Alex, do you know how to fix it? 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. 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. It sounds like URLParser is expecting ResourceRequest to canonicalize URIs, while ResourceRequest expects URLParser to do that...? 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. (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? (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? (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. (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. 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. I'd be ok with removing that ParsedURLString tag in ResourceRequestBase::decodeBase. It doesn't actually do anything any more. 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. |