594882 - URL classifier result ignored (malware site not blocked) because of bug 513008

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

I was trying to figure out what's happening in bug 589296.  One hour of brain debugging and I have a good theory what's going on there:

- nsHttpChannel is AsyncOpen'ed
- it calls Connect(true)
- it opens the cache entry, and always expects it asynchronously
- Connect returns NS_OK
- AsyncOpen starts the url classifier
- it immediately suspends the channel (nsHttpChannel::mSuspendCount = 1)
- before we get resumed by the classifier (before it finishes its job) we get OnCacheEntryAvailable
- it calls Connect(false)
- it creates the http transaction and its pump
- the transaction pump runs (we do AsyncRead, nsInputStreamPump::mSuspendCount = 0)
- it does the request and loads the response
- the response is a redirect, so we call the async redirect observers
- we call Suspend on the transaction: nsInputStreamPump::mSuspendCount = 1
- in the meantime classifier finishes its job and resumes us: nsHttpChannel::mSuspendCount = 0 and nsInputStreamPump::mSuspendCount = 0
- the transaction pump gets re-spin before we get the redirect callback
- it continues and finishes the channel load (we get OnStopRequest), we do a cleanup and cancel the channel
- then we get the redirect result, note that the error reported to the callback is NS_ERROR_INVALID_POINTER, quit strange
- we crash because the transaction has already been released

So, we have to postpone call to the cache entry callback, don't allow it sooner before we get resumed again.

Reporting as a new security bug, because Firefox 4 Beta 5 is not correctly classifying URLs before we start loading them.

Comment 1

[[e-mail]]

If this gets blocking+, I can take it.

Comment 2

[Honza Bambas (:mayhemer)]

And to explain why this bug was not present before patch for bug 513008 landed: an http channel was waiting for a cache entry asynchronously only when it was held by another channel.  And that cache entry could not be released sooner then the result from the classifier arrived, the time line was:

- nsHttpChannel 1 
- open and keep a cache entry (sync open)
- call to the classifier
- get suspended
                     - nsHttpChannel 2 
                     - open but wait for a cache entry (async)
                     - call to the classifier
                     - get suspended
- classifier is done
- get resumed
                     - get resumed (actually nothing to resume...)
- do a request, load a response
- release the cache entry
                     - get the cache entry
                     - load from cache

Comment 3

[Bjarne (:bjarne)]

Your brain debugging is more efficient than mine - I think you're right on spot here. :)

Comment 4

[Johnny Stenback (:jst)]

Blocking.

Comment 5

[Honza Bambas (:mayhemer)]

I am working on the patch + re-land of backout from bug 589296.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1

A general approach fix.  Whenever a channel wants to start the transaction pump or the cache pump while it is currently suspended, that start is delayed.  Pumps are then started when the channel gets resumed.

Now pushing to try server, done only some local tests and random browsing.  All seems to work correctly.

Comment 7

[Justin Dolske [:Dolske]]

Hmm, do we not have tests to check if the URL classifier is working? Or did this just break in such a way that it managed to pass existing tests?

Worth adding a high-level test to ensure that visiting a test URL in the DB correctly results in an attack/phish page being shown?

Comment 8

[Bjarne (:bjarne)]

(In reply to comment #6)
> A general approach fix.  Whenever a channel wants to start the transaction pump
> or the cache pump while it is currently suspended, that start is delayed. 
> Pumps are then started when the channel gets resumed.

Just an idea: Would it make sense to rather let the pump(s) "inherit" the value of mChannel::mSuspendCount when the channel creates them? I.e. make sure that a suspended channel creates suspended pumps in order to keep their suspendcount-semaphores in sync.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 474396 [details] [diff] [review]
v1

Dropping review flag.  This patch regularly failed on try server.  Going to investigate and react to all comments when I know more.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> Hmm, do we not have tests to check if the URL classifier is working? Or did
> this just break in such a way that it managed to pass existing tests?
> 
> Worth adding a high-level test to ensure that visiting a test URL in the DB
> correctly results in an attack/phish page being shown?

Bug 589585?  Excellent oversight of a security problem?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to comment #10)
> Excellent oversight of a security problem?
Ah, I it's a blocking bug, so no oversight, just missing link to bug 513008, sorry.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 474396 [details] [diff] [review]
v1

So, test failures were caused by non-closed cache entries when we failed resume.  That helped to fix the test timeouts.

However, before the patch for bug 513008 landed, we allowed to do the request as a prefetch but didn't allow load of the content to upper levels.  I wrongly assumed we even don't want to do the request before we check the URI ; that assumption is not correct, right?

So, we really need to sync the suspend counters on pumps to allow the prefetch but don't allow the content be processed, correct?

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Updated v1 patch.  Just for a reference...

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2

What Bjarne suggested, much cleaner and safer patch, suspend the pump as many times as the channel has been suspended.

Pushing to try again.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Fixed missing return NS_OK in each method (only a warning on windows).

Tested with classifier thread slowed rapidly down (::Sleep()) that we get the intermittent failure from bug 589585 w/o this patch and not w/ the patch.

Passed try server.

Comment 16

[Daniel Veditz [:dveditz]]

Do we need this on the older branches? The patch applies, but I guess without bug 513008 it's not biting us. Can we count on that or have we just gotten lucky?

Comment 17

[Honza Bambas (:mayhemer)]

No.  This is a change touching an old and sensitive code, we should not take this on stable branches.  Another reason is that the patch from bug 513008 that caused this bug has landed only on mozilla-central and is not planned to land on older branches, at least AFAIK.

Comment 18

[Honza Bambas (:mayhemer)]

Thanks biesi.

bz, do you want to check the patch as well or is Cristian's r+ enough?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 474574 [details] [diff] [review]
v2.1

biesi' r= is just fine here.  He knows this stuff way better than I do.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v2.1 + re-land of backout from bug 589296 [Check-in comment 21]

This what has to be landed.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 475562 [details] [diff] [review]
v2.1 + re-land of backout from bug 589296 [Check-in comment 21]

http://hg.mozilla.org/mozilla-central/rev/ec6c7ae80489