329869 - Dynamically loaded scripts don't degrade security state

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Daniel Veditz [:dveditz]]

If a secure (SSL) page includes a script from a non-SSL URI the security state correctly shows the "mixed" state. But once a secure page is loaded if it later dynamically adds a script node loaded from a non-SSL URI the secure state is not degraded to "mixed".

By contrast, adding a non-SSL iframe node does cause the icon to change to "mixed".

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

We should also check for dynamic insecure CSS, and adding secure CSS that includes non-SSL bindings etc.

Comment 3

[Daniel Veditz [:dveditz]]

And XMLHttpRequest or SOAP request to an insecure URL should flip it to mixed. We currently restrict XMLHttpRequest to same-origin so we don't need to do anything on that right now. SOAP can break out of the same-origin restriction if the 3rd party site allows it, and a similar mechanism is proposed for XMLHttpRequest in the future.

Comment 4

[Daniel Veditz [:dveditz]]

A fix seems unlikely for the 1.8.0 branch, it would involve some pretty deep changes in the security UI listener design that would be more appropriate for a final release with betas etc. to shake out problems.

Comment 5

[Jesse Ruderman]

Keeping this bug hidden doesn't protect users, so I'm making it public.

Comment 6

[Darin Fisher]

Not going to block FF2 for this bug, but we'd happily consider a safe patch.

Comment 7

[Johnathan Nightingale [:johnath]]

Dan, the testcase no longer works for me (though I confirm the script it uses still exists.)  Is this still an issue?

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: updated testcase

Updated testcase, the mozilla.com script used by the old one had been moved.

Comment 9

[Daniel Veditz [:dveditz]]

This is still an issue in the latest nightly, please reconsider the blocking decision.

Added a different testcase that better shows why it's important -- I've loaded insecure content from ha.ckers.org that can steal the bugzilla cookies, and it's still showing a "secure" lock icon.

Ideally we would fix bug 62178 and bug 321022 instead which would make this case not even come up (assuming we don't prompt). If we do the prompt (or have a pref bypass) though we'd then still have to fix this case.

Comment 10

[Robert Sayre]

sg:low, no patch, won't block.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Necessary but not sufficient

So the first problem is that docloader eats all notifications for loads which happen after onload.  This patch should fix that.  I still don't see the right behavior with it, though.

Comment 12

[Kai Engert [:KaiE:]]

We learned that we never retrieve OnStateChange with TRANSFERRING for the plain text JS. Boris patch fixes that.

In fact, with his patch applied, I can see that we do switch to "mixed state", however, only for a fraction of a second.

Because immediately afterwards, we receive a new notification that indicates a new top level load is starting. It is a wyciwyg load, which is the modified document (produced by document.write).

As soon as we receive that new toplevel START, we reset our state tracking (and forget all previous insecure loads). We find the wyciwyg channel has a security info associated with a secure state and decide to switch a good lock icon.

So, after having a chat with Boris, he recommends we should remember the broken state, and carry it around in the security info associated to the original document.

This is a good idea. And actually something like this has already been done recently! Honza had worked on it, we already have nsIAssociatedContentSecurity.

So that's good news. I was able to produce a very small patch that will attempt to restore our state, if the new request is a wyciwyg channel.

That helps, but it's still not sufficient.
I'll attach a patch, and write another comment...

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Patch v2 - more code, still not sufficient [NEVER LANDED ON M-C]

Comment 14

[Kai Engert [:KaiE:]]

So, with both patches applied, I load the test page, I click the button, and the lock icon immediately switches to broken. That's good.

Unfortunately, if I click "back", the page reverts to "secure", and if I click "forward", the page remains at "secure" :-/

Comment 15

[Kai Engert [:KaiE:]]

We load the first page, we go secure.

Click the button, we load the script, we remember one insecure item and go insecure.

we see the new toplevel load. before we reset, we save the state (one insecure item) to the security info.

we reset the state. we load the state. it's still the same security info. we succesfully retrieve the remembered insecure item. We remain at insecure.

click the back button. we save the sate (still having one insecure item).

we reset the state.

now, the new load (the original https address) is neither a bfcache load, nor a wyciwyg load. therefore, we don't attempt to restore. we go secure.

click the forward button. as always, we save our state (secure, no insecure sub items). we reset the state. we discover it's now wyciwyg. we load, but the associated info now says: secure

Comment 16

[Kai Engert [:KaiE:]]

and another problem:

- visit the test page
- click the button (we switch to the broken icon)
- cancel that dialog
- hit ESC to stop the load
- quit, with session restore enabled
- start again
- we incorrectly show the page as secure

Comment 17

[Kai Engert [:KaiE:]]

I changed the code to always attempt to retrieve the remembered insecure sub items.

When going backward to the original https, we still switch back to secure. This probably makes sense. But it means, the original page has its own security info, separate from the one that stores info for the modified page.

When going forward (to the modified page), we get a security info with "zero insecure". This surprises me.

When going forward, I would have expected to get the same security info object that we had had when we originally left the modified page.

So, maybe, when going forward to the modified page, we no longer have the old security info object from the modified page, but get a fresh copy from the good page?

Comment 18

[Honza Bambas (:mayhemer)]

-> me

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: patch 3

work in progress. this is additional patch (dependent on the two previous) that solves the problem. Someone (me :)) forget to add the associated content info to nss socket info serialization. Problem is that Wyciwyg channel loads on forward from cache and also restores the security info object from the cache.

I use the fact the nss socket info is always load from separate metadata field, i.e. there is no data behind the nss data. I append the four numbers (number of hi, low, broken and no security subloads) at the end of the metadata.

Comment 20

[Honza Bambas (:mayhemer)]

I think this bug is dependent on tests for mixed content. There could be scenario we don't see now for which these patches could still be inefficient.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Update of "necessary but not sufficient" patch to tip

Comment 22

[Kai Engert [:KaiE:]]

Comment on attachment 349436 [details] [diff] [review]
patch 3

Honza, does this patch fix the bug?


>+
>+  PRUint32 avail;
>+  nsresult rv = stream->Available(&avail);
>+  if (NS_SUCCEEDED(rv) && avail) {
>+    stream->Read32((PRUint32*)&mSubRequestsHighSecurity);
>+    stream->Read32((PRUint32*)&mSubRequestsLowSecurity);
>+    stream->Read32((PRUint32*)&mSubRequestsBrokenSecurity);
>+    stream->Read32((PRUint32*)&mSubRequestsNoSecurity);
>+  }


I guess, you query for "available" bytes, because the file on disk could contain an old object version, that did not contain those additional bytes.
Is my guess correct?

Does the stream contain multiple objects?

If the stream contains multiple objects, how do you know the "available bytes" are really what you expect?
Could the next bytes belong to the next object in the stream?

I guess we must use some form of object versioning.
Is there anythink we can use to distinguish old streams from new streams?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Version of "necessary but sufficient" patch that actually builds

Comment 24

[Boris Zbarsky [:bzbarsky]]

This is why all consumers of nsBinaryInput/OutputStream should really be versioning their data files (and dropping the deserialization on the floor on version mismatch, since it might cause security issues to deserialize an incorrect array length, say).

Does that not happen here?

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to comment #22)
(In reply to comment #24)

I count on fact the object is always in a separate file. I believe it is wrong, with your objections even more.

True solution compatible in all ways would be to have the version in the file name or stream identifier. If we create a record in old FF version it must not be accessible to newer and vice-versa.

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 349436 [details] [diff] [review]
patch 3

I don't see any kind of versioning in the ::Write and ::Read functions. Maybe there is something as part of the WriteCompoundObject, but there are variables written directly, in addition. Also I conclude the existing stream carries multiple object.

r-

I think in order to fix this bug, versioning must be introduced (and a mechanism to identify old streams without versioning/object identifiers).

Comment 27

[Honza Bambas (:mayhemer)]

Attachment: patch 3, v2 [Checkin comment 37]

Added versioning, confirmed by the tests.

Comment 28

[Kai Engert [:KaiE:]]

Comment on attachment 365331 [details] [diff] [review]
patch 3, v2 [Checkin comment 37]

Thanks Honza! r=kaie

Unless Boris wants a different approach... this seems like a reasonable approach to introduce versioning and handle the old non-versioned streams.

Comment 29

[Boris Zbarsky [:bzbarsky]]

That looks good to me.  Very nice solution!

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 362601 [details] [diff] [review]
Version of "necessary but sufficient" patch that actually builds

This was checked in as part of bug 480713.

Comment 31

[Honza Bambas (:mayhemer)]

Ops, sorry, there is still a patch to land.

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 365331 [details] [diff] [review]
patch 3, v2 [Checkin comment 37]

This patch was forgotten to land on both m-c and 1.9.1.

Comment 33

[Reed Loden [:reed]]

(In reply to comment #32)
> (From update of attachment 365331 [details] [diff] [review])
> This patch was forgotten to land on both m-c and 1.9.1.

Are you going to land it on m-c then?

Comment 34

[Honza Bambas (:mayhemer)]

Very soon.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #32)
> (From update of attachment 365331 [details] [diff] [review])
> This patch was forgotten to land on both m-c and 1.9.1.

Does that mean this bug isn't fixed1.9.1?

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to comment #35)
> Does that mean this bug isn't fixed1.9.1?

Exactly. This confusion comes from more then one patch attached to this bug. One of them was landed as part of another bug.

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 365331 [details] [diff] [review]
patch 3, v2 [Checkin comment 37]

http://hg.mozilla.org/mozilla-central/rev/4851a9d9a60c

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 365331 [details] [diff] [review]
patch 3, v2 [Checkin comment 37]

Does this patch still apply cleanly to 1.9.2?

Comment 39

[Samuel Sidler (old account; do not CC)]

(In reply to comment #38)
> Does this patch still apply cleanly to 1.9.2?

s/1.9.2/1.9.1.2

Comment 40

[Honza Bambas (:mayhemer)]

(In reply to comment #39)
> (In reply to comment #38)
> > Does this patch still apply cleanly to 1.9.1.2?

Yes it does and builds. I just have to confirm it fixes the problem (there is no automated test).

Comment 41

[Samuel Sidler (old account; do not CC)]

Comment on attachment 365331 [details] [diff] [review]
patch 3, v2 [Checkin comment 37]

Not for 1.9.1.2.

Comment 42

[Samuel Sidler (old account; do not CC)]

Honza: Have you confirmed that your patch fixes the problem on 1.9.1?

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 335617 [details] [diff] [review]
Patch v2 - more code, still not sufficient [NEVER LANDED ON M-C]

This bug has never been by (my) mistake fixed as per the description on either the trunk or branch. To explain:

There are 3 patches for this bug:
1. Attachment 335617 [details] [diff] "Patch v2 - more code, still not sufficient". This patch has never landed anywhere and is necessary to fix this bug!

2. Attachment 335595 [details] [diff] "Necessary but not sufficient". This patch has been landed as part of bug 480713 comment 24.

3. Attachment 365331 [details] [diff] "Patch 3". It is a regression fix caused by landing the second patch.


There were a test for this bug, created and landed BEFORE this bug got fixed with TODO check for the issue in the description. That test has been disabled because I locally figured out it was causing leaks. The test was never run on the product machines. Now I figure out the test is wrong, there have to be regular check, which is failing. After I apply the first patch, the test works as expected and also the manual test is working as expected. Only after that this bug is really fixed.

I'm giving this patch r+ and I am about to attach two patches of all missing changes in the code and tests for the trunk and for the branch.

Comment 44

[Honza Bambas (:mayhemer)]

Attachment: Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52]

Kai, please check the test changes, fairly simple.

Comment 45

[Honza Bambas (:mayhemer)]

Attachment: Missing changes for 1.9.1 branch [Checkin 1.9.1 comment 51]

Same as for m-c, excluding the tests. Checked by the test ported to 1.9.1 and by the manual test.

Comment 46

[Kai Engert [:KaiE:]]

Comment on attachment 393752 [details] [diff] [review]
Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52]

r=kaie

Comment 47

[Honza Bambas (:mayhemer)]

Comment on attachment 393752 [details] [diff] [review]
Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52]

Adding my r+ for nsSecureBrowserUIImpl changes.

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 393752 [details] [diff] [review]
Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52]

http://hg.mozilla.org/mozilla-central/rev/76c02c19390d

Comment 49

[Daniel Veditz [:dveditz]]

Comment on attachment 393752 [details] [diff] [review]
Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52]

This is the one we want for 1.9.2, we don't need to reland the bits landed in comment 37

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 393754 [details] [diff] [review]
Missing changes for 1.9.1 branch [Checkin 1.9.1 comment 51]

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 51

[Honza Bambas (:mayhemer)]

Comment on attachment 393754 [details] [diff] [review]
Missing changes for 1.9.1 branch [Checkin 1.9.1 comment 51]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c96e5e667a3b

Comment 52

[Honza Bambas (:mayhemer)]

Comment on attachment 393752 [details] [diff] [review]
Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/10595ea48572

Comment 53

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1 using DVeditz's testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20090930 Shiretoko/3.5.4pre. Mixed icon now appears.