LGTM2 On Fri, Sep 1, 2023 at 11:54 PM Mike Taylor <[email protected]> wrote:
> I also had an offline discussion with Daniel to confirm shipping as-is in > M117, and sanitizing in 118 is acceptable from a security POV (trust but > verify, etc). > > LGTM1 to ship. > On 8/31/23 7:55 AM, 'Garrett Tanzer' via blink-dev wrote: > > After some discussion offline, we're going to sanitize the macro keys and > values with EscapeQueryParamValue > <https://source.chromium.org/chromium/chromium/src/+/main:base/strings/escape.h;l=29?q=EscapeQueryParamValue%20-f:out> > so > that macro substitution always stays within the original query parameter > field. This should prevent XSS-y substitutions while keeping the API > surface exactly the same for regular use. > > This fix will be implemented in M118, and we've decided that it need not > block launch in M117 due to the low severity (especially because the > existing reportEvent surface grants the buyer reporting worklet complete > control over the destination URL, so the ad creative is already known to > trust the buyer wrt where its reports get sent). > > On Monday, August 28, 2023 at 11:55:39 AM UTC-4 Garrett Tanzer wrote: > >> Hi Daniel, >> >> - There are a few relevant call sites in the overall reporting flow: >> - Declare allowlist of reporting destination origins >> - This happens in navigator.joinAdInterestGroup(), by an ad >> auction buyer >> - Declare macros (key:value correspondences) >> - This happens in the buyer reporting worklet, by the same ad >> auction buyer that declared the reporting destination origins >> - Perform report to custom url >> - This happens under the auction's winning ad creative's >> origin, which isn't necessarily the same as the ad auction buyer, >> but it is >> chosen by the ad auction buyer >> - Here is the sequence of events: >> >> >> 1. >> 1. The browser ingests and validates the allowlist when the >> interest group is declared. >> 2. The browser ingests the macro key:value mapping when the >> auction happens. The key/value strings have no structure that is >> validated. The browser adds "${" and "}" around the user-provided keys >> in >> the macro mapping. >> 3. In the ad that results from the auction: >> 1. The ad sends a URL to the browser, including macros like >> ${KEY}. The URL has to be a valid HTTPS url even with the macros >> unsubstituted. (See impl: >> >> https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=8644?q=content%2Fbrowser%2Frenderer_host%2Frender_frame_host_impl.cc%20-f:out >> ) >> 2. The browser performs a simple string substitution to replace >> the keys with the values. The implementation is reused from >> navigator.deprecatedReplaceInURN, another part of Protected >> Audience. It >> doesn't substitute macros recursively, so you can't get an infinite >> loop. >> deprecatedReplaceInURN has apparently not been spec'd, which is was >> unable >> to quickly reuse that spec and left it as a TODO for now. >> 3. The origin of the resulting URL is checked against the >> allowlist. If it doesn't match any of them, no action is performed. >> If the >> URL is invalid, this will create an opaque origin and therefore >> always fail >> the check. (See impl: >> >> https://source.chromium.org/chromium/chromium/src/+/main:content/browser/fenced_frame/fenced_frame_reporter.cc;l=454?q=fenced_frame_reporter.cc%20-f:out) >> We may add an explicit URL valid/HTTPS check after substitution for >> neatness/to be robust to any future changes to the allowlist >> checking. >> 4. If the URL does pass the check, a GET fetch will be >> performed on it (the result is unused). There's no other >> parsing/usage of >> the URL other than performing a fetch. >> >> So in summary, we were not very concerned about XSS because the entity >> choosing the macro pairings is the one we want to protect, and the entity >> providing the base URLs is semi-trusted. The less trusted entity is the >> site of the destination URL, which just receives a GET and doesn't get to >> control anything. Since the URL has to be valid both before and after the >> macro substitution, this limits how wacky the substitutions can get even if >> you don't trust one of those entities. The weirdest stuff you could do >> would be like "FOO}&key=value1&${BAR" -> "key=value2", so that it becomes >> "${FOO}&key=value1&${BAR}" -> "key=value2". We can add a check to exclude >> $,{,} characters in the macro key so even this isn't possible. >> >> Hope this answers your questions, >> Garrett >> >> On Monday, August 28, 2023 at 9:35:34 AM UTC-4 Daniel Vogelheim wrote: >> >>> Hi Liam, >>> >>> This intent has come up in the OWP security triage, and I'm trying to >>> figure out whether there's XSS potential in the 3rd sub-feature, "Creative >>> macros in FFAR". This looks like a string-based pattern replacement where >>> the result string will then be parsed by the browser. Similar things have >>> lent themselves to XSS, e.g. when a string value contains meta characters >>> that code downstream will then parse in unexpected ways. Unfortunately, I >>> can't seem to find enough information about how exactly the replacement & >>> subsequent usage works to make up my mind whether there's a concern or not. >>> Could you help me out with a bit more information? >>> >>> What I've found is this: >>> - In "<dfn>send a beacon</dfn>": "TODO: Substitute macros from |macro >>> map| into |destination url|." (source >>> <https://github.com/WICG/fenced-frame/pull/113/files>) >>> - In "<dfn>asynchronously finish reporting</dfn>" I find where macro map >>> being set, but then it says: "TODO: Pass |macroMap| and >>> |allowedReportingOrigins| to [=Finalize a reporting destination=] when it >>> is updated to take the parameters." (source >>> <https://github.com/WICG/turtledove/pull/762/files>) I can't find that >>> usage. >>> >>> Questions I have: >>> - Am I reading the right docs? Where else should I look? >>> - Is this meant as a simple string-based substitution? Is there any >>> filtering of allowed characters, or so? >>> - What happens with the result values? >>> - Is the entity that sets the macro values always the same that has set >>> the patterns the values are being used for, or could those be different >>> entities? >>> >>> Thanks! >>> >>> >>> On Fri, Aug 25, 2023 at 9:34 PM 'Liam Brady' via blink-dev < >>> [email protected]> wrote: >>> >>>> Contact emails >>>> >>>> [email protected], [email protected], [email protected], >>>> [email protected] >>>> Explainer(s) >>>> >>>> Send Automatic Beacons Once >>>> >>>> https://github.com/WICG/turtledove/pull/718 >>>> >>>> Serializable Fenced Frames Configs - Minor Change, No explainer >>>> available. >>>> >>>> Note: With this change, FencedFrameConfig objects will be serializable >>>> and can be sent through "postMessage()" and other similar calls. >>>> >>>> Creative Macros in Fenced Frames Ads Reporting (FFAR) >>>> >>>> https://github.com/WICG/turtledove/pull/763 >>>> >>>> Spec(s) >>>> >>>> Send Automatic Beacons Once >>>> >>>> https://github.com/WICG/fenced-frame/pull/109 >>>> >>>> Serializable Fenced Frames Configs >>>> >>>> https://github.com/WICG/fenced-frame/pull/111 >>>> >>>> Creative Macros in Fenced Frames Ads Reporting (FFAR) >>>> >>>> Protected Audience: https://github.com/WICG/turtledove/pull/762/files >>>> >>>> Fenced Frames: https://github.com/WICG/fenced-frame/pull/113 >>>> >>>> >>>> >>>> Summary >>>> >>>> We launched Fenced Frames as a part of Chrome 115. We would like to add >>>> the following three functionalities to Fenced Frames. >>>> >>>> 1. Send Automatic Beacons Once >>>> >>>> A common feature in ad frames is the "why this ad?" link. Since that >>>> link is separate from the ad itself, clicking "why this ad?", and its >>>> subsequent navigation, should be considered different from clicking >>>> directly on the advertisement itself. With our current automatic beacon >>>> design, however, once an automatic beacon is set (usually in the click >>>> handler for the ad link), that beacon will send out for any subsequent >>>> top-level navigations, including if "why this ad?" is clicked. This can >>>> result in erroneous impressions being sent out. >>>> >>>> As a solution, this feature introduces a new "once" member to the >>>> FenceEvent <https://wicg.github.io/fenced-frame/#dictdef-fenceevent> >>>> dictionary passed into >>>> "window.fence.setReportEventDataForAutomaticBeacons()". If set to true, the >>>> saved automatic beacon data will be cleared out after the next beacon is >>>> sent, ensuring that automatic beacons with that data are only sent once. >>>> This means that further clicks to non-ad parts of the frame that result in >>>> top-level navigations will not send out erroneous beacons. >>>> >>>> (This feature already shipped in M116. That was our mistake. At the >>>> time, we thought we would only need a PSA for it, and shipped it without it >>>> being behind a flag. We apologize for the mistake.) >>>> >>>> 2. Serializable FencedFramesConfigs >>>> >>>> With this change, FencedFrameConfig objects will be serializable and >>>> can be sent through "postMessage()" and other similar calls. Serialization >>>> allows for a case where the frame that runs an ad auction is not the same >>>> frame that ends up embedding the winning ad in a fenced frame. >>>> FencedFrameConfigs cannot be serialized to storage, nor can they be sent in >>>> a message that crosses a fenced frame boundary. A FencedFrameConfig object >>>> is only valid in the traversable navigable >>>> <https://wicg.github.io/fenced-frame/#traversable-navigables> it was >>>> originally created in, and, if sent outside to a different context, will >>>> not be able to navigate, since the new traversable navigable >>>> <https://wicg.github.io/fenced-frame/#traversable-navigables>'s fenced >>>> frame config mapping >>>> <https://wicg.github.io/fenced-frame/#traversable-navigable-fenced-frame-config-mapping> >>>> will not contain the internal config needed to do the navigation. >>>> >>>> 3. Creative macros in Fenced Frames Ads Reporting (FFAR) >>>> >>>> This feature extends the Fenced Frame Ads Reporting (FFAR) API to >>>> support macro substitution in reporting URLs and allows reports to be sent >>>> to up to ten other origins that have enrolled with the Privacy Sandbox and >>>> allow-listed by the DSP. Use case: In online ad auctions for ad space, >>>> advertisers buying through DSPs in several situations use other adtech >>>> providers to monitor performance and keep track of how their advertising >>>> dollars are spent. (issue link >>>> <https://github.com/WICG/turtledove/issues/477>) >>>> >>>> Blink component >>>> >>>> Blink>FencedFrames >>>> <https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink%3EFencedFrames> >>>> >>>> TAG reviews and status >>>> >>>> Fenced frames existing TAG review appended with these spec changes >>>> https://github.com/w3ctag/design-reviews/issues/838#issuecomment-1693631006 >>>> >>>> Link to Origin Trial feedback summary >>>> >>>> No Origin Trial performed >>>> >>>> Is this feature supported on all six Blink platforms (Windows, Mac, >>>> Linux, Chrome OS, Android, and Android WebView)? >>>> >>>> Supported on all the above platforms except Android WebView. >>>> >>>> Debuggability >>>> >>>> Additional debugging capabilities are not necessary for these feature >>>> changes. >>>> >>>> Risks >>>> >>>> Compatibility >>>> >>>> There are no compatibility risks, as described below: >>>> >>>> 1. Send Automatic Beacons Once: This is backward compatible with the >>>> existing API since the default value of “once” is false which is the same >>>> behavior as the previous behavior. >>>> >>>> 2. Serializable FencedFramesConfigs: This is added functionality and >>>> backward compatible with the existing FencedFramesConfig. >>>> >>>> 3. Creative macros in Fenced Frames Ads Reporting (FFAR): This is >>>> adding a new API and a backward compatible change to reportEvent. >>>> >>>> Interoperability >>>> >>>> there are no interoperability risks as no other browsers have decided >>>> to implement these features yet. >>>> >>>> Is this feature fully tested by web-platform-tests >>>> <https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md>? >>>> Link to test suite results from wpt.fyi. >>>> >>>> Yes >>>> >>>> Tests: >>>> https://github.com/web-platform-tests/wpt/tree/master/fenced-frame >>>> >>>> Results: >>>> https://wpt.fyi/results/fenced-frame?label=experimental&label=master&aligned >>>> >>>> Specifically, these features correspond to the following tests: >>>> >>>> Send Automatic Beacons Once: >>>> >>>> - >>>> >>>> automatic-beacon-two-events-clear.https.html (test >>>> >>>> <https://github.com/web-platform-tests/wpt/blob/master/fenced-frame/automatic-beacon-two-events-clear.https.html>) >>>> (result >>>> >>>> <https://wpt.fyi/results/fenced-frame/automatic-beacon-two-events-clear.https.html?label=experimental&label=master&aligned> >>>> ) >>>> - >>>> >>>> automatic-beacon-two-events-persist.https.html (test >>>> >>>> <https://github.com/web-platform-tests/wpt/blob/master/fenced-frame/automatic-beacon-two-events-persist.https.html>) >>>> (result >>>> >>>> <https://wpt.fyi/results/fenced-frame/automatic-beacon-two-events-persist.https.html?label=experimental&label=master&aligned> >>>> ) >>>> >>>> Serializable FencedFrameConfigs: >>>> >>>> - >>>> >>>> deep-copy-config.https.html (test >>>> >>>> <https://github.com/web-platform-tests/wpt/blob/master/fenced-frame/deep-copy-config.https.html>) >>>> (result >>>> >>>> <https://wpt.fyi/results/fenced-frame/deep-copy-config.https.html?label=experimental&label=master&aligned> >>>> ) >>>> >>>> Creative macros in Fenced Frames Ads Reporting (FFAR): >>>> >>>> - >>>> >>>> fence-report-event-destination-url.https.html (test >>>> >>>> <https://github.com/web-platform-tests/wpt/blob/master/fenced-frame/fence-report-event-destination-url.https.html>) >>>> (result >>>> >>>> <https://wpt.fyi/results/fenced-frame/fence-report-event-destination-url.https.html?label=experimental&label=master&aligned> >>>> ) >>>> >>>> >>>> Anticipated spec changes >>>> >>>> None >>>> >>>> Link to entry on the Chrome Platform Status >>>> >>>> https://chromestatus.com/feature/5103970808233984 >>>> >>>> Links to previous Intent discussions >>>> >>>> Intent to prototype: >>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/Ko9UXQYPgUE/m/URRsB-qvAAAJ >>>> >>>> >>>> Intent to experiment: >>>> >>>> >>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/y6G3cvKXjlg/m/Lcpmpi_LAgAJ >>>> >>>> >>>> Intent to extend origin trial: >>>> >>>> >>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/SD8Ot2gpz4g/m/A9uA-_cGAwAJ >>>> >>>> >>>> >>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/gpmaOi3of_w/m/SyMclFhMAAAJ >>>> >>>> >>>> >>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/CBrV-2DrYFI/m/RTojC6kHAgAJ >>>> >>>> >>>> Intent to ship: >>>> >>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/tpw8wW0VenQ/m/mePLTiHlDQAJ >>>> >>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "blink-dev" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to [email protected]. >>>> To view this discussion on the web visit >>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/62771371-5dbb-4d02-a232-a99ded5b293fn%40chromium.org >>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/62771371-5dbb-4d02-a232-a99ded5b293fn%40chromium.org?utm_medium=email&utm_source=footer> >>>> . >>>> >>> -- > You received this message because you are subscribed to the Google Groups > "blink-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/blink-dev/2b14ffdf-1fdb-4fa7-9054-65356c007b50n%40chromium.org > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/2b14ffdf-1fdb-4fa7-9054-65356c007b50n%40chromium.org?utm_medium=email&utm_source=footer> > . > > -- > You received this message because you are subscribed to the Google Groups > "blink-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/blink-dev/04a15088-83e8-4ffc-8a6f-8b255c74e585%40chromium.org > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/04a15088-83e8-4ffc-8a6f-8b255c74e585%40chromium.org?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfU55BdzLaOxRho0DiWRF5pSTGLXQfj8Kig437exZO-o5A%40mail.gmail.com.
