[breaking] trust setting to indicate whether input text is trusted by edemaine · Pull Request #1794 · KaTeX/KaTeX

edemaine · 2018-11-24T22:36:13Z

Here is a draft of a trust setting to indicate whether the input text is trusted (and thus fix #1771), also allowing a function (like we do with strict) to depend on the specific function that we're worried about (advanced usage).

I did not add support for an array of trusted things like I suggested in #1771, in order to keep this simple, but happy to add something like that if we want.

I also have not added any tests; wanted to see if people like this interface first.

ylemkimon · 2018-11-26T06:10:19Z

I was thinking of a dictionary like:

{
    allowedProtocols: ["https", "http"],
    allowedClass: /katex-.+/,
    ....
}

but I think the function approach is better.

ylemkimon · 2018-11-26T06:12:27Z

docs/options.md

    (outside an array/tabular environment).  In strict mode, no line break
    results, as in LaTeX.
+- `trust`: `boolean` or `function` (default: `false`). If `false` (do not trust input), prevent any commands like `\includegraphics` that could enable adverse behavior, rendering them instead in `errorColor`. If `true` (trust input), allow all such commands. Provide a custom function `handler(command, ...)` to customize behavior depending on the command and possibly its arguments (e.g. a URL).  A list of such commands:
+  - `"\\includegraphics"`, with URL argument


What do you think of deprecating allowedProtocols and allow setting allowed protocols for \url and \href here?

ylemkimon · 2018-11-26T06:18:55Z

src/functions/includegraphics.js

            alt = alt.substring(0, alt.lastIndexOf('.'));
        }

+        if (!parser.settings.isTrusted("\\includegraphics", src)) {


I think we should provide context information as the second argument like:

{ type: "url", url: "https://katex.org/image.png", protocol: "https", ... }

or

{ type: "html", class: "my-class", }

to allow setting across commands and prevent mis-parsing the url.

edemaine · 2018-12-28T20:57:03Z

@ylemkimon Sorry for the long delay on looking at this. I've just pushed a new proposal where each call to the trust function gets a TrustContext which includes the command but also e.g. url and protocol as you suggested. What do you think?

Here are some sample uses: (which maybe I should add to documentation)

Forbid specific command: trust: (context) => context.command !== '\includegraphics'
Allow specific command: trust: (context) => context.command === '\includegraphics'
Allow specific protocol: trust: (context) => context.protocol === 'http'
Forbid specific protocol: trust: (context) => context.protocol !== 'file'

I agree that this could replace allowedProtocols, if I test for trust in \url and \href as well. In that case, however, the default trust of false will forbid all \url and \href calls. Is that what we want? Maybe, actually.

codecov-io · 2018-12-28T21:03:00Z

Codecov Report

Merging #1794 into master will decrease coverage by 0.05%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #1794      +/-   ##
==========================================
- Coverage   93.39%   93.34%   -0.06%     
==========================================
  Files          79       79              
  Lines        4981     4988       +7     
  Branches      872      876       +4     
==========================================
+ Hits         4652     4656       +4     
- Misses        289      291       +2     
- Partials       40       41       +1

Flag	Coverage Δ
#screenshotter	`89.06% <28.57%> (-0.1%)`	⬇️
#test	`86.5% <87.5%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
src/parseNode.js	`84.21% <ø> (ø)`	⬆️
src/defineFunction.js	`93.75% <ø> (ø)`	⬆️
src/functions/includegraphics.js	`0% <0%> (ø)`	⬆️
src/utils.js	`93.75% <100%> (+0.64%)`	⬆️
src/Settings.js	`78.57% <100%> (+2.25%)`	⬆️
src/Parser.js	`95.29% <100%> (-0.09%)`	⬇️
src/functions/href.js	`96% <100%> (-4%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc79f79...c80f5be. Read the comment docs.

ylemkimon · 2018-12-31T18:15:00Z

Is that what we want? Maybe, actually.

I think we should be as conservative as possible, not allowing anything by default.

edemaine · 2019-02-09T21:19:24Z

@ylemkimon Agreed, safest does seem like a good default. This will break the existing release, though we can give a setting trust: (context) => ['\\url ', '\\href'].includes(context.command) && ['http', 'https', 'mailto', '_relative'].include s(context.protocol) (already almost an example in the documentation) that mimics the current trust behavior.

This change breaks a lot of tests, which I still need to fix. Also we should either remove or deprecate allowedProtocols.

I think we should try to settle something soon, because #1842 is a pretty critical bug, so once fixed, we're going to want to release -- but I also think we can't release with \includegraphics (already merged) before we have a trust setting.

* Check `isTrusted` in `\url` and `\href` (so now disabled by default) * Automatically compute `protocol` from `url` in `isTrusted`, so it doesn't need to be passed into every context.

k4b7 · 2019-03-17T22:44:52Z

@edemaine this is a big change. Thanks for tackling this! ❤️

ylemkimon · 2019-03-23T12:30:36Z

docs/security.md

+with untrusted inputs; refer to [Options](options.md) for more details.
+* `maxSize` can prevent large width/height visual affronts.
+* `maxExpand` can prevent infinite macro loop attacks.
+* `allowedProtocols` can prevent certain protocols in URLs (e.g., with `\href`)


I think as we're in 0.x (major version zero) stage, we can remove allowedProtocols without deprecation.

ylemkimon · 2019-03-23T14:46:22Z

src/utils.js

+ * Return the protocol of a URL, or "_relative" if the URL does not specify a
+ * protocol (and thus is relative).
+ */
+export const urlToProtocol = function(url: string): string {


protocolFromUrl or getProtocol(FromUrl)

I'm going to go with protocolFromUrl since we use fooFromBar in a number of places already.

ylemkimon

Could you update the PR?

k4b7 · 2019-07-05T20:06:18Z

I resolved the merge conflicts which were due to this PR modifying the /includegraphics docs and the docs being deleted on master. The resolution was to delete them here as well. We can add the docs back in the future once we re-enable /includegraphics after this PR is merged.

k4b7 · 2019-07-05T20:06:31Z

@ylemkimon I can make the changes you requested.

k4b7 · 2019-07-05T20:14:41Z

Looks like there are some flow errors after the merge. I'll fix those up too.

k4b7 · 2019-07-05T23:41:27Z

docs/options.md

+
+  - Forbid specific command: `trust: (context) => context.command !== '\\includegraphics'`
+  - Allow specific command: `trust: (context) => context.command === '\\url'`
+  - Allow multiple specific commands: `trust: (context) => ['\\url', '\\href'].includes(context.command)`


@edemaine I really like these examples.

k4b7 · 2019-07-05T23:42:15Z

docs/security.md

+with untrusted inputs; refer to [Options](options.md) for more details.
+* `maxSize` can prevent large width/height visual affronts.
+* `maxExpand` can prevent infinite macro loop attacks.
+* `trust` can allow certain commands that are not always safe (e.g., `\includegraphics`)


I was thinking of changing to something else, but I think we can just re-enable \includegraphics once this PR is merged.

k4b7 · 2019-07-05T23:44:13Z

src/Parser.js

                }
-                result = this.handleUnsupportedCmd();
+                result = this.formatUnsupportedCmd(text);
+                this.consume();


The consume was moved out of formatUnsupported to here.

k4b7 · 2019-07-05T23:45:06Z

src/Settings.js

+        protocol?: string,
+    |},
+    "\\includegraphics": {|
+        command: "\\includegraphics",


I'm going to leave this in since we want to re-enable \includegraphics once this lands.

k4b7 · 2019-07-05T23:48:22Z

src/Settings.js

+    "\\url": {|
+        command: "\\url",
+        url: string,
+        protocol?: string,


While this could be restructured to dedupe the url and protocol we may have other commands that are insecure in other ways in the future.

k4b7 · 2019-07-05T23:48:52Z

src/defineFunction.js

    args: AnyParseNode[],
    optArgs: (?AnyParseNode)[],
-) => ParseNode<NODETYPE>;
+) => UnsupportedCmdParseNode | ParseNode<NODETYPE>;


I had to flip the order to make flow happy. I'll add a link to the issue in the code.

k4b7 · 2019-07-05T23:51:09Z

src/utils.js

+ * Return the protocol of a URL, or "_relative" if the URL does not specify a
+ * protocol (and thus is relative).
+ */
+export const urlToProtocol = function(url: string): string {


I'm going to go with protocolFromUrl since we use fooFromBar in a number of places already.

k4b7 · 2019-07-05T23:52:11Z

src/Settings.js

+     * should be an object with `command` field specifying the relevant LaTeX
+     * command (as a string starting with `\`), and any other arguments, etc.
+     * If `context` has a `url` field, a `protocol` field will automatically
+     * get added by this function (changing the specified object).


This comment is great!

…mUrl

k4b7 · 2019-07-05T23:54:28Z

I renamed this PR to having [breaking] in the title as a reminder to ourselves to bump the version when publishing. @ylemkimon I think I've addressed all of your concerns. Please have another look when you have some time.

edemaine · 2019-07-06T12:38:05Z

Thanks @kevinbarabash for picking up my slack here. (And sorry I've been too busy lately to do it myself.)

Given that \includegraphics was blocking only on this, perhaps the resolution should have been to put it back in? Up to you though.

edemaine · 2019-07-06T12:43:26Z

test/katex-spec.js

-            allowedProtocols: [],
-        }));
-    });
-


It would be nice to replace these with tests of the corresponding trust settings (as in the documentation examples). In general, it would be good to add trust tests. Because trust violations don't throw errors, you could probably just use snapshot tests... Or test for the appropriate parse node.

Good idea. Will do.

I've re-added the tests but I had to refactor them a bit to be snapshot tests since \href with trust: false parses, but produces a different parse tree from when trust: true is set.

I've also set the default for the unit tests to trust: false. I can probably remove that setting though since false is the default.

k4b7 · 2019-07-06T13:08:09Z

Given that \includegraphics was blocking only on this, perhaps the resolution should have been to put it back in? Up to you though.

I'd like to keep this PR focused on the new trust setting. Re-enabling \includegraphics will require adding screenshots and tests that are unrelated to rest of this PR. I can create a stacked PR so we can see what those changes look like before this is merged.

edemaine · 2019-07-06T13:15:53Z

Makes sense. We'll have the old commits anyway to guide the small changes needed for \includegraphics. I don't think it's necessary to stack PRs.

…he default

ylemkimon

I think this is good to go!

ylemkimon reviewed Nov 26, 2018

View reviewed changes

ylemkimon added GH Review: review-needed security and removed GH Review: review-needed labels Nov 26, 2018

edemaine mentioned this pull request Feb 5, 2019

Add \class and \cssId on non-strict mode #1437

Closed

edemaine added 7 commits February 9, 2019 16:37

trust option to indicate whether input text is trusted

43637aa

Revamp into trust contexts beyond just command

975b843

Document new trust function style

310f8f1

Fix screenshot testing

813bc42

Use trust setting in \url and \href

1e987ef

* Check `isTrusted` in `\url` and `\href` (so now disabled by default) * Automatically compute `protocol` from `url` in `isTrusted`, so it doesn't need to be passed into every context.

Document untrusted features in support list/table

420fe03

Existing tests trust by default

acb9a7f

edemaine force-pushed the trust branch from a6f8c20 to acb9a7f Compare February 9, 2019 21:44

edemaine mentioned this pull request Mar 18, 2019

Release v0.10.1 #1856

Merged

ylemkimon reviewed Mar 23, 2019

View reviewed changes

ylemkimon requested changes May 30, 2019

View reviewed changes

Merge branch 'master' into trust

1a271d5

remove allowedProtocols and fix flow errors

3e1b288

k4b7 changed the title ~~trust setting to indicate whether input text is trusted~~ [breaking] trust setting to indicate whether input text is trusted Jul 5, 2019

remove 'allowedProtocols' from documentation

78ca6bf

k4b7 reviewed Jul 5, 2019

View reviewed changes

add a comment about a flow error, rename urlToProtocol to protocolFro…

0906be4

…mUrl

edemaine commented Jul 6, 2019

View reviewed changes

k4b7 added the GH Review: needs-revision label Jul 6, 2019

k4b7 self-assigned this Jul 6, 2019

Kevin Barabash added 2 commits July 6, 2019 15:26

add tests test that use function version of trust option

65a35f0

default trust to false in MathML tests

744dce7

k4b7 added GH Review: review-needed and removed GH Review: needs-revision labels Jul 6, 2019

fix test title, remove 'trust: false' from test settings since it's t…

1838706

…he default

k4b7 mentioned this pull request Jul 7, 2019

Add \html command to insert HTML #1596

Closed

ylemkimon approved these changes Jul 8, 2019

View reviewed changes

Merge branch 'master' into trust

c80f5be

k4b7 merged commit 3800dc4 into KaTeX:master Jul 9, 2019

edemaine mentioned this pull request Jul 19, 2019

Re-enable \includegraphics #2053

Merged

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Uh oh!

Conversation

edemaine commented Nov 24, 2018

Uh oh!

ylemkimon commented Nov 26, 2018

Uh oh!

ylemkimon Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edemaine commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ylemkimon commented Dec 31, 2018

Uh oh!

edemaine commented Feb 9, 2019

Uh oh!

k4b7 commented Mar 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ylemkimon left a comment

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Jul 5, 2019

Uh oh!

k4b7 commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k4b7 commented Jul 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Jul 5, 2019

Uh oh!

edemaine commented Jul 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Jul 6, 2019

Uh oh!

edemaine commented Jul 6, 2019

Uh oh!

ylemkimon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ylemkimon Nov 26, 2018 •

edited

Loading

edemaine commented Dec 28, 2018 •

edited

Loading

codecov-io commented Dec 28, 2018 •

edited

Loading

k4b7 commented Jul 5, 2019 •

edited

Loading