Connect: Enable font configuration

[gzdunek]

Part of #21914

As we discussed in the last design session, we do not want to configure the main font of the application. I think in this case the best option is to remove it from the config schema completely.

Input validation

We will let users provide their own fonts by configuring them in app_config.json. This creates the security problem of injecting arbitrary CSS.

I prepared a test string ("Arial;} html {background-color: red;}") and passed it to fontFamily.mono to see if it is injected into the app.

At first I started with CSS.escape, as advised in styled-components. I assumed that I should pass only the value I wanted to sanitize and that's all. Unfortunately, it turned out that this function also escapes commas and single quotes, so the result of:

CSS.escape("Menlo, Monaco, 'Courier New', monospace")

was

font-family: Menlo\,\ Monaco\,\ \'Courier\ New\'\,\ monospace;

which didn't work in a browser.
(But maybe I did something wrong with CSS.escape?)

Then I found this great article https://frontarm.com/james-k-nelson/how-can-i-use-css-in-js-securely/ and tried the approach with style. I think is this the way to go. When an invalid string is passed as a style, it just becomes empty.
I'm only afraid that this solution is a bit "hacky" (mainly in xterm), but I didn't find anything better :(

Remote content

From what I have tested, CSP protects us from loading some remote content using url().
And for font-family, url() is not even an allowed value.

It seems that the worst thing a malicious CSS can do is changing the styles, but I think that input validation is a good thing anyway.

[ravicious]

Thanks for the link to that frontarm.com article, it gives a comprehensive overview of the attack vectors I was worried about.

The solution you chose seems to be a good fit and you provided a detailed justification as to why it's secure. 👍

I'll give it a try on Monday as for some reason my brain has huge problems with switching contexts today.

Until then, I've read through some additional resources when reviewing this PR to find another source which would confirm that doing this through style is safe. I couldn't find anything which would dispute this claim. The only thing is that our solution would potentially allow passing url(…) but as you said, it's not supported for font-family. Take a look at those resources though, maybe you'll find something that I missed.

• https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html
• https://security.stackexchange.com/questions/60712/is-xss-through-style-attribute-possible-in-modern-browsers
• https://stackoverflow.com/questions/3607894/cross-site-scripting-in-css-stylesheets
• https://code.google.com/archive/p/browsersec/wikis/Part1.wiki#Cascading_stylesheets
  • This talks about the expression directive which I think was since removed from CSS? The post is several years old.
• https://www.mike-gualtieri.com/posts/stealing-data-with-css-attack-and-defense
• https://owasp.org/www-project-web-security-testing-guide/stable/4-Web_Application_Security_Testing/11-Client-side_Testing/05-Testing_for_CSS_Injection

[zmb3]

@jentfoo added you as a reviewer to this one - would appreciate your review from a security perspective.

[ravicious]

For the record, the attack vector I'm thinking of is a malicious actor modifying app_config.json of the user to inject CSS to set up other attacks. They'd need to already have an access to the victim's computer.

[ravicious]

Connect's Content Security Policy is in the webpack config.

[gzdunek]

I took a look at the links you provided @ravicious and yes, they seem to confirm what we think.

This talks about the expression directive which I think was since removed from CSS? The post is several years old.

From OWASP:

From my experience, calling the expression() function from an execution context (JavaScript) has been disabled.

---

However, I spend some time today trying to find more resources regarding setting style in React, and this post was interesting: facebook/react#3473 (comment)

It's also important to document that style={{backgroundColor: userColor}} is also exploitable and allows injection of arbitrary CSS. It won't stop the problem, but just having it publicly documented is a big step (seeing as it's unlikely that we can ever out-right prevent it when targeting HTML).

I assume it refers to setting multiple properties as a "one" property: border: '1px solid black; background: red'.
However it looks like it was fixed a long time ago facebook/react#5878 (comment) (and I couldn't repro this).

From what I see React sets style using JS API. I was worried that it uses some weird tricks to set the styles, but since it simply uses the API to do it, the browser should protect us.

While digging in React codebase, I found this comment, but I think it relates to things like url?

[ravicious]

Good finds there, Grzegorz.

While digging in React codebase, I found this comment, but I think it relates to things like url?

I cloned the React repo and it looks like dangerousStyleValue is used only in two places: createDangerousStringForStyles and setValueForStyles. The first one is used only in dev mode when using SSR. It constructs the whole style string, such as display: block; color: green; which I assume is then returned from the server, so here it's totally possible to inject arbitrary CSS.

setValueForStyles uses dangerousStyleValue too, but it sets the style using style[styleName] = styleValue which is safe as far as I understand.

[ravicious]

It looks like Powerline glyphs work just fine when choosing a font which supports them:

https://gist.github.com/gdetrez/5845092

[ravicious]

Should we also let people pick terminal font size while we're at it? When I'm changing my terminal/editor font, I often adjust the size too because different fonts have different actual line heights and so on.

[ravicious]

LGTM, though I'd wait for @jentfoo before merging.

Any plans for docs and test plan updates? I was going to suggest that since the web code now lives in the same repo as docs, it could be a good idea to include doc changes as well. But on a second thought I'm not sure about it – submitting a separate PR with docs + test plan only shouldn't make that much of a difference.

[gzdunek]

LGTM, though I'd wait for @jentfoo before merging.

👍

Any plans for docs and test plan updates? I was going to suggest that since the web code now lives in the same repo as docs, it could be a good idea to include doc changes as well. But on a second thought I'm not sure about it – submitting a separate PR with docs + test plan only shouldn't make that much of a difference.

I think it is better to send a separate PR with changes to the docs, so the docs team will review them at once.
I will push a PR covering the config in Connect after implementing all parts.

[jentfoo]

Thank you for looping me in, and thank you @gzdunek for looking into the mechanics of this closer and help answer some of my questions in DM.

This appears to be safe to me also. The concerns I had, @gzdunek helped explain away, and I do agree with him that it appears any script injection here would be a compromise of the contract the browser is presenting to us.

Whenever relying on browser protections to that level there is of course risk that out of date browsers may be at risk. However I don't believe that needs to be considered as part of this change.

[ravicious]

Ah damn, forgot to say that I don't think we need to be as strict here, we could do like min 1 and max, idk, 256. I'd rather allow super tiny values since it doesn't change anything for us but might enable some niche use cases for the users.

[gzdunek]

I think we can do it.

[public-teleport-github-review-bot]

@gzdunek See the table below for backport results.

Branch	Result
branch/v12	Create PR