Closed Bug 478957 Opened 11 years ago Closed 11 years ago

Audit events when loading local files from another domain

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [sg:investigate])

Attachments

(1 file, 4 obsolete files)

When audio/video elements try to load a resource, but access to that resource is restricted by load policy, we should not send any events or make state changes which tell the page that the restricted resource exists.
Whiteboard: [sg:investigate]
This patch adds a mochitest that attempts video/audio loads of local sources from another domain while listening on all specified events and polling every 1ms to detect state changes. This test checks that we have the correct events in the correct order, that we don't leak state, and also checks that pages served from another domain can't load local media.

This patch is based on top of the patch for bug 465458, which updates the load algorithm to match the current spec. The mochitest passes on Windows and Linux (provided the patch for bug 465458 applied).

It's possible that there are weird edge cases that this mochitest misses. I will still go through and audit the code manually, and maybe others want to as well, but this test gives us some confidence and will allows us to detect regressions.
Assignee: nobody → chris
Attachment #363045 - Flags: superreview?(roc)
Attachment #363045 - Flags: review?(chris.double)
This is cool.

typo: "durationchane"

You should probably add a test for cross-origin HTTP loads of non-media content.

As a separate issue, you could think about what, if anything we might want to hide for cross-origin media loads.
Attached patch Patch v2 (obsolete) — Splinter Review
As Patch v1, with spelling error fixed and HTTP cross-domain non-media load test case added.
Attachment #363045 - Attachment is obsolete: true
Attachment #363187 - Flags: superreview?(roc)
Attachment #363187 - Flags: review?(chris.double)
Attachment #363045 - Flags: superreview?(roc)
Attachment #363045 - Flags: review?(chris.double)
Attachment #363187 - Flags: superreview?(roc) → superreview+
Attachment #363187 - Flags: review?(chris.double) → review+
Attached patch Patch v3 - v2 + check currentSrc (obsolete) — Splinter Review
When reviewing the code, I realized I'd not checked the currentSrc attribute in my mochitest, so I've updated the test to check that. I also expanded a comment about a setTimeout() which Chris Double asked me about.
Attachment #363187 - Attachment is obsolete: true
Attachment #363201 - Flags: superreview+
Attachment #363201 - Flags: review+
Result of code audit:

* All state changes to networkState > LOADING or readyState > HAVE_NOTHING are initiated by the decoder.
* Decoders are only constructed if the content policy does not veto the load, or if the channel has an appropriate mime type.
* The only other events sent in nsHTMLMediaElement are loadstart, error, emptied and play. These can be sent regardless of networkState or readyState, and you can't tell the difference between a 404 and a vetoed load with these events.

So I think we're ok.
Keywords: checkin-needed
Whiteboard: [sg:investigate] → [sg:investigate][needs landing]
The patch for bug 465458 must land before this one can.
Depends on: 465458
Keywords: checkin-needed
Whiteboard: [sg:investigate][needs landing] → [sg:investigate][needs landing 465458 first!]
this is ready to land now I guess
Whiteboard: [sg:investigate][needs landing 465458 first!] → [sg:investigate][needs landing]
Keywords: checkin-needed
Attached patch Patch v4 - unbitrotten (obsolete) — Splinter Review
As patch v3, but unbitrotten. This patch is based on top of bug 479711 and bug 479859. As such, the test is changed slightly, we now assume that networkState can be either <= NETWORK_LOADING, or NETWORK_NO_SOURCE (previously <= NETWORK_LOADING, NETWORK_NO_SOURCE is new). We also now never expect to receive an 'emptied' event, since now this only happens when we get an error during load, or we abort a load because a new one has started.
Attachment #363201 - Attachment is obsolete: true
Attachment #366221 - Flags: superreview+
Attachment #366221 - Flags: review+
Depends on: 479711, 479859
I left some "dump" debug calls commented in by mistake, turning them off again.
Attachment #366221 - Attachment is obsolete: true
Attachment #366223 - Flags: superreview+
Attachment #366223 - Flags: review+
Pushed: http://hg.mozilla.org/mozilla-central/rev/5e2e306f2803
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate][needs landing] → [sg:investigate][baking for 1.9.1]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dc3793b2c3e2
Group: core-security
Whiteboard: [sg:investigate][baking for 1.9.1] → [sg:investigate]
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
You need to log in before you can comment on or make changes to this bug.