Move Dangling Markup in target name logic to HTML element [chromium/src : main]
Jun Kokatsu (Gerrit)
Jun Kokatsu uploaded patch set #2 to this change.
Move Dangling Markup in target name logic to HTML element
This change moves checks for Dangling Markup in target name to HTML
element, to avoid bugs such as noopener not being set on new window when
Dangling Markup is detected, and better align implementation with the
new spec[1].
[1] https://github.com/whatwg/html/pull/9309
Bug: 1451273
Change-Id: Ie98afc0c692338d295fa1ec1f407d7ba2f5817d1
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/loader/form_submission.cc
M third_party/blink/renderer/core/page/frame_tree.cc
M third_party/blink/renderer/core/svg/svg_a_element.cc
6 files changed, 37 insertions(+), 41 deletions(-)
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Jun Kokatsu uploaded patch set #3 to this change.
Move Dangling Markup in target name logic to HTML element
This change moves checks for Dangling Markup in target name to HTML
element, to avoid bugs such as noopener not being set on new window when
Dangling Markup is detected, and better align implementation with the
new spec[1].
[1] https://github.com/whatwg/html/pull/9309
Bug: 1451273, 1421440
Change-Id: Ie98afc0c692338d295fa1ec1f407d7ba2f5817d1
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/loader/form_submission.cc
M third_party/blink/renderer/core/page/frame_tree.cc
M third_party/blink/renderer/core/svg/svg_a_element.cc
6 files changed, 37 insertions(+), 41 deletions(-)
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Stefan Zager.
Jun Kokatsu would like Stefan Zager to review this change.
Move Dangling Markup in target name logic to HTML element
This change moves checks for Dangling Markup in target name to HTML
element, to avoid bugs such as noopener not being set on new window when
Dangling Markup is detected, and better align implementation with the
new spec[1].
[1] https://github.com/whatwg/html/pull/9309
Bug: 1451273, 1421440
Change-Id: Ie98afc0c692338d295fa1ec1f407d7ba2f5817d1
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/loader/form_submission.cc
M third_party/blink/renderer/core/page/frame_tree.cc
M third_party/blink/renderer/core/svg/svg_a_element.cc
6 files changed, 37 insertions(+), 41 deletions(-)
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Stefan Zager.
Patch set 3:Quick-Run +1Commit-Queue +1
1 comment:
Patchset:
szager@, could you PTAL? Thanks!
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Zager (Gerrit)
Attention is currently required from: Jun Kokatsu.
1 comment:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {Why not just add this logic to `HTMLAnchorElement::GetEffectiveTarget`? For example, here's a call site that isn't patched in this CL:
Is there any use case for returning the unclean target?
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Stefan Zager.
1 comment:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
Why not just add this logic to `HTMLAnchorElement::GetEffectiveTarget`? For example, here's a call s […]
We are only looking for sanitizing `target` which will be used in `FrameTree::FindOrCreateFrameForNavigation`[1] which comes from HTML (and not from JS like `window.open`).
This is because we are trying to mitigate an attack which uses HTML injection, which doesn't have script execution (due to CSP for example).
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
We are only looking for sanitizing `target` which will be used in `FrameTree::FindOrCreateFrameForNa […]
Additionallt, calls from `<form>` and `<svg:a>` don't necessarily have `HTMLAnchorElement` reference.
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
Attention is currently required from: Jun Kokatsu, Stefan Zager.
2 comments:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
Additionallt, calls from `<form>` and `<svg:a>` don't necessarily have `HTMLAnchorElement` reference […]
I think the _problem_ is that the function that szager pointed to is the one that _actually_ implements the spec algorithm that you referenced as documentation for this function (which is essentially just a post-processing step applied after that). So `CleanLinkTarget` or so seems like a better name with the current implementation (and matching documentation to boot). Could also be a free function somewhere since it doesn't have much of a Element tie.
Canonicalization of the target does seem like would equally well align with the construction of the actual navigation (from an _HTML_ spec perspective I can see that it probably won't matter much how it's aligned).
Given the apparent history here it seems like consolidation of the creation of the navigation data would've been a good direction from the start - this seems to things less maintainable and thus more error-prone (and because of that perhaps more susceptible to similar security loopholes).
(It looks like FormSubmission::Create has a bit of an identity crisis, and may even end up losing the 'noopener' statein the process?)
File third_party/blink/renderer/core/svg/svg_a_element.cc:
AtomicString target(svg_target_->CurrentValue()->Value());
target = GetCleanTarget(target);```
AtomicString target = GetCleanTarget(svg_target_->CurrentValue()->Value());
```
to avoid unnecessary ref-churn.
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist, Stefan Zager.
2 comments:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
Since `<form>` is not a link, it's little weird to name `CleanLinkTarget`. IMO, `Element::GetCleanTarget` makes sense because the HTML spec specify it as `get an element's target`[1].
Given the apparent history here it seems like consolidation of the creation of the navigation data would've been a good direction from the start ...
Agreed.
File third_party/blink/renderer/core/svg/svg_a_element.cc:
AtomicString target(svg_target_->CurrentValue()->Value());
target = GetCleanTarget(target);
``` […]
I get the following error.
```
error: no viable conversion from 'const String' to 'const AtomicString'
GetCleanTarget(svg_target_->CurrentValue()->Value());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Zager (Gerrit)
Attention is currently required from: Fredrik Söderquist, Jun Kokatsu.
1 comment:
File third_party/blink/renderer/core/svg/svg_a_element.cc:
AtomicString target(svg_target_->CurrentValue()->Value());
target = GetCleanTarget(target);
I get the following error. […]
`AtomicString target = GetCleanTarget(AtomicString(svg_target_->CurrentValue()->Value()));`
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist, Stefan Zager.
Patch set 5:Commit-Queue +1
2 comments:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
Since `<form>` is not a link, it's little weird to name `CleanLinkTarget`. […]
I've made a change to call `GetCleanTarget` in `HTMLAnchorElement::GetEffectiveTarget`. Let me know if that improve things y'all pointed out.
File third_party/blink/renderer/core/svg/svg_a_element.cc:
AtomicString target(svg_target_->CurrentValue()->Value());
target = GetCleanTarget(target);
`AtomicString target = GetCleanTarget(AtomicString(svg_target_->CurrentValue()->Value()));`
Done!
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
Attention is currently required from: Jun Kokatsu, Stefan Zager.
1 comment:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
Since <form> is not a link, it's little weird to name CleanLinkTarget.
Fair enough. `CleanNavigationTarget` then perhaps.
It still doesn't "get" anything - it does some post/pre-processing on a value.
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist, Stefan Zager.
Patch Set #4, Line 8811:
AtomicString Element::GetCleanTarget(const AtomicString& target) {
> Since <form> is not a link, it's little weird to name CleanLinkTarget. […]
Done!
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Zager (Gerrit)
Attention is currently required from: Fredrik Söderquist, Jun Kokatsu.
4 comments:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;I agree with fs@ that it's weird for this to be defined as a static method on Element. I think it makes more sense to put it on blink::LocalDOMWindow; see following comments...
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #6, Line 521:
void LogDanglingMarkupHistogram(Document& document,```
void LogDanglingMarkupHistograme(ExecutionContext& context, ...) {
context.CountUse(...);
...
}
```Patch Set #6, Line 8825:
const AtomicString& Element::CleanNavigationTarget(```
const AtomicString& LocalDOMWindow::CleanNavigationTarget(...) {
...
LogDanglingMarkupHistogram(*this, target);
...
}
```
File third_party/blink/renderer/core/html/html_anchor_element.cc:
Patch Set #6, Line 395:
return CleanNavigationTarget(target);`return GetDocument().domWindow()->CleanNavigationTarget(target);`
or optionally put a convenience method on Document:
```
const AtomicString& Document::CleanNavigationTarget(target) {
return domWindow()->CleanNavigationTarget(target);
}
```
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist, Stefan Zager.
1 comment:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;
I agree with fs@ that it's weird for this to be defined as a static method on Element. […]
Though, we don't want this to be called from `window.open` or any other JS APIs (where they can specify `target`).
I'm not sure if keeping in `LocalDOMWindow` is the right answer, as it allows anyone outside of HTML to use `CleanNavigationTarget`.WDYT?
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Zager (Gerrit)
Attention is currently required from: Fredrik Söderquist, Jun Kokatsu.
1 comment:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;
Though, we don't want this to be called from `window. […]
Not sure I follow you. My reasoning is that navigation target is a bit of functionality that is more closely related to frame/window than to element. If you prefer, LocalFrame would also be a fine place to define it.
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist, Stefan Zager.
1 comment:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;
Not sure I follow you. […]
Right, the context of the HTML spec change[1] is to mitigate a type of dangling markup injection[2] which abuses `target` attribute in HTML to leak DOM information across origins[3].
Therefore, we only want to clean `target` value which comes from HTML element (e.g. `<a>`, `<form>`, `<input type="submit" formtarget="xyz">`) but not from JS APIs (e.g. `window.open`).
While I'm happy to move it to `LocalDOMWindow`, I think `Element` makes more sense because it is only meant to be used for HTML/SVG attributes.
[1] https://github.com/whatwg/html/pull/9309
[2] https://portswigger.net/web-security/cross-site-scripting/dangling-markup
[3] https://portswigger.net/research/evading-csp-with-dom-based-dangling-markup
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
Attention is currently required from: Jun Kokatsu, Stefan Zager.
1 comment:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;
Right, the context of the HTML spec change[1] is to mitigate a type of dangling markup injection[2] […]
The place where I'd like it to reside doesn't exist (yet?), so I'd be willing to offer some slack on that issue. I'll suggest just putting it as a free function in core/loader/frame_load_request.{cc,h} and document what it is for. If a suitable location were to appear it can be folded into that at that point.
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Zager (Gerrit)
Attention is currently required from: Fredrik Söderquist, Jun Kokatsu.
1 comment:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;
The place where I'd like it to reside doesn't exist (yet?), so I'd be willing to offer some slack on […]
+1 to `frame_load_request.(cc|h)`. It's just weird to be able to call `CleanNavigationTarget()` on any Element.
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist, Stefan Zager.
Patch Set #6, Line 1202:
const AtomicString& CleanNavigationTarget(const AtomicString& target) const;
+1 to `frame_load_request.(cc|h)`. […]
Done!
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #6, Line 521:
void LogDanglingMarkupHistogram(Document& document,
``` […]
Done
Patch Set #6, Line 8825:
const AtomicString& Element::CleanNavigationTarget(
``` […]
Done
File third_party/blink/renderer/core/html/html_anchor_element.cc:
Patch Set #6, Line 395:
return CleanNavigationTarget(target);
`return GetDocument().domWindow()->CleanNavigationTarget(target);` […]
Done
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Zager (Gerrit)
Attention is currently required from: Fredrik Söderquist, Jun Kokatsu.
Patch set 11:Code-Review +1
1 comment:
Patchset:
lgtm
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Jun Kokatsu (Gerrit)
Attention is currently required from: Fredrik Söderquist.
Patch set 11:Commit-Queue +2
1 comment:
Patchset:
lgtm
Thank you for reviewing!
To view, visit change 4595781. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Move Dangling Markup in target name logic to HTML element
This change moves checks for Dangling Markup in target name to HTML
element, to avoid bugs such as noopener not being set on new window when
Dangling Markup is detected, and better align implementation with the
new spec[1].
[1] https://github.com/whatwg/html/pull/9309
Bug: 1451273, 1421440
Change-Id: Ie98afc0c692338d295fa1ec1f407d7ba2f5817d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4595781
Reviewed-by: Stefan Zager <sza...@chromium.org>
Commit-Queue: Jun Kokatsu <jkok...@google.com>
Cr-Commit-Position: refs/heads/main@{#1155717}
---
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/loader/form_submission.cc
M third_party/blink/renderer/core/loader/frame_load_request.cc
M third_party/blink/renderer/core/loader/frame_load_request.h
M third_party/blink/renderer/core/page/frame_tree.cc
M third_party/blink/renderer/core/svg/svg_a_element.cc
6 files changed, 57 insertions(+), 46 deletions(-)