Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-images] image-orientation:none violates same-origin policy #5165

Open
annevk opened this issue Jun 4, 2020 · 56 comments
Open

[css-images] image-orientation:none violates same-origin policy #5165

annevk opened this issue Jun 4, 2020 · 56 comments

Comments

@annevk
Copy link
Member

annevk commented Jun 4, 2020

As I realized in whatwg/html#5603, this leaks an additional bit of information for opaque responses.

cc @heycam

@annevk
Copy link
Member Author

annevk commented Jun 4, 2020

An alternative to consider is to act as if images generated from opaque responses never have EXIF data (or any kind of metadata other than width/height/ratio). Requiring each feature that can disable or override some EXIF data to take such images into account feels very brittle to me.

@noamr
Copy link
Contributor

noamr commented Jun 4, 2020

An alternative to consider is to act as if images generated from opaque responses never have EXIF data. Requiring each feature that can disable EXIF data to take such images into account feels very brittle to me.

I think that that would create a requirement that would make people enable CORS when they didn't otherwise have to - just to have their images display correctly.

I think there's no reason why this requirement should be on the usage of EXIF, instead on the feature that overrides (and thus exposes) EXIF, such as image-orientation and image-resolution CSS properties.

Otherwise, it feels like we're trying to prevent a threat of a hypothetical future API. Is that a necessary thing to do?

@annevk
Copy link
Member Author

annevk commented Jun 4, 2020

They're not hypothetical, this API (image-orientation) already exists, whatwg/html#5603 adds another. Making the APIs enforce the policy violates the principle of least privilege and will likely lead to numerous bugs.

@heycam
Copy link
Contributor

heycam commented Jun 5, 2020

cc @stephenmcgruer

@heycam
Copy link
Contributor

heycam commented Jun 5, 2020

If we do have to prevent image-orientation from working on images that came from opaque responses, it would be nice if we could unconditionally apply the orientation (and I guess pretend from the whatwg/html#5603 APIs that there was no orientation metadata), so that we can try to treat orientation as an implementation detail of the image file representation. But that would make it tricky for authors wanting to use image-orientation: none to turn off the new re-orientation effects for their pages that rely on it not being applied.

@noamr
Copy link
Contributor

noamr commented Jun 5, 2020

If we do have to prevent image-orientation from working on images that came from opaque responses, it would be nice if we could unconditionally apply the orientation (and I guess pretend from the whatwg/html#5603 APIs that there was no orientation metadata), so that we can try to treat orientation as an implementation detail of the image file representation. But that would make it tricky for authors wanting to use image-orientation: none to turn off the new re-orientation effects for their pages that rely on it not being applied.

I think that's a better approach... the threat comes from the "overriding" feature, not from the implementation detail of using EXIF. An image format may similarly have an internal representation of orientation/resolution supported internally in the decoder - would that also be limited to same-origin/CORS images?

EXIF is not the issue here - it's the mixing of image-originated data and markup-originated data, which is something that currently occurs only for naturalWidth/naturalHeight.
If we want to take a more generic approach - I think it should tackle those blurred boundaries between content and markup.

@annevk
Copy link
Member Author

annevk commented Jun 5, 2020

@noamr again, it's not just overriding, it's also reading as linked above. There's various different ways this will end up being exposed.

@heycam how do we model it in such a way that we don't need security checks all over?

I guess what we could do is that we take the orientation into account for decoding purposes, but don't store it as a field on the resulting image if it was generated from an opaque response. So it appears rotated, but if you query its metadata it'll return the default orientation values.

The tricky aspect is when metadata can be overridden, as it can be here. If the internal representation still has non-default metadata you would need to take that into account somehow. I.e., if an image was rotated 90 degrees and an API asked for it not to be rotated, it would have to remain rotated at 90 degrees. Model-wise that follows from the preceding paragraph, but in implementations that might be a bit trickier.

@noamr
Copy link
Contributor

noamr commented Jun 5, 2020

@noamr again, it's not just overriding, it's also reading as linked above. There's various different
ways this will end up being exposed.

Sure, I meant reading/overriding - anything that enables reading directly or indirectly.

The tricky aspect is when metadata can be overridden, as it can be here. If the internal representation still has non-default metadata you would need to take that into account somehow. I.e., if an image was rotated 90 degrees and an API asked for it not to be rotated, it would have to remain rotated at 90 degrees. Model-wise that follows from the preceding paragraph, but in implementations that might be a bit trickier.

I think that the overriding features in this case should be disabled for the opaque resource. E.g. CSS image-orientation would simply not apply, maybe even regarded as invalid style. I think that would be reasonable implementation-wise.

@annevk
Copy link
Member Author

annevk commented Jun 5, 2020

I think the model we end up with shouldn't require each new feature to check whether the image was generated from an opaque response. So if some theoretical feature allowed setting EXIF rotation to 90 and the opaque image already had it 90 (exposed as 0 per the above model), it should be as if it was 180 and be exposed to the world as 90.

@noamr
Copy link
Contributor

noamr commented Jun 5, 2020

I think the model we end up with shouldn't require each new feature to check whether the image was generated from an opaque response. So if some theoretical feature allowed setting EXIF rotation to 90 and the opaque image already had it 90 (exposed as 0 per the above model), it should be as if it was 180 and be exposed to the world as 90.

I like that. Makes metadata "embedded" into the image for opaque images, but still working as expected if there's nothing that tries to override/read it.

@heycam
Copy link
Contributor

heycam commented Jun 6, 2020

I guess what we could do is that we take the orientation into account for decoding purposes, but don't store it as a field on the resulting image if it was generated from an opaque response. So it appears rotated, but if you query its metadata it'll return the default orientation values.

I think this would be the right approach. The intrinsic orientation spec concept for opaque images would be "zero degrees, no flip", and the image dimensions (whether the image is opaque or not) would have the orientation taken into account. image-orientation's behavior would then be written in terms of the image's intrinsic orientation.

@noamr
Copy link
Contributor

noamr commented Jun 15, 2020

cc @chrishtr @smfr

@noamr
Copy link
Contributor

noamr commented Jun 21, 2020

I don't see a lot of movement on this ticket... does any implementer have an opinion about this?
It's currently blocking whatwg/html#5574, and these same-origin policy violations are already in the wild... would be good to figure out if we see EXIF orientation/(resolution) data as a cross-origin information leak, and if it is, how to mitigate it.

@chrishtr
Copy link
Contributor

chrishtr commented Jul 1, 2020

I didn't see it stated very clearly clearly in this issue, so let me first state what I think the information leak is:

Developers can detect whether there is EXIF rotation information in an image by rendering it twice - once with image-orientation: from-image and one with image-orientation: none, and observing if there is a difference in the layout size of the result.

Therefore, for a cross-domain image, the developer can obtain one bit of information about these images.

However, don't sites already know multiple "bits of information" about cross-origin images, such as their width and height?

@noamr
Copy link
Contributor

noamr commented Jul 1, 2020

I didn't see it stated very clearly clearly in this issue, so let me first state what I think the information leak is:

Developers can detect whether there is EXIF rotation information in an image by rendering it twice - once with image-orientation: from-image and one with image-orientation: none, and observing if there is a difference in the layout size of the result.

Therefore, for a cross-domain image, the developer can obtain one bit of information about these images.

Yes, and same for a potential implementation of image-resolution, and for querying image orientation from javascript (whatwg/html#5602).

However, don't sites already know multiple "bits of information" about cross-origin images, such as their width and height?

I think the only bits of information they know right now is an image's width and height. Is exposing related information such as orientation/density a problem? It's hard for me to fathom how that info can be used, but it's difficult to be certain.

@annevk
Copy link
Member Author

annevk commented Jul 2, 2020

I think it is a problem. There's a long history of all these communication channels leading to privacy and security issues. We should hold the line where we can and clearly we can here.

@yoavweiss
Copy link

One concrete scenario that can be problematic:
PhotoSharing.example allows non-CORS cross-origin fetching of credentialed images, but only for logged-in users or users that belong to a certain group (which the image was shared with).
PhotoSharing.example already knows about the width and height leak, as well as timing attacks that may result from it not serving the image in the disallowed cases. As a result, it creates an empty image with the same dimensions and makes sure that the response timing looks similar to the real deal (without setting Timing-Allow-Origin on neither image).

But, if the original image contains orientation or resolution information, adding those capabilities would surprise PhotoSharing folks and cause them to potentially expose log-in state or group affiliation across origins.

It seems like this is a problem that will go away when browsers limit cross-origin credentials, but we're not there yet.

Would it make sense to only respect orientation/resolution for CORP enabled images? CORP seems like a clear signal saying the image can be embedded. I wonder what would be the impact of that on deployability.

/cc @eeeps @mikewest @arturjanc

@annevk
Copy link
Member Author

annevk commented Jul 6, 2020

I don't want to drag a dependency on CORP into this. That would change its semantics from a boolean check in fetch to a property of the response. All CORP: cross-origin means is that you're okay being side-channel attacked. This is not a side channel.

@noamr
Copy link
Contributor

noamr commented Jul 6, 2020

I found a scenario in the related issue whatwg/html#5574 where some indirect means can be used to figure out the image's resolution. See this comment. I am convinced that this needs to be addressed.

Recapping the two current proposals (following IRC discussion with @annevk):

  1. Ignore metadata for opaque-response images
  2. Bake the metadata in for opaque-response images (e.g. rotate and scale the image but ignore that notion when applying CSS rotation/srcset scaling).

In either case, a cross-origin image might appear different depending on which origin is embedding it. In (1), it will appear different by default. In (2), it will appear different only in certain cases. e.g. when CSS image-rotation, image-resolution or srcset is being used, or in future scenarios that we are not yet aware of.

Also both (1) and (2) would require changes in current implementations, as image-orientation: none is already shipped.

I believe that (1) is easier to implement and grasp, however, it would have a higher chance of breaking some current sites using EXIF-rotated images (if the images are cross-origin and don't have the CORS headers).

@yoavweiss
Copy link

IMO, a CORS restriction would be too restrictive, and will pose a significant deployment hurdle.
@annevk - can you expand on why this wouldn't work well with CORP? It seems to be leaking the exact same information that CORP "allows" you to leak.

/cc @camillelamy

@annevk
Copy link
Member Author

annevk commented Jul 6, 2020

CORP is about allowing a Spectre-read gadget to potentially get at your data. It's not an assertion that it's fine to make that data public and it's not guaranteed that Spectre-read gadgets will be able to get at it forever. Otherwise we might as well have required CORS there as the initial design did. Also, all data, not just the metadata.

@noamr
Copy link
Contributor

noamr commented Jul 15, 2020

I found a scenario in the related issue whatwg/html#5574 where some indirect means can be used to figure out the image's resolution. See this comment. I am convinced that this needs to be addressed.

Recapping the two current proposals (following IRC discussion with @annevk):

  1. Ignore metadata for opaque-response images
  2. Bake the metadata in for opaque-response images (e.g. rotate and scale the image but ignore that notion when applying CSS rotation/srcset scaling).

In either case, a cross-origin image might appear different depending on which origin is embedding it. In (1), it will appear different by default. In (2), it will appear different only in certain cases. e.g. when CSS image-rotation, image-resolution or srcset is being used, or in future scenarios that we are not yet aware of.

Also both (1) and (2) would require changes in current implementations, as image-orientation: none is already shipped.

I believe that (1) is easier to implement and grasp, however, it would have a higher chance of breaking some current sites using EXIF-rotated images (if the images are cross-origin and don't have the CORS headers).

Blocking metadata with CORS completely could cause an issue I haven't thought of earlier - it means that CSS-loaded images can't use orientation/resolution, as those don't expose a crossorigin attribute (which is currently only meaningful for canvas drawing). OTOH CSS-loaded images don't leak any of the metadata information as the image's size is not readable and doesn't affect layout.

In addition, it would require regular images to start including crossorigin when using a CDN, just to have their image displayed correctly. That doesn't seem reasonable.

As today so many images are CDN-delivered and don't bother with a crossorigin attribute (or can't because the image is CSS-loaded), I think it's a blocker for using (1) - it would make image orientation and resolution less than usable.

CORP seems less suitable as it's meant to block embedding at all, not just reading of metadata.

I believe that this should be blocked with an additional HTTP header (yikes), similar to Timing-Allow-Origin, or not at all - servers who want to offer this kind of protection to their images can bake the metadata into that image and not expose it.

@noamr
Copy link
Contributor

noamr commented Jul 15, 2020

TL;DR: proposing an HTTP header (maybe Media-Transform-Allow-Origin), similar to Timing-Allow-Origin. If that header is not present, image orientation/resolution from EXIF should be ignored.

@camillelamy
Copy link
Member

I am wondering if there would be value in having a combined header like Metadata-Allow-Origin where we can specify which kind of metadata are allowed for which origins (eg, timing, image orientation/resolution). This way, when a similar issue comes up next we can extend this header instead of defining a new one.

@noamr
Copy link
Contributor

noamr commented Jul 17, 2020

I am wondering if there would be value in having a combined header like Metadata-Allow-Origin where we can specify which kind of metadata are allowed for which origins (eg, timing, image orientation/resolution). This way, when a similar issue comes up next we can extend this header instead of defining a new one.

Sounds like an interesting alternative, kind of like Access-Control-Allow-Headers.
Maybe something like this would have sufficient granularity:
Metadata-Allow-Origin: *; Metadata-Allow-Properties: Orientation,Resolution,Timing

@tabatkins
Copy link
Member

OTOH CSS-loaded images don't leak any of the metadata information as the image's size is not readable and doesn't affect layout.

They do, fwiw - ::before { content: url(...); } creates an anonymous replaced box containing the specified image, which will affect layout (or makes the pseudo-element itself into a replaced element containing the image, to the same effect).

In either case, a cross-origin image might appear different depending on which origin is embedding it. In (1), it will appear different by default. In (2), it will appear different only in certain cases. e.g. when CSS image-rotation, image-resolution or srcset is being used, or in future scenarios that we are not yet aware of.

Just because it'll still allow images to look correct by default, I lean strongly toward (2). Each potentially-exposed bit of metadata just needs to define a "default" value that it'll masquerade as for the purpose of in-page manipulations. This is trivial for orientation, but I guess resolution will have to pretend to be 1x? That'll break srcset (it'll density-correct images twice), but that might be unavoidable here.

@philcunliffe
Copy link

From what I hear, as long as image orientation is respected, and image-orientation css is used for backwards compatibility, IMO there is no solution but to close this issue and let that leakage be. (intrinsic image resolution is a different story, as it doesn't have significant backwards compatibility implications).

This would work for us, but in an ideal world we would like to respect EXIF orientation as well. In our use-case respecting EXIF orientation requires knowing the orientation value itself. This was the original impetus behind this pull-request from @heycam whatwg/html#5603 which would put orientation into parity with width and height.

@noamr
Copy link
Contributor

noamr commented Sep 3, 2020

Not sure where this is standing now...
@tabatkins @cbiesinger, seems like this was resolved in the CSS work group. Do you care yo address the web compat concerns from @philcunliffe? if not, what is the next step - does this need to go into the image-orientation CSS spec? Happy to help with a PR if that's what's needed.

@noamr
Copy link
Contributor

noamr commented Oct 4, 2020

@noamr
Copy link
Contributor

noamr commented Oct 4, 2020

noamr added a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2020
See w3c/csswg-drafts#5165

image-orientation: none should effectively be ignored for cross-origin images.
@tabatkins
Copy link
Member

Note to self: blocked on edits for the moment because Fetch doesn't seem to have any terms I can use to refer to an image that's "CORS-clean" or not.

@noamr
Copy link
Contributor

noamr commented Oct 6, 2020

Note to self: blocked on edits for the moment because Fetch doesn't seem to have any terms I can use to refer to an image that's "CORS-clean" or not.

I believe that image data can be CORS-cross-origin:
From the spec:
"Otherwise, response's unsafe response is image
request's image data. It can be either
CORS-same-origin or CORS-cross-origin"

@philcunliffe
Copy link

@noamr if I understand these bug reports there doesn't seem to be any pathway to backwards compatibility in terms of prevention of rotation and no way for the client to read the orientation value. Is that correct?

@noamr
Copy link
Contributor

noamr commented Oct 7, 2020

@noamr if I understand these bug reports there doesn't seem to be any pathway to backwards compatibility in terms of prevention of rotation and no way for the client to read the orientation value. Is that correct?

For cross-origin images that is correct. The bug reports and tests follow what the CSS-wg has decided. If the decision changes the tests and implementation would be different of course.

@noamr
Copy link
Contributor

noamr commented Oct 9, 2020

Landed in webkit: https://trac.webkit.org/changeset/268249/webkit

@philcunliffe
Copy link

philcunliffe commented Oct 9, 2020

Landed in webkit: https://trac.webkit.org/changeset/268249/webkit

When this does make it into the large browsers I would suggest making this a very loud change in patch notes, it's again changing existing behavior that many applications depend on.

noamr added a commit to web-platform-tests/wpt that referenced this issue Oct 10, 2020
See w3c/csswg-drafts#5165

image-orientation: none should effectively be ignored for cross-origin images.
@philcunliffe
Copy link

philcunliffe commented Oct 14, 2020

While I'm glad a decision was made so the uncertainty is gone, I really disagree with the direction.

I agree that from a security standpoint it's unfortunate that cross-origin images are allowed at all but given that they are it's important that they can be used properly. This middle ground approach where some metadata (width and height) is available and others like orientation are not only serves to subtlety break people in difficult to understand ways because the rules are not consistent. Holding the line on orientation alone doesn't isn't win for either paradigm, all it does is make cross-origin images more difficult to use.

@annevk
Copy link
Member Author

annevk commented Oct 15, 2020

They already are more difficult to use, if you don't use CORS to fetch them you cannot paint them on canvas and then read from that canvas, for instance. If you want to make full use of a cross-origin image, use CORS.

@noamr
Copy link
Contributor

noamr commented Oct 15, 2020

This middle ground approach where some metadata (width and height) is available and others like orientation are not...

It's not a middle ground. It's as strict as we can make it, considering the unfortunate situation where exposing width/height for cross-origin images is something that is probably too late to backtrack.

@annevk
Copy link
Member Author

annevk commented Nov 6, 2020

https://bugzilla.mozilla.org/show_bug.cgi?id=1655598 tracks this for Firefox.

@JiaboHu
Copy link

JiaboHu commented Apr 1, 2021

I guess what we could do is that we take the orientation into account for decoding purposes, but don't store it as a field on the resulting image if it was generated from an opaque response. So it appears rotated, but if you query its metadata it'll return the default orientation values.

Such a big change deserves a big announcement, at least an online conference on youtube to give developers enough time to understand and update their apps.

@mzur
Copy link

mzur commented May 28, 2021

I am the maintainer of an image annotation web application and in a similar position than @philcunliffe (only that I wasn't aware of this before it was implemented). Now I'm caught cold with the implementation in Chromium which totally breaks our application without any way to implement backwards compatibility. We already went to great lengths to ensure backwards compatibility with the previous breaking change of always respecting the EXIF orientation (I also commented on that here but nobody seems to be interested there anymore).

I realize that it's probably too late and the discussion is already over but I wanted to leave a note that the previous decision to apply EXIF rotation by default and now this decision are huge breaking changes for some of us. Sorry for the rant, I'm just a little frustrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests