Closed Bug 1006781 Opened 10 years ago Closed 10 years ago

[B2G][MMS] No thumbnail preview when sending or recieving an MMS when CSP 1.0 parser is enabled

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g-v1.4 unaffected, b2g-v2.0 affected)

Status:
RESOLVED DUPLICATE of bug 1012663
Tracking Flags:
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- affected

People

(Reporter: jschmitt, Unassigned)

References

Details

(Whiteboard: [sms-most-wanted])

Attachments

(2 files)

log.txt
(deleted), text/plain
Details
issue: left with the CSP compliant pref; right without
(deleted), image/png
Details
Attached file log.txt (deleted) — 
Description:
Thumbnail images are missing when sending an image through the messaging app.

Repro Steps:
1) Update a Open_C to BuildID: 20140506040204
2) Open the Messaging app
3) Create a new message
4) Input a valid phone number
5) Attach an image from either Camera or Gallery
6) Notice no preview when sending the image

Actual:
There is no thumbnail preview when sending an image mms.

Expected:
Thumbnail images are shown when sending an mms.

2.0 Environmental Variables:
Device: Open_C 2.0
BuildID: 20140506040204
Gaia: 4af716f09747edfbea637f5b60f7fd7d0183f19b
Gecko: 81651ad5e43c
Version: 32.0a1
Firmware Version: FFOS_US_EBAY_P821A10V1.0.0B06_LOG_DL

Notes:
Repro frequency: 100%
See attached: logcat
Issue repros on 2.0 Buri build.

Issue does not repro on 1.4 Open_C build.

1.4 Environmental Variables:
Device: Open_C 1.4
BuildID: 20140506000202
Gaia: b1242f33981024de59b8b4c26bacff8b876211b1
Gecko: fe4080728c60
Version: 30.0
Firmware Version: P821A10-ENG_20140410
status-b2g-v1.4: --- → unaffected
Keywords: regression, regressionwindow-wanted
blocking-b2g: --- → 2.0?
Keywords: qaurgent
QA Contact: pcheng
Regression window was done using Buri on master builds.
Mozilla inbound Regression Window:

Last Working Environmental Variables:
Device: Buri MOZ
BuildID: 20140505103237
Gaia: e8a08a3f7a608993f0b302371e016e73faceea70
Gecko: 81587bb1f916
Version: 32.0a1
Firmware Version: v1.2-device.cfg

First Broken Environmental Variables:
Device: Buri MOZ
BuildID: 20140505111137
Gaia: e8a08a3f7a608993f0b302371e016e73faceea70
Gecko: 7b05ebf0a2d5
Version: 32.0a1
Firmware Version: v1.2-device.cfg

Gaia is the same on both builds, indicating that this is a Gecko issue.

Mozilla inbound pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=81587bb1f916&tochange=7b05ebf0a2d5
Keywords: regressionwindow-wanted
Maybe caused by bug 858787? Garrett - What do you think?
Flags: needinfo?(grobinson)
ni? Julien as well
Flags: needinfo?(felash)
I haven't tried it myself yet, is the preview missing in the composer while composing the message, or in the thread after sending the message, or both ?

If it's only in the composer (where we're using an iframe), then it could definitely be a CSP issue.

And actually, these messages are the proof:

05-06 13:29:02.723: E/GeckoConsole(2304): [JavaScript Warning: "Content Security Policy: The page's settings blocked the loading of a resource: An attempt to apply inline style sheets has been blocked" {file: "app://sms.gaiamobile.org/index.html#thread=4" line: 0 column: 0 source: "background-image: url(data:image/jpeg;ba..."}]
05-06 13:29:02.723: E/GeckoConsole(2304): [JavaScript Warning: "Content Security Policy: The page's settings blocked the loading of a resource: An attempt to apply inline style sheets has been blocked" {file: "app://sms.gaiamobile.org/index.html#thread=4" line: 0}]
05-06 13:29:03.123: D/NetlinkEvent(277): Unexpected netlink message. type=0x0


I don't know what we should do here; should we relax the CSP checks for the app? Is there an issue in the CSP check? Or should we find a way that does not fail with CSP?


Also, the patch from bug 995116 will change this (move from data/url to a blob/url) so maybe this bug will be resolved (althouth I doubt it). Is it possible to test with the patch from bug 995116 ?
Flags: needinfo?(felash)
Keywords: qawanted
Given that the potential cause has been backed out, we should retest this on today's build to see if this still reproduces.
jsmith, that was very likely caused by the patches from bug 858787 (they have since been backed out), per the logging in comment 5.

Note that the default certified app CSP *blocks* inline style, and always has [0] No certified app should use inline style and expect to work. As noted in bug 968906 (which will be resolved by bug 858787), this policy was not being fully enforced and so inline styles were incorrectly being allowed.

We have bug 968907 to track removing inline styles from all of the default certified apps that we ship.

[0] https://developer.mozilla.org/en-US/Apps/CSP
Flags: needinfo?(grobinson)
Blocks: 858787
Marking fixed by backout then.
blocking-b2g: 2.0? → 2.0+
Keywords: qawanted
As Garrett pointed out correctly, the default CSP policy for certified apps does not allow any inline styles [1]. We should not mark this as fixed 'by backout' but rather fix the problem of using inline styles for certified apps. As soon as 951457 lands, we will only have a parser that enforces spec compliant policies, which means CSP will block all inline styles for certified apps.

Julien, is there an easy way/fix to load that image from an external resource?

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#397
Flags: needinfo?(felash)
Talked with Christoph in person - he would like to keep this bug open to get the CSP issue resolved so that we can land CSP support without smoketest regressions.
blocking-b2g: 2.0+ → ---
Keywords: regression, smoketest
Summary: [B2G][MMS] No thumbnail preview when sending or recieving an MMS → [B2G][MMS] No thumbnail preview when sending or recieving an MMS when CSP 1.0 parser is enabled
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> As Garrett pointed out correctly, the default CSP policy for certified apps
> does not allow any inline styles [1]. We should not mark this as fixed 'by
> backout' but rather fix the problem of using inline styles for certified
> apps. As soon as 951457 lands, we will only have a parser that enforces spec
> compliant policies, which means CSP will block all inline styles for
> certified apps.
> 
> Julien, is there an easy way/fix to load that image from an external
> resource?

We're generating an iframe content here, and the image itself is dynamic, so it's really not ideal nor easy. Is it possible to create an iframe with relaxed CSP rules? What's the problem with inline styles exactly (apart from bad practice in general)?

I can only think of dirty things like generating a CSS "file" from a data or blob URL, but that does not really satisfy me.

FTR, we use a background image because we want to leverage the fact that background-size keeps the width/height ratio.
Flags: needinfo?(felash)
(In reply to Jason Smith [:jsmith] from comment #6)
> Given that the potential cause has been backed out, we should retest this on
> today's build to see if this still reproduces.

if it's still needed to confirm, the issue no longer reproduces on today's trunk build

Open C v2.0
BuildID: 20140507040203
Gaia: 870a5c518742665d36b17e7e88c2ab07d440b94c
Gecko: 417acde736e7
Version: 32.0a1
P821A10V1.0.0B06_LOG_DL
> Is it possible to create an iframe with relaxed CSP rules?

Hm, if the inline styles are only in the iframe, than CSP should not be interfering (CSP is not inherited by a protected document's frames). This might be a little different on B2G, though. Maybe a certified app's iframes also have APP_STATUS_CERTIFIED? A quick way to test this would be to add the mozbrowser attribute to the attachment thumbnail iframe, since it short-circuits the GetAppStatus call in nsDocument::InitCSP and prevents the default certified app CSP from loading.

Otherwise, this might be a bug.

> What's the problem with inline styles exactly (apart from bad practice in general)?

It enables an attacker who can inject style to perform window redressing aka phishing attacks. This is why it is blocked as part of the default certified app CSP.

> I can only think of dirty things like generating a CSS "file" from a data or blob URL, but that does not really satisfy me.

I'd want to avoid this workaround too. You will always have to change some of your development practices to conform with CSP, but I'd like to avoid making it onerous unless necessary.
I'll need to check where the inline style exactly is. How urgent is it?

Also, pardon this sort of dumb question, what do we call exactly 'inline style' in the CSP context?

1. Is it an attribute "style="?
2. Is it a "<style>" tag?
3. Is it specifying "element.style.backgroundImage = ..." in JavaScript? 
4. What about "element.style = "background-image: ..." ?
5. What about "element.setAttribute('style', 'background-image: ...')" ?
julienw, 1+2 are what is meant by "inline style". 3-5 are CSSOM setters, and *are not* be blocked by CSP (I just double-checked).

There was a proposal for CSP to control CSSOM (via style-src 'unsafe-eval', not 'unsafe-inline') [0] but that has stalled out in the WG.

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=873302
Can you please cc me in bug 968906?
Julien, if you tell me how I can reproduce this problem on my local machine, I can help you fix the problem with the inline style blocking? I do have an emulator setup, is that sufficient to run smoketests, or this particular test?
Minor correction to Comment 15: 5 is not a CSSOM setter, you are setting the style attribute on an element, and that will be blocked by CSP as "inline style". It's basically the same case as 1, only set by JS. So, to be clear:

1,2,5 are blocked by CSP as "inline style"
3,4 are not (CSSOM)
Random hunch from skimming the code. It looks like you use an iframe for draft attachments, but a div for non-draft attachments [0]. An iframe should not have the CSP of the parent document automatically applied/inherited - if it does, that's a bug. However, a div is of course subject to the parent docuemnt's CSP. Perhaps that is what's causing the problem here?

If you find that the CSP is being auto-applied to the iframe, then that is a bug we need to investigate.

needinfo for this question, and also Comment 17.

[0] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/attachment.js#L156
Flags: needinfo?(felash)
Yeah, I was actually building a current central right now, and enabling the pref, to reproduce. Because my question in comment 5 was never answered.

The actual used markup is made from templates at [1].

So it seems that none of these cases are working, including the inline "font-size" in the iframe. Seems like that at least the "style=" markup used to inject the background-image will need to be changed to (probably) a JS-based code using the CSSOM.

[1] https://github.com/mozilla-b2g/gaia/blob/014d16eaaf09fd9f0f57e5cac7aa2008dc764f2b/apps/sms/index.html#L472-L509


BTW, could the CSP being applied in an iframe also produce bug 963137 ?

From my PoV actually it looks fine if the CSP applies to an iframe that has the same origin than the application.
Sorry, I somehow missed comment 17.

You "only" need to send a MMS with an image to yourself. You don't even need to have network working, because when there is an error, the message is still added to the thread, so it should work on the emulator.

There are 2 issues then: we don't see the image in the thread (it's in a <div> with an inline style attribute) nor in the "composer" part when you compose the message (it's in an <iframe> that contains also a markup with an inline style attribute -- same markup).
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #22)
> Sorry, I somehow missed comment 17.
> 
> You "only" need to send a MMS with an image to yourself. You don't even need
> to have network working, because when there is an error, the message is
> still added to the thread, so it should work on the emulator.

Unfortunately it's not that simple. I can run the emulator, but there are no pre-existing picture available in the gallery and I can not take one myself because my desktop machine does not have a camera :-). I am pretty sure there is a way I can adb-push some picture into some folder, so I can access it in the emulator right? Maybe it's just easier to run the smoketest which causes that problem!
Flags: needinfo?(felash)
Holding the 'Home' button on my keyboard and pressing the 'power' button in the emulator allows you to take a screenshot, which I can then use to load into an MMS. Unfortunately the emulator gets stuck with the 'spinning wheel' right next to the image when attaching to an MMS. So I can't really tell if that image got loaded properly or not. What I can tell so far, there was no call to CSP that got blocked. Anyway, this needs further investigation. If I would have better explanation of the test-setup, that would simplify things a lot on my end.
You can try to push the reference workload using:

  APP=sms make reference-workload-light

From the Gaia directory.

This will push a small db of dummy messages containing MMS with images.

From there, you can display the various threads (most of them contains MMS, there is one near the start called BIG-THREAD-MMS containing only MMS). If you longclick a message you can also forward it, and this way you can exercize the "compose" issue.


If you can enable the CSP on Firefox, you can try to reproduce in Firefox too. Run:

  PROFILE_FOLDER=profile-sms DEBUG=1 DESKTOP=0 make
  /path/to/firefox --no-remote -profile ./profile-sms

Then load "http://sms.gaiamobile.org:8080" in this firefox session, and press CTRL-SHIFT-M to use the "responsive mode" that gives you a mobile form factor.

From there you can manipulate the SMS app with dummy data. The thread 052780 (the last one) contains MMS with images.

You can't attach images in this environment but you can also forward a message using a right click on a message.

Hope this helps reproducing on your side.
Flags: needinfo?(felash)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23)
> (In reply to Julien Wajsberg [:julienw] from comment #22)
> > Sorry, I somehow missed comment 17.
> > 
> > You "only" need to send a MMS with an image to yourself. You don't even need
> > to have network working, because when there is an error, the message is
> > still added to the thread, so it should work on the emulator.
> 
> Unfortunately it's not that simple. I can run the emulator, but there are no
> pre-existing picture available in the gallery and I can not take one myself
> because my desktop machine does not have a camera :-). I am pretty sure
> there is a way I can adb-push some picture into some folder, so I can access
> it in the emulator right? Maybe it's just easier to run the smoketest which
> causes that problem!

And you can probably push an image to /sdcard. This needs to be a small image (smaller than 5 Megapixels) to be picked up by the gallery.
julien, we dug into this today and verified that the iframe does indeed inherit the parent's CSP. This is intentional, because the iframe is created by document.createElement and doesn't have a src attribute. It gets a copy of the parent's principal, which includes its CSP [0]. See the discussion on seceng for more details.

[0] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#518

Either way, the code needs to be changed to use the CSSOM, as you suggested in Comment 21. We will be sending a follow-up email to dev-gaia shortly, so everyone is reminded me of the default CSP policies. If you have any major problems conforming to the default policy please let us know.
Garett, I'd like to know when you'd need this change to be done. Especially, does it need to be done before the end of next week, or can it wait the week after? This is to plan our team's work :) Thanks !
Julien, we are going to start landing patches for the new CSP implementation this week which will co-exist with the current implementation and live behind a pref for some time. To answer your question, you don't have to fix the problem this week, but the sooner the better.

When running the emulator with the new implementation (in other words blocking inline styles for certified apps) we realized that CSP is blocking several other inline styles (e.g. from the keyboard). We will reach out in an email to dev-gaia today and will make developers aware of the upcoming switch. We would like to do the switch as soon as possible, but also all of the discovered problems regarding inline style blocking in gaia needs to be rewritten first.
Ok thanks !

I'm setting a flag so that we won't forget the bug when we plan the next sprint.
blocking-b2g: --- → 2.0?
triage: according to comment 29, this needs to be fixed.
blocking-b2g: 2.0? → 2.0+
FYI we couldn't take this for the new sprint as we had feature work to finish. Please tell me if this is blocking for other work and I can try to reprioritize.
(In reply to Wesley Huang [:wesley_huang] from comment #31)
> triage: according to comment 29, this needs to be fixed.

This actually only needs to be fixed if the CSP 1.0 parser gets enabled, which is currently not the case. Given the current regression risk of that feature, we should push that into 2.1 at this point to decrease the # of regressions possible.

Not a blocker unless CSP 1.0 gets enabled.
blocking-b2g: 2.0+ → ---
(In reply to Jason Smith [:jsmith] from comment #33)
> (In reply to Wesley Huang [:wesley_huang] from comment #31)
> > triage: according to comment 29, this needs to be fixed.
> 
> This actually only needs to be fixed if the CSP 1.0 parser gets enabled,
> which is currently not the case. Given the current regression risk of that
> feature, we should push that into 2.1 at this point to decrease the # of
> regressions possible.
> 
> Not a blocker unless CSP 1.0 gets enabled.

Jason, Julien, just as an update: bug 951457 landed a few weeks ago; currently the new csp-parser lives behind a pref (security.csp.newbackend.enable) which we are going to flip for FF32. The new CSP parser only supports CSP 1.0+ and therefore will cause this to break.

When do you think, you can have that part updated? If I remember correctly, this was the only smoke test that failed when enabling the spec-compliant parser, right?
Flags: needinfo?(jsmith)
Flags: needinfo?(felash)
Can we followup offline on this with bbajaj? From the conversation I had with her on this, we wanted to have this wait until FF33 to avoid destabilizing 2.0 at this point, as we've already got too many regressions in the 2.0 release as is.
Flags: needinfo?(jsmith)
Are there other major bugs caused by CSP landing?  If this was identified 3 weeks ago seems like we should have a good idea of the risk by now.
(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #36)
> Are there other major bugs caused by CSP landing?  If this was identified 3
> weeks ago seems like we should have a good idea of the risk by now.

See the dependencies on bug 858787.

Note - this was preffed on for only a day when we were only doing 2.0 smoketesting, so we don't understand the full breadth of the regression risk right now. We instead only know of an initial set of fallouts that occurred when we flipped the switch for a day's period of time.
I'd like to have this as part of next sprint so that we can flip the pref as early as possible in the 2.1 cycle.
Flags: needinfo?(felash)
Whiteboard: [sms-most-wanted]
That would mean a fix by June 20th at the latest.
Blocks: 1021970
No longer blocks: 858787
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: