Bug 84352 - Browser Frame Injection Vulnerability
Summary: Browser Frame Injection Vulnerability
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: khtml (show other bugs)
Version: unspecified
Platform: Debian testing Linux
: NOR grave
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords:
: 84365 86796 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-07-02 11:22 UTC by Jose Antonio
Modified: 2005-04-13 21:45 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
patch (3.68 KB, patch)
2004-07-02 17:13 UTC, Dirk Mueller
Details
patch (4.84 KB, application/x-gzip)
2004-07-04 18:51 UTC, Waldo Bastian
Details
khtml-2.patch (11.77 KB, patch)
2004-07-07 14:59 UTC, Waldo Bastian
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jose Antonio 2004-07-02 11:22:40 UTC
Version:            (using KDE KDE 3.2.3)
Installed from:    Debian testing/unstable Packages
Compiler:          3.x 
OS:                Linux

See http://secunia.com/advisories/11978/ for a long description. The test confirms the vulnerability on Konqueror 3.2.3.
Comment 1 Germain Garand 2004-07-02 15:28:32 UTC
*** Bug 84365 has been marked as a duplicate of this bug. ***
Comment 2 Germain Garand 2004-07-02 16:55:49 UTC
As of now konqueror implements frame searching as described in
    http://www.w3.org/TR/html401/appendix/notes.html#notes-frames
    sec. B.8
which was the "common practice".

So, should we just ditch point 3) altogether:
   3- If no such frame was found in (2), apply step 2 to each window, in a
   front-to-back ordering. Stop as soon as you encounter a frame with exactly the
   same name.
(which is implemented in KonqMainWindow::slotOpenURLRequest)
or rather restrict the search to frames with a matching url.host()?





Comment 3 Waldo Bastian 2004-07-02 17:09:33 UTC
I think the frame lookup by konqueror in KonqView::hostExtension() should be moved out of konqueror and into the html part and then made subject to domain restrictions. I'm working on a patch to make that possible.
Comment 4 Dirk Mueller 2004-07-02 17:13:17 UTC
Created attachment 6539 [details]
patch

This patch adds domain checks for cross-frame part loading. it breaks
the case when the frame it should load is not reachable from the current
root of the frame tree. I'm not sure if that is relevant or not. 

Another possibility would be to only implement this stricter checking for
https urls, but its probably wise to check what the other browser vendors
came up with.
Comment 5 Waldo Bastian 2004-07-02 17:42:29 UTC
See also http://bugzilla.mozilla.org/show_bug.cgi?id=246448
Comment 6 Waldo Bastian 2004-07-02 18:01:52 UTC
It looks like the deutsch bank site that the heise article is linking to is now returning a frameless page.
Comment 7 Waldo Bastian 2004-07-02 18:32:15 UTC
http://bugzilla.mozilla.org/show_bug.cgi?id=246448#c37 contains a useful testcase
Comment 8 Waldo Bastian 2004-07-04 18:51:37 UTC
Created attachment 6550 [details]
patch
Comment 9 Waldo Bastian 2004-07-04 18:57:37 UTC
Patch #6550 implements comment #3. 

It tries to implement the behavior as described for the testcase linked to in comment #7 but I don't quite get that because htmlDocument().domain() does not
give me what I would expect to get. E.g. if a document containing a frameset has http://content.localhost/framespoof/contentframe.html as URL then I had expected the domain for that document to be "content.localhost", but instead it seems to get the domain from its parent frameset.
Comment 10 Waldo Bastian 2004-07-07 14:59:34 UTC
Created attachment 6586 [details]
khtml-2.patch

Updated khtml patch (Replaces khtml.patch from 6550)
Comment 11 Germain Garand 2004-07-08 00:07:00 UTC
I could only test Dirk's patch for now...
It looks simple and predictible enough to be safely applied on BRANCH, but maybe lacks flexibility... 

I found there is still a gap at least for form submissions in this patch: they use to go directly through openURLRequest(), letting it find a suitable frame. 
So http://bugzilla.mozilla.org/attachment.cgi?id=150586&action=view can still spoof us.
Also, the patch doesn't let script without target be executed.
Incrementally, it would at least need something like:

--- khtml_part.cpp	2004-07-03 19:49:00.000000000 +0100
+++ khtml_part.cpp.n	2004-07-03 18:14:35.000000000 +0100
@@ -868,6 +868,9 @@
 {
   destpart = this;

+  if (target.isEmpty())
+     return true;
+
   QString trg = target.lower();

   if (target == "_top") {
@@ -4350,6 +4353,9 @@
   }
   else
   {
+    KHTMLPart* dest;
+    if ( !allowCrossFrameTarget( args.frameName, dest ) )
+      args.frameName = "_blank";
     emit d->m_extension->openURLRequest( u, args );
   }
 }
Comment 12 Waldo Bastian 2004-07-08 17:37:55 UTC
Re #11: 6550 and 6586 address these issues by concentrating the checking in findFrame/findFrameParent

6539 also seems to break the case where a frame in another window is referenced, while 6550/6586 will handle that subject to domain restrictions.
Comment 13 Waldo Bastian 2004-07-19 17:08:29 UTC
Commited to CVS HEAD since it fixes the reported vulnerability, but the domains reported by the parts still seem to be wrong.
Comment 14 Waldo Bastian 2004-07-23 16:44:39 UTC
Ugh, in CVS the test on http://secunia.com/advisories/11978/ still fails.

That's because it runs in the
   if (!htmlDocument().isNull()) {

check which returns true. If I return false there the normal frame navigation on msdn.com stops working. The part with the frameset doesn't seem to have a htmlDocument at all???
Comment 15 Waldo Bastian 2004-07-28 17:20:53 UTC
The mystery in #14 is solved by changing the check into
   if (htmlDocument().isNull()) {

Boolean logic is always tricky :-}
Comment 16 Carsten Lohrke 2004-08-06 12:17:57 UTC
I got it right, that the first and third patch are needed? Are these patches simple to backport or do I have to care for something?
Comment 17 Waldo Bastian 2004-08-06 12:43:55 UTC
Backported patches send by mail
Comment 18 Germain Garand 2004-08-08 05:03:43 UTC
mmh, I made some comments on the related kfm-devel thread some time ago and CC-ed here, but I used the wrong email address so it never reached here.

Waldo, there seem to be some problems left concerning the frame that is chosen.
I found one good testcase today:

http://ktown.kde.org/~coolo/regression/output/

1) open in a tab
2) duplicate the tab
3) go back to first tab and select one testcase
=> will open in the_other_ tab.
Comment 19 Waldo Bastian 2004-08-08 13:46:30 UTC
Thanks, fixed now.
Comment 20 Stephan Kulow 2004-08-08 16:12:53 UTC
*** Bug 86796 has been marked as a duplicate of this bug. ***
Comment 21 Waldo Bastian 2004-08-11 21:07:05 UTC
*** Bug has been marked as fixed ***.
Comment 22 Rafał Rzepecki 2004-09-15 14:27:53 UTC
So how was it fixed in the end? And when? 
As of Konqueror 3.3, JavaScript open() function is still utterly broken... it breaks my e-banking site into pieces (literally). See testcase at http://barium.mine.nu/~divide/brokenjs/
Comment 23 Rafał Rzepecki 2005-04-13 21:45:10 UTC
It's OK now, thanks for your great work.