Closed Bug 338114 Opened 18 years ago Closed 18 years ago

Adblock 0.5.3.042 accesses content without XPCNativeWrappers

Categories

(addons.mozilla.org Graveyard :: Public Pages, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwkbugzilla, Assigned: morgamic)

References

Details

Attachments

(1 file)

I found at least two occasions on which Adblock 0.5.3.042 accesses content nodes without XPCNativeWrappers (simply doing a search for wrappedJSObject on the code). One is the event handler for the "adblock-pageblock" event that *explicitely* unwraps event.target before accessing properties such as ownerDocument. Note that this event is never generated by the extension itself, the code seems unused. Still, the event handler is registered. My testcase for this vulnerability didn't work - it seems that untrusted events aren't forwarded to chrome any more, so Adblock is lucky here.

Not that much luck with the second hole. Adblock's nsIContentPolicy implementation receives a node that isn't wrapped (at least on the 1.8 branch). Adblock was supposed to fix this and wrap the node explicitely. Unfortunately the "fix" looks like this (function adblockWrap() in component.js):

  if(!forceWrap && object.wrappedJSObject || !nativeWrappingProvided)
    return object; 
  else {
    // Wrap the object and return
  }

I talked to rue in December and brought the fact to his attention that an unwrapped node might have a property wrappedJSObject as well. I suggested he either simply calls new XPCNativeWrapper() regardless of whether the node is wrapped already (the constructor prevents double-wrapping anyway) or at least uses instanceof for the check. Rue agreed - and still, half a year later the bug is still there. I will attach a testcase.
Attached file Testcase
This testcase triggers content policies by setting the src property of an image node. Prior to this the property wrappedJSObject is set on the image. When viewed with a vulnerable Adblock installed Adblock's content policy will try to resolve the ownerDocument property on the *unwrapped* node, causing the message "You are vulnerable" to be displayed. This definitely happens in Firefox 1.5.0.3 with Adblock 0.5.3.042, probably in some situation with trunk builds as well. This won't happen in either Adblock Plus 0.5.11.3 (mcm's enhanced Adblock, properly doing new XPCNativeWrapped() on the node received) or Adblock Plus 0.7.0.1 (my ad blocking extension, uses lookupMethod for Gecko 1.7 compatibility).
Note that bug 337095 fixed the second issue.  Is this bug fixed now?
Depends on: 337095
I don't think so. It is great that you fixed the immediate security issue - but this doesn't change the fact that Adblock does stupid things that have an impact on security. Adblock still needs to be fixed, especially as it claims backwards compatibility with everything up to Mozilla 1.4. But we can probably clear the security flag after FF 1.5.0.5 is released.
Well, wait.  If adblock needs to be fixed, we should file a bug on adblock, or possibly on AMO if we want to blacklist adblock.  Bugs on Firefox about adblock being broken are really not that useful...
I agree - unfortunately there is nothing like an Adblock bug tracker or even a developer who would answer mails. I posted a few pointers to this bug in forums to make sure rue finds it when he comes out of his hiding again (I don't think he is reading bug mail). But maybe something like Tech Evangelism would be a more appropriate product :-/

I don't think there is a point in moving this bug to AMO, they don't seem to be interested.
Err, that's not true, we've been catching up on other things.  We can take care of this.  Here are our options:
* give Rue some time (a week?)
* if nothing happens
** delete adblock?
** force an update?
* and blocklist it for future versions?

Certainly this is a good case for why blocklisting and establishing policy surrounding it is important -- see:
* policy regarding abandoned add-ons (bug 278234)
* should we bump up blocklisting in priority for AMO? (bug 278234)

Wladimir - what would you prefer happen?  Shaver - any thoughts on the matter?
Assignee: nobody → morgamic
Group: security
Component: General → Web Site
Product: Firefox → Update
Target Milestone: --- → 2.1
Version: 1.5.0.x Branch → unspecified
My honest opinion: you should send rue a warning and if nothing happens (I'm pretty sure nothing will happen) - kill it. This isn't the first time we have a situation like this with Adblock and it definitely isn't the last one. See http://aasted.org/adblock/viewtopic.php?t=3056 for a list of bugs - and that was only a quick glance at the code, I'm sure I'll have five times more once I have time to finish my feature comparison. Unless we see somebody actually working on Adblock (and it needs lots and lots of work) it should be delisted.

Note: I am not exactly a neutral party here. I tried to reach an agreement with rue about taking over Adblock project and when this failed I started developing Adblock Plus.
QA Contact: general → web-ui
Don't think Mike meant to clear the security flag -- bug 303183 strikes again
Group: update-security
We can't blacklist extensions usefully today, because we don't have a client in the field that understands blacklists.  We could remove AdBlock from the site and fake an update, but that opens a can of policy worms that I don't want to open right now if I can avoid it.  (Worms range from our "common carrier" status of not being responsible for the behaviour of extensions, to dealing with an onslaught of complaints and spurious-or-not reports of other extensions with security bugs.)

But maybe I can't avoid it, so...

What other extensions have this pattern?  Given the source for them, how can we detect this case usefully?
If by "this pattern" you mean accessing content nodes without wrappers - I found this simply by searching for "wrappedJSObject", both hits were security relevant. As to other extensions - website integration category is most likely to have extensions using the same "trick". I checked and among the first 20 extensions (default sorting - by last updated I think) in this category GMail Skins, Bumble Search and Googlepedia use wrappedJSObject, in the first two cases content nodes are definitely accessed without wrappers (can't say for sure about Googlepedia, it has some weird manipulations with wrapped/unwrapped windows).

These holes are very unlikely to be ever exploited however, first because AFAICT only one web site could exploit this and second because none of these extension is anywhere near as popular as Adblock. So I checked the most popular list as well and found Stumbleupon and Torrent Search Bar who have the same issue. On the other hand, Adblock Plus, NoScript, Wizz RSS News Reader, GMail Space access wrappedJSObject for chrome objects which is perfectly fine.
Cc:ing mcm.ham so that he can help us get a fixed version to upload.
Adblock 0.5.3.043 uploaded by mcm.ham and approved by me, no more wrappedJSObject use anywhere else.  Huzzah!  (A slightly awkward policy move there to let mcm.ham upload as an author, but I think it's for the best, and I'll apologize to Rue if he's offended -- and he surfaces!)

Next week, or maybe this weekend, I'll get in contact with the authors of the other extensions mentioned by Wladimir, but for now this is all better.  Whew.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(In reply to comment #12)
> no more wrappedJSObject use anywhere else. 

Sorry, not exactly true. The "adblock-pageblock" event listener is still there and using wrappedJSObject to unwrap content nodes. Fortunately content can't trigger this event any more and Adblock itself doesn't...
Check again, the "adblock-pageblock" event listener should no longer unwrap the content node.
You are right, I didn't notice that you commented this line out. Then it is in fact all fine.
Target Milestone: 2.1 → ---
In the light of comment 7 - can this extension be moved into the sandbox now that there is one? There hasn't been any changes since than, rue is still missing in action and Adblock's homepage still offers to download the buggy 0.5.3.042 version. Adblock hasn't been updated for Firefox 2 and its forum has been overrun by spam (not that any developers made an appearance there in the last year). I think this more than qualifies it for the sandbox.
We should blacklist it, not sandbox it.  We _do_ have clients that understand blacklists now.

I've at least updated adblock.mozdev.org to redirect new users to the updated version on AMO, and the update.rdf file to update existing users. I didn't realize that my CVS account hadn't been disabled; last I'd checked, rue had been doing everything in his power to keep me from getting CVS access to mozdev.

I think blacklisting is a too-drastic measure at this point. Sandboxing seems much more appropriate.
> I think blacklisting is a too-drastic measure at this point.

As I understand the situation, there are open security holes in it and the author hasn't responded to the situation in months (not even to say "I'm working on it").  Do I understand incorrectly?
Well, comment #15 acknowledges that the version on addons.mozilla.org does not have security holes. And, as of earlier today, mozdev.org's download link is just a pointer to the fixed version on addons, so that's fine too.

Regarding authorship, Michael McDonald was quick to fix the version of Adblock being distributed by Mozilla, once he was given access. He doesn't have an account on mozdev, but when I was cleaning up my mailbox earlier this week I discovered that I do have such an account, so between the two of us I think we constitute an "author" that's, well, not vanished.
Ah, excellent.  I'm happy to be wrong.  I don't _want_ to blacklist extensions; I just want users to be secure.
Blacklisting supports version ranges.  What's the last insecure version?  We should blacklist that version and lower.
0.5.3.042, to my knowledge.
Yes, it is 0.5.3.042. But the problem is still that this extension is abandoned and has been for a long time. Since Ben has access to the web site he can make sure that Adblock gets at least some minimal maintenance in future and that there is a contact address if problems arise again. If he wants to do this of course. Otherwise this is still a prime candidate for the sandbox.
Ok, so does the following seem reasonable?

* Add Adblock 0.5.3.042 and lower to the blocked extensions list.
* Provide an info link (there's a details link in the blocked UI, IIRC) detailing the situation around minimal maintenance and provide alternatives to users.
Product: addons.mozilla.org → addons.mozilla.org Graveyard
I think that this should be made public by now.
Flags: needinfo?(jorge)
Group: client-services-security
Flags: needinfo?(jorge)
You need to log in before you can comment on or make changes to this bug.