394485 - <object> loads should be checked for malware

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Dave Camp (:dcamp)]

bug 384941 adds malware checking to docshell loads, but leaves <object> loads unchecked.  These loads should be checked for malware too.

Comment 1

[Jesse Ruderman]

I don't think it should be specific to <object>.  I think everything -- iframe, object, script, stylesheet, image -- should be subject to malware checking through the content policy API.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The content policy API is not rich enough to allow that.

Comment 3

[Jesse Ruderman]

I learned from chatting with biesi and dcamp that the malware-site database code lives on a separate thread in order to provide an asynchronous API.  That's great because it prevents disk latency from interfering with page load speed (the malware-site database is large enough that it can't just be in memory).  But it also means that it can *only* be used asynchronously.

I guess the solution to the "hacked ad server" scenario will have to be one of:

* Provide the same asynchronous API for <script> loads, so we can fire off the <script> request on a "blocked" HTTP channel that returns nothing until the malware database check has passed.

* Maintain a separate "temporarily block all loads from this site" list, and install a content policy hook when this list is non-empty.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't see how you can have malware in images or stylesheets, or even scripts. What exactly do we classify as malware and are trying to block here? If someone creates an image that crashes the jpeg decoder and does evilness to it, it seems hard to block that on a uri basis as someone can just move the image.

Doesn't <objects> that display webpages create docshells and thereby will use the current malware blocking?

Basically, can someone describe what type of attacks we want to prevent, or attach a testcase?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Doesn't <objects> that display webpages create docshells

Yes.

> and thereby will use the current malware blocking

No, since it doesn't start the load in the docshell.  Please see comments in bug 384941.

Comment 6

[Dave Camp (:dcamp)]

Attachment: check <object> loads with the URI classifier

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't know whether I'll be able to review this in a reasonable timeframe.  You may want to ask Christian instead.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Biesi, are you going to be able to get to this before beta2?

Comment 9

[Dave Camp (:dcamp)]

Attachment: updated to trunk

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One thing is making me worried, though it's not really introduced with this patch i guess. On redirects you dispatch a runnable to the current thread which is used to suspend the channel. Is there any guarantees that your runnable with run before the channel finishes loading? For example if we're redirected to a cached url?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 306582 [details] [diff] [review]
updated to trunk

Looks good otherwise.

Comment 12

[Dave Camp (:dcamp)]

(In reply to comment #10)
> One thing is making me worried, though it's not really introduced with this
> patch i guess. On redirects you dispatch a runnable to the current thread which
> is used to suspend the channel. Is there any guarantees that your runnable with
> run before the channel finishes loading? For example if we're redirected to a
> cached url?

This should be safe, at least with the existing set of channels.  The Suspend event will be posted to the queue before the new load is started, so will be processed before the pump has a chance to process any stream-ready events.  Once suspended, the pump will ignore stream-ready events until unsuspended.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 306582 [details] [diff] [review]
updated to trunk

sr=biesi

Comment 14

[Dave Camp (:dcamp)]

Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.72; previous revision: 1.71
done
Checking in content/base/src/nsObjectLoadingContent.h;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.h,v  <--  nsObjectLoadingContent.h
new revision: 1.27; previous revision: 1.26
done
Checking in docshell/base/Makefile.in;
/cvsroot/mozilla/docshell/base/Makefile.in,v  <--  Makefile.in
new revision: 1.68; previous revision: 1.67
done
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.888; previous revision: 1.887
done
Checking in docshell/base/nsDocShell.h;
/cvsroot/mozilla/docshell/base/nsDocShell.h,v  <--  nsDocShell.h
new revision: 1.221; previous revision: 1.220
done
RCS file: /cvsroot/mozilla/docshell/base/nsIChannelClassifier.idl,v
done
Checking in docshell/base/nsIChannelClassifier.idl;
/cvsroot/mozilla/docshell/base/nsIChannelClassifier.idl,v  <--  nsIChannelClassifier.idl
initial revision: 1.1
done
Checking in docshell/build/nsDocShellCID.h;
/cvsroot/mozilla/docshell/build/nsDocShellCID.h,v  <--  nsDocShellCID.h
new revision: 1.6; previous revision: 1.5
done
Checking in docshell/build/nsDocShellModule.cpp;
/cvsroot/mozilla/docshell/build/nsDocShellModule.cpp,v  <--  nsDocShellModule.cpp
new revision: 1.37; previous revision: 1.36
done