Bug 186535 – Bad optional access in WebCore::ContentSecurityPolicySource::portMatches

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

More fallout from switching to C++ 17 std::optional:

#2  0x00007fc701c65925 in std::__throw_bad_optional_access () at /usr/include/c++/8.1.0/optional:98
#3  std::optional<unsigned short>::value() const & () at /usr/include/c++/8.1.0/optional:1251
#4  WebCore::ContentSecurityPolicySource::portMatches () at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:95

[Michael Catanzaro]

Here's a better backtrace. It's built with -g1 so I don't have local variables, but the problem is clear enough: we now use libstdc++'s std::optional instead of WTF's implementation. The WTF implementation returns harmlessly when the std::optional is set to nullopt. But the standard std::optional throws. Then we abort immediately due to -fno-exceptions.

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fe0c6dd64ac in __GI_abort () at abort.c:79
#2  0x00007fe0d3784925 in std::__throw_bad_optional_access () at /usr/include/c++/8.1.0/optional:98
#3  std::optional<unsigned short>::value() const & () at /usr/include/c++/8.1.0/optional:1251
#4  WebCore::ContentSecurityPolicySource::portMatches ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:95
#5  0x00007fe0d37874e8 in WebCore::ContentSecurityPolicySource::matches ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:53
#6  0x00007fe0d37900fb in WebCore::ContentSecurityPolicySourceList::matches ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:138
#7  0x00007fe0d3783b7b in checkSource ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:60
#8  WebCore::ContentSecurityPolicyDirectiveList::violatedDirectiveForConnectSource ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:222
#9  0x00007fe0d378dc9b in WebCore::ContentSecurityPolicy::allPoliciesAllow<WebCore::ContentSecurityPolicyDirective const* (WebCore::ContentSecurityPolicyDirectiveList::*)(WebCore::URL const&, bool) const, WebCore::URL const&, bool>(std::function<void (WebCore::ContentSecurityPolicyDirective const&)>&&, WebCore::ContentSecurityPolicyDirective const* (WebCore::ContentSecurityPolicyDirectiveList::*&&)(WebCore::URL const&, bool) const, WebCore::URL const&, bool&&) const ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:321
#10 0x00007fe0d37858e8 in WebCore::ContentSecurityPolicy::allowConnectToSource ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:612
#11 0x00007fe0d36e81b7 in WebCore::EventSource::create ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/page/EventSource.cpp:71
#12 0x00007fe0d2b1f58f in WebCore::JSDOMConstructor<WebCore::JSEventSource>::construct ()
    at /run/build-runtime/WebKitGTK+/DerivedSources/WebCore/JSEventSource.cpp:145
#13 0x00007fe0d0f9f53b in JSC::NativeFunction::operator() ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/NativeFunction.h:50
#14 JSC::TaggedNativeFunction::operator() ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/NativeFunction.h:84
#15 handleHostCall ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1402
#16 0x00007fe0d0f9f802 in JSC::LLInt::setUpCall ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1449
#17 0x00007fe0d0f9e2c5 in llint_entry () from /usr/lib/x86_64-linux-gnu/libjavascriptcoregtk-4.0.so.18
#18 0x00007fe0d0f97613 in vmEntryToJavaScript ()
   from /usr/lib/x86_64-linux-gnu/libjavascriptcoregtk-4.0.so.18
#19 0x00007fe0d0eef6b7 in JSC::JITCode::execute ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/jit/JITCodeInlines.h:38
#20 JSC::Interpreter::executeProgram ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/interpreter/Interpreter.cpp:964
#21 0x00007fe0d1103cf1 in JSC::evaluate ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/Completion.cpp:103
#22 0x00007fe0d1103e84 in JSC::profiledEvaluate ()
    at /run/build-runtime/WebKitGTK+/Source/JavaScriptCore/runtime/Completion.cpp:118
#23 0x00007fe0d30c8a4d in WebCore::JSMainThreadExecState::profiledEvaluate ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/bindings/js/JSMainThreadExecState.h:78
#24 WebCore::ScriptController::evaluateInWorld ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/bindings/js/ScriptController.cpp:130
#25 0x00007fe0d33175c9 in WebCore::ScriptElement::executeClassicScript ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/dom/ScriptElement.cpp:387
#26 0x00007fe0d33228d7 in WebCore::ScriptElement::prepareScript ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/dom/ScriptElement.cpp:267
#27 0x00007fe0d3533e81 in WebCore::HTMLScriptRunner::runScript ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLScriptRunner.cpp:250
#28 0x00007fe0d3534b3e in WebCore::HTMLScriptRunner::execute ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLScriptRunner.cpp:140
#29 0x00007fe0d351f032 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:212
#30 0x00007fe0d351f1a9 in WebCore::HTMLDocumentParser::pumpTokenizerLoop ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:231
#31 0x00007fe0d351f3ee in WebCore::HTMLDocumentParser::pumpTokenizer ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:281
#32 0x00007fe0d3521bef in WebCore::HTMLDocumentParser::resumeParsingAfterYield ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/html/parser/HTMLDocumentParser.cpp:189
#33 0x00007fe0d37e05ee in WebCore::ThreadTimers::sharedTimerFiredInternal ()
    at /run/build-runtime/WebKitGTK+/Source/WebCore/platform/ThreadTimers.cpp:117
#34 0x00007fe0d1432d93 in operator() ()
    at /run/build-runtime/WebKitGTK+/Source/WTF/wtf/glib/RunLoopGLib.cpp:170
#35 _FUN () at /run/build-runtime/WebKitGTK+/Source/WTF/wtf/glib/RunLoopGLib.cpp:176
#36 0x00007fe0c97e7448 in g_main_dispatch (context=0x561130776320) at gmain.c:3176
#37 g_main_context_dispatch (context=context@entry=0x561130776320) at gmain.c:3829
#38 0x00007fe0c97e7838 in g_main_context_iterate (context=0x561130776320, block=block@entry=1, 
    dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3902
#39 0x00007fe0c97e7b32 in g_main_loop_run (loop=0x561130802340) at gmain.c:4098
#40 0x00007fe0d14331f0 in WTF::RunLoop::run ()
    at /run/build-runtime/WebKitGTK+/Source/WTF/wtf/glib/RunLoopGLib.cpp:96
#41 0x00007fe0d2a429b8 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> ()
    at /run/build-runtime/WebKitGTK+/Source/WebKit/Shared/unix/ChildProcessMain.h:61
#42 0x00007fe0c6dd80cb in __libc_start_main (main=0x56112e85ac80 <main()>, argc=3, argv=0x7ffd1f282ee8, 
    init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffd1f282ed8)
    at ../csu/libc-start.c:308
#43 0x000056112e85ad0a in _start () at ../sysdeps/x86_64/start.S:120

[Michael Catanzaro]

Reported bug #186536 to hopefully help surface these.

Problem is here:

    if (isDefaultPortForProtocol(m_port.value(), "http") && ((!port && url.protocolIs("https")) || isDefaultPortForProtocol(port.value(), "https")))
        return true;

which is wrong because m_port.value() is used unsafely without a call to m_port.has_value(), and port.value() is used unsafely without a call to port.has_value().

Crash occurs when url=https://pagure.io:8088/fedora-workstation/issue/42.

The CSP is on this page is:

Content-Security-Policy: default-src 'self' https:; script-src 'self' 'unsafe-eval' 'unsafe-inline' https://apps.fedoraproject.org; style-src 'self' 'unsafe-inline' https://apps.fedoraproject.org

But that's almost irrelevant, except to note that it doesn't include a URL with port 8088. In the usual case, the function returns earlier because port == m_port and there is no crash.

Writing a layout test seems difficult because the test server listens on 8080, so the same CSP works fine under WebKitTestRunner.

[Michael Catanzaro]

Created attachment 342471 [details]
Patch

[Michael Catanzaro]

Comment on attachment 342471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342471&action=review

> Source/WebCore/ChangeLog:11
> +        This is hard to test. If the layout test script-src-parsing-implicit-and-explicit-port-number
> +        continues to pass for WebKitLegacy, then I have at least probably not broken anything. To

The test is passing!

[EWS Watchlist]

Comment on attachment 342471 [details]
Patch

Attachment 342471 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8163556

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 342661 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Catanzaro]

Daniel, do you want to review this one?

[Michael Catanzaro]

Ping any WebKit reviewers, this one is critical

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> Ping any WebKit reviewers, this one is critical

Ping again. This can be handled by any reviewer. You can note:

 * That if either of the checks I added to this conditional are not satisfied, then the code will immediately crash
 * That the test added along with the code that added this conditional is still passing

The crash can be reproduced by visiting https://pagure.io/fedora-workstation/issue/60 on a non-Cocoa port, when built with GCC 7 or GCC 8.

[Michael Catanzaro]

Third ping, for any reviewer. Please, I need to use pagure. :P

[Daniel Bates]

Comment on attachment 342471 [details]
Patch

r=me

[Michael Catanzaro]

Comment on attachment 342471 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 342471 [details]
Patch

Clearing flags on attachment: 342471

Committed r233036: <https://trac.webkit.org/changeset/233036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41327866>