Bug 27721 - [WML] Deck access control is completly broken
Summary: [WML] Deck access control is completly broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20393
  Show dependency treegraph
 
Reported: 2009-07-27 11:26 PDT by Nikolas Zimmermann
Modified: 2010-08-17 07:49 PDT (History)
4 users (show)

See Also:


Attachments
Initial patch (38.71 KB, patch)
2009-08-01 18:07 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (48.99 KB, patch)
2009-08-09 15:36 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch - incorporating Georges comments (48.97 KB, patch)
2009-08-09 15:57 PDT, Nikolas Zimmermann
staikos: review+
staikos: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2009-07-27 11:26:47 PDT
Deck access control (a very important security feature in the WML world) is broken. Affects manual-tests/wml/deck-access-control.wml.
Comment 1 Nikolas Zimmermann 2009-08-01 18:07:54 PDT
Created attachment 33943 [details]
Initial patch
Comment 2 Eric Seidel (no email) 2009-08-06 20:47:15 PDT
Comment on attachment 33943 [details]
Initial patch

Does WML execute script?  Is it possible to change these values?  If so, setting to an invalid value will not clear domain or path since you if (value.isEmpty()) return before setting.  Is that intentional behavior?

What does this do, and why?
    if (!initialized) {
 58         if (WMLPageState* wmlPageState = wmlPageStateForDocument(this)) {
 59             if (Page* page = wmlPageState->page())
 60                 page->goBack();
 61         }
 62     }
Why not use early return there?

typo:
 71     // Remember that we'e successfully entered the deck

Seems:
bool initialized
is the wrong name.  It's accessAllowed or something like that.

Change seems large.  Too large for my little brain at this moment.  maybe we could/should add the layout tests first...

r- hoping that we could split this to make it easier to review.
Comment 3 Nikolas Zimmermann 2009-08-09 09:43:30 PDT
(In reply to comment #2)
> (From update of attachment 33943 [details])
> Does WML execute script?  Is it possible to change these values?  If so,
> setting to an invalid value will not clear domain or path since you if
> (value.isEmpty()) return before setting.  Is that intentional behavior?
No, WML doesn't have any JS support at all. WMLScript support is not desired to be implemented. So it's all static.
 
> What does this do, and why?
This is deck-access control. A deck is a document, a card is contained within a specific deck.
Suppose Card 1 of Deck A tries to enter Card 1 of Deck B. Deck B contains "<access domain='not-existant.com'...>", then Deck A is not allowed to enter Deck B. Deck B immediately goes back to the invoking card, and an error is displayed on the original page, that it failed to enter the target deck (task-execution-failure). This is specified WML behaviour.

>     if (!initialized) {
>  58         if (WMLPageState* wmlPageState = wmlPageStateForDocument(this)) {
>  59             if (Page* page = wmlPageState->page())
>  60                 page->goBack();
>  61         }
>  62     }
> Why not use early return there?
Fixed.

> 
> typo:
>  71     // Remember that we'e successfully entered the deck
> 
> Seems:
> bool initialized
> is the wrong name.  It's accessAllowed or something like that.
Fixed.

Uploading a slighly revised version soon, which also fixes a bug regarding intrinsic event execution upon task-execution-failure.
Comment 4 Nikolas Zimmermann 2009-08-09 15:36:26 PDT
Created attachment 34433 [details]
Updated patch
Comment 5 Nikolas Zimmermann 2009-08-09 15:57:27 PDT
Created attachment 34434 [details]
Updated patch - incorporating Georges comments
Comment 6 Nikolas Zimmermann 2009-08-09 16:34:23 PDT
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/wml/access-target-domain-deny-expected.txt
Adding         LayoutTests/http/tests/wml/access-target-domain-deny.html
Adding         LayoutTests/http/tests/wml/access-target-expected.txt
Adding         LayoutTests/http/tests/wml/access-target-path-deny-expected.txt
Adding         LayoutTests/http/tests/wml/access-target-path-deny.html
Adding         LayoutTests/http/tests/wml/access-target.html
Adding         LayoutTests/http/tests/wml/resources/access-target-domain-deny.js
Adding         LayoutTests/http/tests/wml/resources/access-target-domain-deny.wml
Adding         LayoutTests/http/tests/wml/resources/access-target-path-deny.js
Adding         LayoutTests/http/tests/wml/resources/access-target-path-deny.wml
Adding         LayoutTests/http/tests/wml/resources/access-target.js
Adding         LayoutTests/http/tests/wml/resources/access-target.wml
Adding         LayoutTests/http/tests/wml/resources/locked-deck.wml
Adding         LayoutTests/http/tests/wml/resources/unreachable-domain.wml
Adding         LayoutTests/http/tests/wml/resources/unreachable-path.wml
Adding         LayoutTests/wml/access-target-deny-expected.txt
Adding         LayoutTests/wml/access-target-deny.html
Adding         LayoutTests/wml/access-target-expected.txt
Adding         LayoutTests/wml/access-target.html
Adding         LayoutTests/wml/resources/access-target-deny.js
Adding         LayoutTests/wml/resources/access-target-deny.wml
Adding         LayoutTests/wml/resources/access-target.js
Adding         LayoutTests/wml/resources/access-target.wml
Adding         LayoutTests/wml/resources/locked-deck.wml
Sending        LayoutTests/wml/variable-reference-invalid-character-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/inspector/ConsoleMessage.h
Sending        WebCore/inspector/InspectorController.h
Sending        WebCore/loader/FrameLoader.cpp
Sending        WebCore/loader/FrameLoaderTypes.h
Sending        WebCore/page/Console.cpp
Sending        WebCore/page/Console.h
Sending        WebCore/page/Console.idl
Sending        WebCore/wml/WMLAccessElement.cpp
Sending        WebCore/wml/WMLAccessElement.h
Sending        WebCore/wml/WMLCardElement.cpp
Sending        WebCore/wml/WMLDocument.cpp
Sending        WebCore/wml/WMLErrorHandling.cpp
Sending        WebCore/wml/WMLPageState.cpp
Sending        WebCore/wml/WMLPageState.h
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebFramePrivate.h
Sending        WebKit/win/ChangeLog
Sending        WebKit/win/Interfaces/IWebFramePrivate.idl
Transmitting file data .............................................
Committed revision 46966.
http://trac.webkit.org/changeset/46966
Comment 7 Pavel Feldman 2010-08-17 07:49:06 PDT
Guys, is this console.lastWMLErrorMessage web-facing? Or is it only used in the layout tests? If latter, let us pass it via the layoutTestController instead. The reason I ask is that I saw some nasty ifdef(INSPECTOR) checks within WebCore/inspector/ConsoleMessage.cpp, while my assumption was that ConsoleMessage.cpp was inspector's property (and hence resides in the inspector folder).

I'd also like to remove InspectorController::consoleMessages() as a whole so that other clients would not use the interface. Your code seems to be the only client.