LGTM3 On Wed, Sep 6, 2023 at 8:47 AM Yoav Weiss <[email protected]> wrote:
> 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 > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfU55BdzLaOxRho0DiWRF5pSTGLXQfj8Kig437exZO-o5A%40mail.gmail.com?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/CAOMQ%2Bw9GwFyhb_RW6cnDx%3DpbLoQ1c4hccj0xyiUjeqDmuQnQSA%40mail.gmail.com.
