Add ReDoS linter + remove complex regexes #3602

antgamdia · 2021-10-18T19:15:12Z

Description of the change

After an open issue, we discovered we are adding some ReDoS vulnerable regexes, meaning some malformed (or even legal) inputs could make Kubeapps crash. Since it is mainly client code, the worst-case scenario is a browser tab crash, but, of course, it is sth we should prevent from happening.

This PR is just to add a linter checking the existence of regex that could result in a problem (aka having quadratic or exponential complexity).

Benefits

We will detect problematic regex at build time.

Possible drawbacks

Perhaps the lining step will take a bit longer, but I think it is worth it.

Applicable issues

closes UI: InternalError too much recursion when the ns exceeds some length #3587

Additional information

~~This PR won't pass the tests since currently, we have some vulnerable regexes. At some point it will, though (hopefully).~~

Whereas this PR was initially for adding the linter, I take the opportunity to also remove the complex regexes we were using.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity · 2021-10-18T22:43:40Z

This PR won't pass the tests since currently, we have some vulnerable regexes. At some point it will, though (hopefully).

Can we just update the offending lines in this PR so it's landable? Or is there an issue doing so?

antgamdia · 2021-10-19T09:37:18Z

Can we just update the offending lines in this PR so it's landable? Or is there an issue doing so?

We could ignore the offending lines, but solving them would require some extra effort. In fact, solving it would address this issue #3587 (comment).
The main problem here is coming up with a regex validating a "valid URL, but without including some characters and with a complexity less than O(x^n)". The other one should be easier to fix, though.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2021-10-19T15:06:09Z

dashboard/src/components/AppView/AccessURLTable/AccessURLItem/AccessURLIngressHelper.tsx

+export function ShouldGenerateLink(url: string): boolean {
+  // Check if it is a valid URL, delegating the check to the browser API
+  let builtURL;
+  try {
+    // Note this browser API constructor will accept many invalid URLs (not well-encoded, wrong IP octets, etc.)
+    builtURL = new URL(url);
+  } catch (_) {
+    return false;
+  }
+  if (!(builtURL.protocol === "http:" || builtURL.protocol === "https:")) {
+    return false;
+  }
+
+  // Check if any url fragment includes a character likely used in a regex, that is, is not encoded
+  // c.f. https://datatracker.ietf.org/doc/html/rfc3986#section-2.2
+  // even if it won't catch every possible case, we avoid having an O(x^n) regex
+  const regex = /^[^:/?#[\]@!$&'*+,;=]*$/;
+
+  // If it is an URL but it includes some "ilegal" characters, then return false
+  return !builtURL.pathname.split("/")?.some(p => !regex.test(p));


This is the new proposed way to check if the URL is "renderable" or not.
Instead of using a complex regex, we just delegate the tedious check to the browser API and then just apply an O(n) regex to check if it contains some illegal (aka non-encoded) character.

Some false positives and negatives could arise, but the drawback is just a link being rendered (nor not) in the UI. I mean, the text will always appear; it is just the href target. We better fail in generating the link rather than blocking the UI because of a malformed string.

Yep, sounds good.

antgamdia · 2021-10-19T15:08:34Z

dashboard/src/shared/Auth.ts

-      "base64url.bearer.authorization.k8s.io." + btoa(token).replace(/=*$/g, ""),
+      // Trimming the b64 padding character ("=") as it is not accepted by k8s
+      // https://github.com/kubernetes/apiserver/blob/master/pkg/authentication/request/websocket/protocol.go#L38
+      "base64url.bearer.authorization.k8s.io." + btoa(token).replace("=", ""),


Given that = is always a padding character in base64, we can safely remove it without any fancy (but O(n²) haha regex)

.

antgamdia · 2021-10-19T15:09:27Z

dashboard/src/components/AppView/AccessURLTable/AccessURLItem/AccessURLIngressHelper.tsx

@@ -1,9 +1,6 @@
 import { IIngressSpec, IIngressTLS, IResource } from "shared/types";
 import { IURLItem } from "./IURLItem";

-const isURLRegex =


Happy to remove this monolith:

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity

Nice solution.

absoludity · 2021-10-19T22:34:46Z

dashboard/src/components/AppView/AccessURLTable/AccessURLItem/AccessURLIngressHelper.tsx

+  } catch (_) {
+    return false;
+  }
+  if (!(builtURL.protocol === "http:" || builtURL.protocol === "https:")) {


I assume we don't need to worry about data: urls, given they weren't matched by the previous regex.

absoludity · 2021-10-19T22:40:45Z

dashboard/src/components/AppView/AccessURLTable/AccessURLItem/AccessURLIngressHelper.tsx

+export function ShouldGenerateLink(url: string): boolean {
+  // Check if it is a valid URL, delegating the check to the browser API
+  let builtURL;
+  try {
+    // Note this browser API constructor will accept many invalid URLs (not well-encoded, wrong IP octets, etc.)
+    builtURL = new URL(url);
+  } catch (_) {
+    return false;
+  }
+  if (!(builtURL.protocol === "http:" || builtURL.protocol === "https:")) {
+    return false;
+  }
+
+  // Check if any url fragment includes a character likely used in a regex, that is, is not encoded
+  // c.f. https://datatracker.ietf.org/doc/html/rfc3986#section-2.2
+  // even if it won't catch every possible case, we avoid having an O(x^n) regex
+  const regex = /^[^:/?#[\]@!$&'*+,;=]*$/;
+
+  // If it is an URL but it includes some "ilegal" characters, then return false
+  return !builtURL.pathname.split("/")?.some(p => !regex.test(p));


Yep, sounds good.

absoludity · 2021-10-19T22:42:10Z

dashboard/src/shared/Auth.ts

@@ -42,7 +42,9 @@ export class Auth {
      return [];
    }
    return [
-      "base64url.bearer.authorization.k8s.io." + btoa(token).replace(/=*$/g, ""),
+      // Trimming the b64 padding character ("=") as it is not accepted by k8s
+      // https://github.com/kubernetes/apiserver/blob/master/pkg/authentication/request/websocket/protocol.go#L38


Since code changes in main branches, best to link to the code in the latest tag imo. (much less likely to change).

absoludity · 2021-10-19T22:57:27Z

dashboard/src/shared/Auth.ts

-      "base64url.bearer.authorization.k8s.io." + btoa(token).replace(/=*$/g, ""),
+      // Trimming the b64 padding character ("=") as it is not accepted by k8s
+      // https://github.com/kubernetes/apiserver/blob/master/pkg/authentication/request/websocket/protocol.go#L38
+      "base64url.bearer.authorization.k8s.io." + btoa(token).replaceAll("=", ""),


I was wondering why you weren't just removing them from the end, but given that = isn't part of radix-64, they can only be present as padding.

Yep, it won't appear if the string is a multiple of a 3 character number since b64 encodes each 3 bytes. There is a good answer here: https://stackoverflow.com/questions/6916805/why-does-a-base64-encoded-string-have-an-sign-at-the-end
As you said, "=" can only be padding, so it is safe to be wiped out.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia added 3 commits October 18, 2021 19:13

Add redos eslint plugin

5f99a77

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Skip redos check in some tests

0777c75

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Remove leftover lines

0b4db3d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia added 3 commits October 19, 2021 12:24

Merge branch 'master' into addRedosLinter

19616e7

Remove unnecesary regex

ddf20f2

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Avoid complex regex for checking URLs

c6c50de

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia changed the title ~~Add ReDoS linter~~ Add ReDoS linter + remove complex regexes Oct 19, 2021

antgamdia commented Oct 19, 2021

View reviewed changes

Use replaceAll instead of replace

106b5b3

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia mentioned this pull request Oct 19, 2021

UI: InternalError too much recursion when the ns exceeds some length #3587

Closed

antgamdia marked this pull request as ready for review October 19, 2021 15:19

absoludity approved these changes Oct 19, 2021

View reviewed changes

antgamdia added 2 commits October 20, 2021 23:32

Merge branch 'master' into addRedosLinter

f3f3810

Use a fixed tag instead of the main branch in the comment

09612c2

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia merged commit 04e8109 into vmware-tanzu:master Oct 20, 2021

antgamdia deleted the addRedosLinter branch October 20, 2021 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReDoS linter + remove complex regexes #3602

Add ReDoS linter + remove complex regexes #3602

antgamdia commented Oct 18, 2021 •

edited

absoludity commented Oct 18, 2021

antgamdia commented Oct 19, 2021

antgamdia Oct 19, 2021

absoludity Oct 19, 2021

antgamdia Oct 19, 2021

antgamdia Oct 19, 2021

absoludity left a comment

absoludity Oct 19, 2021

absoludity Oct 19, 2021

absoludity Oct 19, 2021

absoludity Oct 19, 2021

antgamdia Oct 20, 2021

Add ReDoS linter + remove complex regexes #3602

Add ReDoS linter + remove complex regexes #3602

Conversation

antgamdia commented Oct 18, 2021 • edited

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity commented Oct 18, 2021

antgamdia commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia commented Oct 18, 2021 •

edited