Closed Bug 1642729 Opened 9 months ago Closed 7 months ago

[MediaControl-Linux] Show artwork image on the virtual control interface (MPRIS)

Categories

(Core :: Audio/Video: Playback, task, P1)

Product:
Core
Component:
Audio/Video: Playback
Platform:
Desktop
Linux
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox81 --- fixed

People

(Reporter: chunmin, Assigned: chunmin)

References

Details

Attachments

(7 files)

Bug 1642729 - P1: Set track image to MPRIS
47 bytes, text/x-phabricator-request
Details | Review
data: URI image being displayed correctly in media controls on Plasma 5.19.2
147.49 KB, image/png
Details
Bug 1642729 - P2: Move same utils in SMTC and MPRIS backend to MediaControlUtils.h
47 bytes, text/x-phabricator-request
Details | Review
Bug 1642729 - P3: Only fetch image from http(s) URL
47 bytes, text/x-phabricator-request
Details | Review
Bug 1642729 - P4: Load URL without user-specific data
47 bytes, text/x-phabricator-request
Details | Review
Bug 1642729 - P5: Set the permission of the temporary file to 600
47 bytes, text/x-phabricator-request
Details | Review
Bug 1642729 - P6: Save images in a dictionary
47 bytes, text/x-phabricator-request
Details | Review

Show the MediaImage defined in media-session to the MPRIS interface.

Per bug 1623971 comment 12, we could use data url to work around the security issue.

I'd be interested in maybe working on this over a few weekends if someone tells me where to start, or maybe after the Windows infrastructure lands so I can see how it looks from there.

See Also: → 1642829
Type: enhancement → task

(In reply to Ilya K from comment #1)
I plan to sort some common things out in Windows part first and then bring them to Linux part.

  1. For security reason, Firefox shouldn't throw some random URL to the underlying API directly, so the plan is to fetch the image inside Firefox and then check if it's valid
  2. We need some cache mechanism to avoid fetching the same image repeatedly
See Also: → 1643102

I tried using data url on Ubuntu this morning but it doesn't work.

The image is from http://rust-lang.org/logos/rust-logo-16x16.png
and it's converted data uri from is: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAABOUlEQVR4AYTMAecUQRjH8QfAvYR9C3WXpCtpk6TdPYlWOoSIO0S5UpLewIDRu1jsCwh0kAEQCEBBADQYAL++fx7+Z1n35WPGM4+xaV+aKzUCErJLCKhtrs/N1QoRBZpREFHZaZ+aZYUBckfsIfQQvkFuwOUnH5prEcJvvMfC54L8vvC3vxCiGb1rr9co0Bmj7+8gFNT2tr0RIPTQGTuMkAv2ur2ZoCn8wUeXIYwQegjJ9u2tDGFpxCn3D0d8h9wPjJDL9qq7kyGsjPw+57nvLCFke9ndTRDeGHHK/cQaW8j9muwke9HdCxAyNIX13BuCbbv7NQo04za+QhMFtRk96x5ECD2EhU26mE12opn3dPOwwgC5Iw5YuYPP5AZUdtqTzaMKEQWaURBR2VyPN02NgITsEgL+DyM7AwBIx19/QAPbbQAAAABJRU5ErkJggg==

The data uri is converted from

I worked out a prototype, by creating a temp image file (via nsIFile) and feed it to MPRIS. The patch is quite ugly now though.

I found that "mpris:artUrl" has some kind of cache mechanism, at least on Ubuntu 18.04:

  1. The image shown on the notification bar won't be updated as long as the URL of the image is the same, even if the whole image file is overwritten/updated
  2. Once the image URL is used, its image is cached. Even the original image is destroyed
    • For example,
      1. Open Firefox with a media element. The MPRIS would start automatically.
      2. Feed image A whose filename is test-1.png to "mpris:artUrl", the MPRIS image will be image A
      3. Feed image B whose filename is test-2.png to "mpris:artUrl", the MPRIS image will be image B
      4. Close Firefox
      5. Open Firefox with a media element again. The MPRIS would start automatically.
      6. Feed image C whose filename is test-1.png to "mpris:artUrl", the MPRIS image will be image A
        • What a surprise! I guess it's image C, but it's not!
    • Therefore, it's better to create a unique image URL for each image. Now I use a timestamp as a unique identifier.

Sorry for the slow response, but data: URIs definitely do work for me on Plasma. Other implementations might not support them, though.

Tim,

Due to the limitation of the MPRIS in the gnome-desktop environment (e.g., ubuntu), the implementation needs to download the image specified by an URL to a local file. Since the local image file could be read by other applications if it has permission. For privacy, this process runs only when tracking-protection is disabled but I am not sure if it's enough. I was wondering if you could share your opinions. Or do you know who is the right person I should ask?

Flags: needinfo?(tihuang)

I think it's fine in terms of privacy because the image is located in a local file. So, there is no network request made. And we don't really block requests to the local resource for tracking-protection. The only privacy threat I can think of is that third-party software might be able to inspect image files to know information about users, maybe about their tastes about music. But, I think it's a low privacy risk if we don't write users' data into the image file. That is my two cents.

Steve, could you share your insight about this? Thanks.

Flags: needinfo?(tihuang) → needinfo?(senglehardt)
Attachment #9157808 - Attachment description: Bug 1642729 - Set track image to MPRIS → Bug 1642729 - P1: Set track image to MPRIS

(In reply to C.M.Chang[:chunmin] from comment #7)

Due to the limitation of the MPRIS in the gnome-desktop environment (e.g., ubuntu), the implementation needs to download the image specified by an URL to a local file. Since the local image file could be read by other applications if it has permission. For privacy, this process runs only when tracking-protection is disabled but I am not sure if it's enough. I was wondering if you could share your opinions. Or do you know who is the right person I should ask?

Would you mind to share some additional context here:

  1. When the image is downloaded, does that request contain credentials (e.g., cookies)? Do you know the loadingPrincipal for this request (i.e., is it loading from system privileged code or within content)?
  2. What do you mean by "For privacy, this process runs only when tracking-protection is disabled but I am not sure if it's enough."?

Since the local image file could be read by other applications if it has permission.

This isn't my area of expertise, but I suspect this is true with any files we store on disk. Another application could also just read our history database.

We'd only have a concern here if web content could somehow probe to determine whether this image has already been downloaded. Is it possible for arbitrary web content to load the local file?

Flags: needinfo?(senglehardt) → needinfo?(cchang)

(In reply to Steven Englehardt [:englehardt] from comment #10)

  1. When the image is downloaded, does that request contain credentials (e.g., cookies)? Do you know the loadingPrincipal for this request (i.e., is it loading from system privileged code or within content)?

The image would only be downloaded within the chrome process. The URL is loaded via asyncOpen in nsIChannel. It would do some security check about the passed URL.

  1. What do you mean by "For privacy, this process runs only when tracking-protection is disabled but I am not sure if it's enough."?
  1. The image would only be downloaded only when "privacy.trackingprotection.enabled" is false
  2. If the image is download, the permission of the download image file on Linux would be set to 644. Permission 644 on Linux means:
    • The file is readable and writeable by the owner of the file
    • The file is readable by users in the group owner of that file
    • The file is readable by everyone else

The download image might give a hint about what music or video the user is listening to or watching, so that's why the (1) above is checked for now.

I wonder

  1. Is it necessary to check "privacy.trackingprotection.enabled" before downloading an image? I am not sure if it meets what "privacy.trackingprotection.enabled" was designed to do.
  2. Are other preferences needed to be applied?
  3. Should the file-permission need to be changed to 600 (only readable and writeable by the owner of the file) or other?

Since the local image file could be read by other applications if it has permission.

This isn't my area of expertise, but I suspect this is true with any files we store on disk. Another application could also just read our history database.

I saw the file permission of cookies.sqlite is 644 as well, at least in Firefox Nightly.

We'd only have a concern here if web content could somehow probe to determine whether this image has already been downloaded. Is it possible for arbitrary web content to load the local file?

I don't find a Browser Extensions API that allows a browser extension to read local files. So it should be fine.

However, the add-on developer can use XPCOM nsIFile interface in the past. So if users install an unauthenticated add-on that calls nsIFile. That add-on might have a way to read the downloaded image.

Flags: needinfo?(cchang)
Flags: needinfo?(senglehardt)

We would definitely want to ensure the image is downloaded credential-less and using the Null principal. Christoph can review/guide you on how to make sure that is the case. We would want to restrict the schemes allowed, it should only be http/https - no about: or file:// or otherwise.

I am not thrilled about it being downloaded in the chrome process; but I'm not sure a reasonable alternative exists.

I do not feel like checking privacy.trackingprotection.enabled is necessary; but I don't know under what circumstances this code is being hit. If we're already sending media data over this interface I feel like web tracking is not really coming into play here?

This entire feature should be behind a pref 'just in case'.

I don't know if we have a standard for what permission bits we set on what are essentially temp files created on Linux. Hopefully this is an easy question for gcp?

Flags: needinfo?(gpascutto)
Flags: needinfo?(ckerschb)

(In reply to C.M.Chang[:chunmin] from comment #11)

(In reply to Steven Englehardt [:englehardt] from comment #10)

  1. When the image is downloaded, does that request contain credentials (e.g., cookies)? Do you know the loadingPrincipal for this request (i.e., is it loading from system privileged code or within content)?

The image would only be downloaded within the chrome process. The URL is loaded via asyncOpen in nsIChannel. It would do some security check about the passed URL.

Thanks. Tom has already flagged the right folks that can give advice from here.

  1. What do you mean by "For privacy, this process runs only when tracking-protection is disabled but I am not sure if it's enough."?
  1. The image would only be downloaded only when "privacy.trackingprotection.enabled" is false
  2. If the image is download, the permission of the download image file on Linux would be set to 644. Permission 644 on Linux means:
    • The file is readable and writeable by the owner of the file
    • The file is readable by users in the group owner of that file
    • The file is readable by everyone else

The download image might give a hint about what music or video the user is listening to or watching, so that's why the (1) above is checked for now.

I wonder

  1. Is it necessary to check "privacy.trackingprotection.enabled" before downloading an image? I am not sure if it meets what "privacy.trackingprotection.enabled" was designed to do.
  2. Are other preferences needed to be applied?
  3. Should the file-permission need to be changed to 600 (only readable and writeable by the owner of the file) or other?

From a web tracking perspective my only concern is whether a remote party could use the image requests to learn about your listening history and tie that to your web browsing. How does the browser know the URL to fetch the image? Who would ultimately provide that URL?

Checking privacy.trackingprotection.enabled is not the right approach, as you suspected. This pref is set to true when the user has ETP Strict enabled -- it basically signifies whether or not the feature is enabled.

A good first step here would be to ensure the request is not credentialed. You can do that by adding the LOAD_ANONYMOUS flag to the channel. This will strip cookies and HTTP Auth credentials from the request.

Since this load is from chrome it's unclear to me if the URL classifier will run against the request. Assuming it's an arbitrary URL specified by a remote party, it seems prudent to also run the channel through the URL classifier and block requests to known tracking domains (e.g., if the album image was loaded from "doubleclick.net" for some reason). Dimi should know whether the request will be classified, and if there's a way to force classification.

Since the local image file could be read by other applications if it has permission.

This isn't my area of expertise, but I suspect this is true with any files we store on disk. Another application could also just read our history database.

I saw the file permission of cookies.sqlite is 644 as well, at least in Firefox Nightly.

We'd only have a concern here if web content could somehow probe to determine whether this image has already been downloaded. Is it possible for arbitrary web content to load the local file?

I don't find a Browser Extensions API that allows a browser extension to read local files. So it should be fine.

However, the add-on developer can use XPCOM nsIFile interface in the past. So if users install an unauthenticated add-on that calls nsIFile. That add-on might have a way to read the downloaded image.

Thanks! I was mainly concerned with whether the images were ultimately web-accessible. I.e., such that an unrelated web page could probe a set of URLs to determine whether a user has listened to a certain album. (This is similar to webpages probing web-accessible resources from extensions to determine which extensions are installed -- e.g., https://dl.acm.org/doi/abs/10.1145/3029806.3029820?download=true). It sounds like that's not a concern here.

Flags: needinfo?(senglehardt) → needinfo?(dlee)

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #12)

We would definitely want to ensure the image is downloaded credential-less and using the Null principal. Christoph can review/guide you on how to make sure that is the case. We would want to restrict the schemes allowed, it should only be http/https - no about: or file:// or otherwise.

The current implementation skips the URL starts with file://. It's easy to add a check to skip about: as well. The passed URL will be loaded via asyncOpen in nsIChannel. For http or https case, nsHttpChannel::AsyncOpen will be called. It seems some content security check is done there.

If we're already sending media data over this interface I feel like web tracking is not really coming into play here?

Currently (without adding the patches here), the information sent over MPRIS includes track title, track artist, and album name, without checking the privacy.trackingprotection.enabled.

What the patches here do is to pack a path to a local image file to the information sent over the MPRIS as well, besides the track info above. So maybe we can skip checking privacy.trackingprotection.enabled and focus on what permission we should set for a temp file created on Linux?

This entire feature should be behind a pref 'just in case'.

For now, this feature is behind dom.media.mediasession.enabled, which is only enabled in Firefox Nightly.

I don't know if we have a standard for what permission bits we set on what are essentially temp files created on Linux.

From a quick scan through this bug, it looks like the hostile application would potentially be running as the same user? So then it doesn't really matter? If the attacker is potentially another user, it may.

So the question is what API is used to create this "temp file". But looking at the code, it's just doing:

rv = mLocalImageFile->Create(nsIFile::NORMAL_FILE_TYPE, 0644);

So it's gonna be world readable.

Flags: needinfo?(gpascutto)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #15)

So the question is what API is used to create this "temp file". But looking at the code, it's just doing:

rv = mLocalImageFile->Create(nsIFile::NORMAL_FILE_TYPE, 0644);

So it's gonna be world readable.

Does gecko have Web API or Browser Extension API that allows a third-party application to read the local files? If not, does changing 0644 to 0600 make it a bit safer?

Flags: needinfo?(gpascutto)

If we have a way to check the image specified by the URL is secure, then we don't need to download the image. We can forward the URL that has been checked to the MPRIS interface directly.

The passed URL will be loaded via asyncOpen in nsIChannel. For http or https case, nsHttpChannel::AsyncOpen will be called. It seems some content security check is done there.

Now the security-check is done by nsContentSecurityManager in nsHttpChannel::AsyncOpen, via FetchImageHelper::FetchImage(). Assume that check does the right job. As a result, if the image could be fetched successfully, then we can assume the URL specifying the image is secure. Then the URL could be forwarded to MPRIS directly, instead of saving a local copy from the fetched image and forwarding its path. Is this approach better?

(In reply to Steven Englehardt [:englehardt] from comment #13)

Since this load is from chrome it's unclear to me if the URL classifier will run against the request. Assuming it's an arbitrary URL specified by a remote party, it seems prudent to also run the channel through the URL classifier and block requests to known tracking domains (e.g., if the album image was loaded from "doubleclick.net" for some reason). Dimi should know whether the request will be classified, and if there's a way to force classification.

Right now we don't have a specific flag to force classification, but if the channel is not created by System Principal, URL classifier should classify it.
From Comment 17, it seems the image is fetched by FetchImageHelper::FetchImage(), which uses null principal if I understand correctly, so the channel should be classified.

Flags: needinfo?(dlee)

(In reply to C.M.Chang[:chunmin] from comment #14)

The current implementation skips the URL starts with file://. It's easy to add a check to skip about: as well. The passed URL will be loaded via asyncOpen in nsIChannel. For http or https case, nsHttpChannel::AsyncOpen will be called. It seems some content security check is done there.

The more we can restrict, the better obviously. So if we know that the file is only http(s), then why not flip the model around and only allow that? allow-list VS deny-list. Using FetchImageHelper::FetchImage() uses a NullPrincipal and hence can be used for the job, though I suggest we add the LOAD_ANONYMOUS flag as well so to not send any cookies. Other than that, I think we are good!

Flags: needinfo?(ckerschb)

Does gecko have Web API or Browser Extension API that allows a third-party application to read the local files?

Extensions are allowed to communicate with native apps, which could read those files.

If not, does changing 0644 to 0600 make it a bit safer?

As already explained, this makes no difference whatsoever because it would be running as the same user anyway. If the file contains highly sensitive content, perhaps it should sit somewhere where content processes don't have access to it (i.e. where it would be blocked by sandbox restrictions)? I don't know if the current dir that is used qualifies.

I can't really give meaningful input here without understanding what the threat model would be. I can only observe what is technically possible and what is being done right now, and I'm not sure if that's really helpful :)

Flags: needinfo?(gpascutto)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #20)

As already explained, this makes no difference whatsoever because it would be running as the same user anyway. If the file contains highly sensitive content, perhaps it should sit somewhere where content processes don't have access to it (i.e. where it would be blocked by sandbox restrictions)? I don't know if the current dir that is used qualifies.

The permission of cookies.sqlite on my linux is -rw-r--r-- (0644). Is it something that should be taken care of? (open another bug maybe)

The URL specifying the track image is only valid if its scheme is http
or https.

Depends on D82806

The URL specifying the track image should be loaded without any
user-specific data like cookie headers, or HTTP Auth credentials.

Depends on D83694

Attachment #9163873 - Attachment description: Bug 1642729 - Only fetch image from http(s) URL → Bug 1642729 - P3: Only fetch image from http(s) URL
Attachment #9163874 - Attachment description: Bug 1642729 - Load URL without user-specific data → Bug 1642729 - P4: Load URL without user-specific data

Hi,

I wonder if saving a temporary file is a big concern (comment #20) to stop shipping the code. I am not sure what's the protocol when having a privacy concern. Is this risk acceptable if the current code is shipped? Do we have a procedure to decide whether the risk is acceptable or not?

If leaking the data through a browser extension that communicates with the third-party native app on the computer is a real concern, are all the data stored in the computer with 0644 permission (e.g., cookies, session-storage, ...) vulnerabilities?

Aside from that, the media session's information would be exposed to the D-bus interface, so any application or script reading data from D-bus can get the track info sent from Firefox, such as track title, album name, artist name, which should give more information than a temporary image. If browser extension has a way to communicate with the native app that reads data from D-bus, then all the track info could be exposed to a browser extension. Is that something we need to concern?

Flags: needinfo?(senglehardt)
Flags: needinfo?(gpascutto)
Flags: needinfo?(ckerschb)
Flags: needinfo?(astevenson)

The permission of cookies.sqlite on my linux is -rw-r--r-- (0644). Is it something that should be taken care of?

That's a valid point. I don't see why we should have any files in the user profile that are world readable.

(And presumably, the fact that we're doing that immediately renders all local-other-user considerations moot until that would be fixed)

Flags: needinfo?(gpascutto)

(And presumably, the fact that we're doing that immediately renders all local-other-user considerations moot until that would be fixed)

We do remove world/group-execute permissions on the profile dir which means the file still isn't readable. But there's still an issue here with best practices. dveditz has pointed out we fixed this at some point, but behavior has regressed since. Checking an old profile seems to confirm that older files in the profile were indeed only user readable.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

We do remove world/group-execute permissions on the profile dir which means the file still isn't readable.

So, is it ok to save a file temporarily with 0644 permission under a dir without world/group-execute permissions? It follows the same manner.

So, is it ok to save a file temporarily with 0644 permission under a dir without world/group-execute permissions? It follows the same manner.

I would say yes but why not use 0600 to begin with? We want to fix this for the profile.

The temporary files should be saved under a dictionary without
world/group-execute permissions

Depends on D85075

(In reply to C.M.Chang[:chunmin] from comment #24)

I wonder if saving a temporary file is a big concern (comment #20) to stop shipping the code. I am not sure what's the protocol when having a privacy concern. Is this risk acceptable if the current code is shipped? Do we have a procedure to decide whether the risk is acceptable or not?

Let's loop in Tom here. Tom is there an official procedure to follow? If not, what's your take, acceptable or not?

Flags: needinfo?(ckerschb) → needinfo?(tom)

The temporary file now is saved under a under a dir without world/group-execute permissions, as what other saved files do (in D85075 and D85076).

I wonder if saving a temporary file is a big concern

I think that's okay. My inclination about threat model is that exposing this image to a local attacker (including a compromised content process from another origin) is not a strong concern. We shouldn't do it if it's easily avoidable (and it appears we are easily avoiding doing so) but we don't need to scorch the earth ensuring we're not. We should; however, be really certain that the HTTP/HTTPS request we are making for this image is not sending any credentials (cookies, Basic Auth) nor can the response be interpreted incorrectly[0]. I'm punting that part of the assurance back to you Christoph =)

:chunmin - thanks for putting up with us through all of this. Our response is a little disorganized, in the future this type of thing would probably be best handled by emailing secreview@ in the beginning.

[0] I'm imagining something like "The image request redirects to an HTML page which is then loaded in the parent process" or "The image request redirects to facebook, and we write the HTML contents of your facebook page to the temporary file".

Flags: needinfo?(tom)

(In reply to Tom Ritter [:tjr] (OOTO until Aug 3) from comment #33)

We should; however, be really certain that the HTTP/HTTPS request we are making for this image is not sending any credentials (cookies, Basic Auth) nor can the response be interpreted incorrectly[0]. I'm punting that part of the assurance back to you Christoph =)

The image will only be downloaded from a HTTP/HTTPS URL (D83694) with LOAD_ANONYMOUS flag (D83695), so the cookies and HTTP Auth credentials would be stripped.

:chunmin - thanks for putting up with us through all of this. Our response is a little disorganized, in the future this type of thing would probably be best handled by emailing secreview@ in the beginning.

Thanks for the info! I'll definitely start with an email next time.

(In reply to Tom Ritter [:tjr] (OOTO until Aug 3) from comment #33)

I'm punting that part of the assurance back to you Christoph =)

Christoph, are you ok with the current approach? I'd like to land the patches if it's ok for you.

Flags: needinfo?(ckerschb)

(In reply to Tom Ritter [:tjr] (OOTO until Aug 3) from comment #33)

We should; however, be really certain that the HTTP/HTTPS request we are making for this image is not sending any credentials (cookies, Basic Auth) nor can the response be interpreted incorrectly[0]. I'm punting that part of the assurance back to you Christoph =)

Yeah, we took good care of not sending any credentials. I think we are good here - I am fine with the approach!

Flags: needinfo?(ckerschb)
Pushed by cchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cda926214110
P1: Set track image to MPRIS r=alwu
https://hg.mozilla.org/integration/autoland/rev/a28932a71510
P2: Move same utils in SMTC and MPRIS backend to MediaControlUtils.h r=alwu
https://hg.mozilla.org/integration/autoland/rev/af0e28f70bde
P3: Only fetch image from http(s) URL r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/a40700a5fec1
P4: Load URL without user-specific data r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/467aa64be962
P5: Set the permission of the temporary file to 600 r=gcp
https://hg.mozilla.org/integration/autoland/rev/11689ad9792d
P6: Save images in a dictionary r=gcp
Flags: needinfo?(senglehardt)
Flags: needinfo?(astevenson)
You need to log in before you can comment on or make changes to this bug.