Pin suggested-value pseudo elements to a generic font family [chromium/src : main]
Dileep Maurya (Gerrit)
Dileep Maurya added 2 comments![]( "Open in Gerrit")
Dileep MauryaGeneral questions:
Is it possible that a website was relying on this shadowing behavior and would be broken by this change?
Is there some way to scope our defense to the autofill popup? As I understand, the autofill popup is the only scenario where we'd leak information a webpage doesn't already have access to.
Thanks for these questions. The previous patch didn't address whether a site relying on the shadowing behavior could break. The new change addresses that by scoping the defense to the autofill preview only, so author content and committed input values are unaffected.
Dileep MauryaGeneral questions:
Is it possible that a website was relying on this shadowing behavior and would be broken by this change?
Is there some way to scope our defense to the autofill popup? As I understand, the autofill popup is the only scenario where we'd leak information a webpage doesn't already have access to.
Thanks for these questions. The previous patch didn't address whether a site relying on the shadowing behavior could break. Current patchset addresses that by scoping the defense to the autofill preview only, so author content and committed input values are unaffected.
Related details
- Kevin Babbitt
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
2 comments
font-family: sans-serif !important;Suggest adding a comment here explaining the importance of pinning to a system font keyword, for example:
```suggestion
/* Author @font-face rules with single-character unicode-ranges could create a
side channel for disclosure of autofill preview text. Prevent this by using
a generic font family keyword, which cannot be overridden by @font-face. */
font-family: sans-serif !important;
```
Related details
- Dileep Maurya
- Joey Arhar
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
platform name (e.g. "Arial"), so the !important rule onDoes this mean that adding `font-family:sans-serif!important` will actually change the behavior because its not arial anymore? Or does sans-serif resolve to arial in this case?
font-family: sans-serif !important;I am modifying the same code here: https://chromium-review.git.corp.google.com/c/chromium/src/+/7885624
I don't think that my patch adds a forced font-family like this. So `font: -webkit-small-control !important` doesn't actually force a specific font-family?
font-family: sans-serif !important;Suggest adding a comment here explaining the importance of pinning to a system font keyword, for example:
```suggestion
/* Author @font-face rules with single-character unicode-ranges could create a
side channel for disclosure of autofill preview text. Prevent this by using
a generic font family keyword, which cannot be overridden by @font-face. */
font-family: sans-serif !important;
```
Would it be possible to add a web_test which demonstrates this?
Related details
- Dileep Maurya
- Virali Purbey
Dileep Maurya (Gerrit)
Dileep Maurya added 3 comments![]( "Open in Gerrit")
Does this mean that adding `font-family:sans-serif!important` will actually change the behavior because its not arial anymore? Or does sans-serif resolve to arial in this case?
`sans-serif` is a generic family, so it doesn't resolve specifically to Arial, it maps to whatever the platform/user `generic-sans-serif` setting is. In practice:
- Windows/Linux: sans-serif and `-webkit-small-control` both resolve to Arial/Arimo, so there's no visible change, the existing reftests pass unmodified.
- macOS: they differ - `-webkit-small-control` is the system control font and sans-serif is Helvetica - so the autofill preview glyphs do change there (I updated the mac-rel reftest baselines for this).
The change is limited to the transient greyed-out preview text in `::-internal-input-suggested` / `::-internal-select-autofill-preview-text`; once a suggestion is accepted the field renders the real value in its own font, and textarea previews are unaffected.
font-family: sans-serif !important;I am modifying the same code here: https://chromium-review.git.corp.google.com/c/chromium/src/+/7885624
I don't think that my patch adds a forced font-family like this. So `font: -webkit-small-control !important` doesn't actually force a specific font-family?
The font shorthand resets every font longhand, and the system-font keyword resolves font-family to a concrete platform family `system_font->ResolveFontFamily() -> LayoutThemeFontProvider::SystemFontFamily() -> DefaultGUIFont()` (e.g. "Arial") — as a non-generic kFamilyName (style_builder_converter.cc). That concrete name is exactly what gets looked up in the author @font-face cache (`if (!font_family.FamilyIsGeneric()) guard in CSSFontSelector::GetFontData`), which is the side channel this CL closes. `font-family: sans-serif !important` overrides only the family with a generic one, which skips that lookup.
Your CL#7885624 — we overlap on `select::-internal-select-autofill-preview-text`. As long as the hard-coded family is a generic keyword (sans-serif / monospace), the security property holds; a concrete family name would reintroduce the leak. Happy to coordinate ordering - I can rebase on top of yours. PTAL.
Joey ArharSuggest adding a comment here explaining the importance of pinning to a system font keyword, for example:
```suggestion
/* Author @font-face rules with single-character unicode-ranges could create a
side channel for disclosure of autofill preview text. Prevent this by using
a generic font family keyword, which cannot be overridden by @font-face. */
font-family: sans-serif !important;
```
Would it be possible to add a web_test which demonstrates this?
Done. Added the suggested comment and added a web_test demonstrating the same. Thanks!
Related details
- Joey Arhar
- Kevin Babbitt
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
platform name (e.g. "Arial"), so the !important rule onDileep MauryaDoes this mean that adding `font-family:sans-serif!important` will actually change the behavior because its not arial anymore? Or does sans-serif resolve to arial in this case?
`sans-serif` is a generic family, so it doesn't resolve specifically to Arial, it maps to whatever the platform/user `generic-sans-serif` setting is. In practice:
- Windows/Linux: sans-serif and `-webkit-small-control` both resolve to Arial/Arimo, so there's no visible change, the existing reftests pass unmodified.
- macOS: they differ - `-webkit-small-control` is the system control font and sans-serif is Helvetica - so the autofill preview glyphs do change there (I updated the mac-rel reftest baselines for this).
The change is limited to the transient greyed-out preview text in `::-internal-input-suggested` / `::-internal-select-autofill-preview-text`; once a suggestion is accepted the field renders the real value in its own font, and textarea previews are unaffected.
Thanks for the explanation, its unfortunate that this changes the behavior on mac. can you add a runtime enabled features flag here in case anything goes wrong and we have to revert and find another way to fix this that doesn't change the fonts on mac?
font-family: sans-serif !important;Dileep MauryaI am modifying the same code here: https://chromium-review.git.corp.google.com/c/chromium/src/+/7885624
I don't think that my patch adds a forced font-family like this. So `font: -webkit-small-control !important` doesn't actually force a specific font-family?
The font shorthand resets every font longhand, and the system-font keyword resolves font-family to a concrete platform family `system_font->ResolveFontFamily() -> LayoutThemeFontProvider::SystemFontFamily() -> DefaultGUIFont()` (e.g. "Arial") — as a non-generic kFamilyName (style_builder_converter.cc). That concrete name is exactly what gets looked up in the author @font-face cache (`if (!font_family.FamilyIsGeneric()) guard in CSSFontSelector::GetFontData`), which is the side channel this CL closes. `font-family: sans-serif !important` overrides only the family with a generic one, which skips that lookup.
Your CL#7885624 — we overlap on `select::-internal-select-autofill-preview-text`. As long as the hard-coded family is a generic keyword (sans-serif / monospace), the security property holds; a concrete family name would reintroduce the leak. Happy to coordinate ordering - I can rebase on top of yours. PTAL.
thanks, im happy to deal with conflicts too if this patch gets merged first
Related details
- Dileep Maurya
- Kevin Babbitt
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
platform name (e.g. "Arial"), so the !important rule onDileep MauryaDoes this mean that adding `font-family:sans-serif!important` will actually change the behavior because its not arial anymore? Or does sans-serif resolve to arial in this case?
Joey Arhar`sans-serif` is a generic family, so it doesn't resolve specifically to Arial, it maps to whatever the platform/user `generic-sans-serif` setting is. In practice:
- Windows/Linux: sans-serif and `-webkit-small-control` both resolve to Arial/Arimo, so there's no visible change, the existing reftests pass unmodified.
- macOS: they differ - `-webkit-small-control` is the system control font and sans-serif is Helvetica - so the autofill preview glyphs do change there (I updated the mac-rel reftest baselines for this).
The change is limited to the transient greyed-out preview text in `::-internal-input-suggested` / `::-internal-select-autofill-preview-text`; once a suggestion is accepted the field renders the real value in its own font, and textarea previews are unaffected.
Thanks for the explanation, its unfortunate that this changes the behavior on mac. can you add a runtime enabled features flag here in case anything goes wrong and we have to revert and find another way to fix this that doesn't change the fonts on mac?
I just ran into a similar issue with my patch, and after reading the code it looks to me like every platform uses Arial for -webkit-small-control via LayoutThemeFontProvider::SystemFontFamily -> DefaultGUIFont. Where does mac get Helvetica?
If mac really does get Helvetica, should we just set that in a mac platform-specific css file or something? Maybe its not worth the effort but it should prevent this patch from actually changing anything which might be nice.
Dileep Maurya (Gerrit)
Dileep Maurya voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dileep Maurya
| Commit-Queue | +1 |
1 comment
platform name (e.g. "Arial"), so the !important rule onDileep MauryaDoes this mean that adding `font-family:sans-serif!important` will actually change the behavior because its not arial anymore? Or does sans-serif resolve to arial in this case?
Joey Arhar`sans-serif` is a generic family, so it doesn't resolve specifically to Arial, it maps to whatever the platform/user `generic-sans-serif` setting is. In practice:
- Windows/Linux: sans-serif and `-webkit-small-control` both resolve to Arial/Arimo, so there's no visible change, the existing reftests pass unmodified.
- macOS: they differ - `-webkit-small-control` is the system control font and sans-serif is Helvetica - so the autofill preview glyphs do change there (I updated the mac-rel reftest baselines for this).
The change is limited to the transient greyed-out preview text in `::-internal-input-suggested` / `::-internal-select-autofill-preview-text`; once a suggestion is accepted the field renders the real value in its own font, and textarea previews are unaffected.
Joey ArharThanks for the explanation, its unfortunate that this changes the behavior on mac. can you add a runtime enabled features flag here in case anything goes wrong and we have to revert and find another way to fix this that doesn't change the fonts on mac?
I just ran into a similar issue with my patch, and after reading the code it looks to me like every platform uses Arial for -webkit-small-control via LayoutThemeFontProvider::SystemFontFamily -> DefaultGUIFont. Where does mac get Helvetica?
If mac really does get Helvetica, should we just set that in a mac platform-specific css file or something? Maybe its not worth the effort but it should prevent this patch from actually changing anything which might be nice.
You're right, thanks for the catch, `-webkit-small-control` resolves to "Arial" on every platform, Mac included, via `LayoutThemeFontProvider::SystemFontFamily() -> DefaultGUIFont()`.
The "Helvetica" is introduced by current change: `sans-serif` is generic, and in our font config sans-serif maps to "Helvetica" on all platforms ( web_test_control_host.cc ). Helvetica is only installed on Mac, so Win/Linux fallback to Arial/Arimo, while Mac actually renders Helvetica. That's why only mac-rel rebaselined.
Why it has to be a generic family (why Mac changes): The leak is closed in `CSSFontSelector::GetFontData()` - the author @font-face cache is consulted only when `!font_family.FamilyIsGeneric()` ( css_font_selector.cc ). A generic keyword skips that lookup and resolves straight from settings, so an author @font-face can never be matched for the preview. A concrete family - including hard-coding Arial - still hits that author-cache lookup and would reintroduce exactly the side channel we're closing. As per my observation there seems to be no generic keyword that maps to Arial on Mac (sans-serif=Helvetica, serif=Times, monospace=Menlo), so any safe value necessarily changes the Mac glyphs.
On "set it in a Mac-specific CSS to avoid changing anything": I'd prefer that too, but that seems not possible with a concrete font, for the reason above. There is another approach with "C++ fix" that was introduced in PatchSet4 to keep Arial on Mac and stay safe (tag system-keyword-resolved families and bypass the author cache for them), which I descoped to keep this fix narrow. Happy to revisit if avoiding the Mac change is worth the larger surface.
Agreed on the flag - I've wrapped this change in a runtime enabled feature flag so it can be reverted. PTAL.
Related details
- Joey Arhar
- Kevin Babbitt
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
2 comments
platform name (e.g. "Arial"), so the !important rule onDileep MauryaDoes this mean that adding `font-family:sans-serif!important` will actually change the behavior because its not arial anymore? Or does sans-serif resolve to arial in this case?
Joey Arhar`sans-serif` is a generic family, so it doesn't resolve specifically to Arial, it maps to whatever the platform/user `generic-sans-serif` setting is. In practice:
- Windows/Linux: sans-serif and `-webkit-small-control` both resolve to Arial/Arimo, so there's no visible change, the existing reftests pass unmodified.
- macOS: they differ - `-webkit-small-control` is the system control font and sans-serif is Helvetica - so the autofill preview glyphs do change there (I updated the mac-rel reftest baselines for this).
The change is limited to the transient greyed-out preview text in `::-internal-input-suggested` / `::-internal-select-autofill-preview-text`; once a suggestion is accepted the field renders the real value in its own font, and textarea previews are unaffected.
Joey ArharThanks for the explanation, its unfortunate that this changes the behavior on mac. can you add a runtime enabled features flag here in case anything goes wrong and we have to revert and find another way to fix this that doesn't change the fonts on mac?
Dileep MauryaI just ran into a similar issue with my patch, and after reading the code it looks to me like every platform uses Arial for -webkit-small-control via LayoutThemeFontProvider::SystemFontFamily -> DefaultGUIFont. Where does mac get Helvetica?
If mac really does get Helvetica, should we just set that in a mac platform-specific css file or something? Maybe its not worth the effort but it should prevent this patch from actually changing anything which might be nice.
You're right, thanks for the catch, `-webkit-small-control` resolves to "Arial" on every platform, Mac included, via `LayoutThemeFontProvider::SystemFontFamily() -> DefaultGUIFont()`.
The "Helvetica" is introduced by current change: `sans-serif` is generic, and in our font config sans-serif maps to "Helvetica" on all platforms ( web_test_control_host.cc ). Helvetica is only installed on Mac, so Win/Linux fallback to Arial/Arimo, while Mac actually renders Helvetica. That's why only mac-rel rebaselined.
Why it has to be a generic family (why Mac changes): The leak is closed in `CSSFontSelector::GetFontData()` - the author @font-face cache is consulted only when `!font_family.FamilyIsGeneric()` ( css_font_selector.cc ). A generic keyword skips that lookup and resolves straight from settings, so an author @font-face can never be matched for the preview. A concrete family - including hard-coding Arial - still hits that author-cache lookup and would reintroduce exactly the side channel we're closing. As per my observation there seems to be no generic keyword that maps to Arial on Mac (sans-serif=Helvetica, serif=Times, monospace=Menlo), so any safe value necessarily changes the Mac glyphs.On "set it in a Mac-specific CSS to avoid changing anything": I'd prefer that too, but that seems not possible with a concrete font, for the reason above. There is another approach with "C++ fix" that was introduced in PatchSet4 to keep Arial on Mac and stay safe (tag system-keyword-resolved families and bypass the author cache for them), which I descoped to keep this fix narrow. Happy to revisit if avoiding the Mac change is worth the larger surface.
Agreed on the flag - I've wrapped this change in a runtime enabled feature flag so it can be reverted. PTAL.
lgtm, thanks for the context
@font-face. crbug.com/517710554. */Could you say more explicitly here that ideally we would be using Arial which is what -webkit-small-control does, but we can't because it hits the @font-face issue?
Related details
- Dileep Maurya
- Kevin Babbitt
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dileep Maurya (Gerrit)
Dileep Maurya added 1 comment![]( "Open in Gerrit")
Could you say more explicitly here that ideally we would be using Arial which is what -webkit-small-control does, but we can't because it hits the @font-face issue?
- Kevin Babbitt
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dileep Maurya (Gerrit)
Dileep Maurya voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Kevin Babbitt (Gerrit)
Kevin Babbitt voted Code-Review+1![]( "Open in Gerrit")
- Dileep Maurya
- Virali Purbey
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Pin suggested-value pseudo elements to a generic font family
`font: -webkit-small-control` resolves font-family to a concrete
platform name (e.g. "Arial"), so the !important rule on
`::-internal-input-suggested` and
`::-internal-select-autofill-preview-text` could still pick up an
author @font-face declared for that name. Append
`font-family: sans-serif !important` after the shorthand so the
preview text always resolves to a generic family, matching the
existing `textarea::-internal-input-suggested` rule.
- M third_party/blink/renderer/core/html/forms/html_input_element_test.cc
- M third_party/blink/renderer/core/html/resources/html.css
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/fast/forms/suggested-value-author-font-face-leak.html
- M third_party/blink/web_tests/fast/forms/text/input-appearance-autocomplete-expected.html
- M third_party/blink/web_tests/fast/forms/text/input-appearance-autocomplete-suggested-value-over-placeholder-value-expected.html
- M third_party/blink/web_tests/fast/forms/text/input-appearance-autocomplete-suggested-value-when-underlying-placeholder-is-removed-expected.html
- M third_party/blink/web_tests/fast/forms/text/input-appearance-autocomplete-suggestion-maxlength-expected.html
- M third_party/blink/web_tests/fast/forms/text/input-appearance-autocomplete-very-long-value-expected.html
- M third_party/blink/web_tests/fast/forms/text/input-appearance-autocomplete-with-initial-value-expected.html
- M third_party/blink/web_tests/fast/forms/text/password-input-suggested-value-appearance-expected.html
Code-Review: +1 by Joey Arhar, +1 by Kevin Babbitt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |