Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regular expression injection fix #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gitworkflows
Copy link

Sanitize User Input:

The code now escapes special characters in the versionedRegexString before creating the RegExp object. This prevents malicious users from injecting special characters that could modify the behavior of the regular expression.

The replace(/[-[]{}()*+?.,\^$|#\s]/g, "\$&") line escapes all special characters that have meaning in regular expressions.

Example:

Let's say a user inputs the following string as versionedRegex: (a*|.*)

Without sanitization, this could lead to a Denial of Service attack due to the exponential time complexity of the regex.

After sanitization, the regex becomes (a*|.*), which is safe and prevents the attack.

Important:

Security: Unsanitized user input in regular expressions can lead to serious security vulnerabilities, including:

Denial of Service (DoS): Malicious regexes can cause the application to consume excessive resources, leading to slowdowns or crashes.

Code Injection: In some cases, attackers could inject code into the application through crafted regexes.

Reliability: Sanitizing user input ensures that the regular expressions behave as expected and don't cause unexpected errors or crashes.

# Sanitize User Input:

The code now escapes special characters in the versionedRegexString before creating the RegExp object. This prevents malicious users from injecting special characters that could modify the behavior of the regular expression. 

The replace(/[-[\]{}()*+?.,\\^$|#\s]/g, "\\$&") line escapes all special characters that have meaning in regular expressions.

# Example:

Let's say a user inputs the following string as versionedRegex: (a*|.*)

Without sanitization, this could lead to a Denial of Service attack due to the exponential time complexity of the regex.

After sanitization, the regex becomes \(a\*\|\.\*\), which is safe and prevents the attack.

# Important:

Security: Unsanitized user input in regular expressions can lead to serious security vulnerabilities, including:

Denial of Service (DoS): Malicious regexes can cause the application to consume excessive resources, leading to slowdowns or crashes.

Code Injection: In some cases, attackers could inject code into the application through crafted regexes.

Reliability: Sanitizing user input ensures that the regular expressions behave as expected and don't cause unexpected errors or crashes.
@ferdnyc
Copy link

ferdnyc commented Dec 16, 2024

@gitworkflows

Example:

Let's say a user inputs the following string as versionedRegex: (a*|.*)

Without sanitization, this could lead to a Denial of Service attack due to the exponential time complexity of the regex.

After sanitization, the regex becomes (a*|.*), which is safe and prevents the attack.

You're probably gonna want to fence that, as GitHub is eating your sanitization. 😉

It should say:


Let's say a user inputs the following string as versionedRegex: (a*|.*)

Without sanitization, this could lead to a Denial of Service attack due to the exponential time complexity of the regex.

After sanitization, the regex becomes \(a\*\|\.\*\), which is safe and prevents the attack.


@ferdnyc
Copy link

ferdnyc commented Dec 16, 2024

@gitworkflows

Taking a step back, though... I'm confused. Doesn't this change also effectively disable the regular expression? \(a\*\|\.\*\) isn't a regular expression, really — all of the RE code has been escaped out of existence, leaving just a useless string that won't really match anything. The versionedRegexString is supposed to be a feature that relies on the user actually being able to supply a working regular expression.

Also, the regular expression configured by the user — whatever form it takes — is used in the author's own workflows. If they create an expression with exponential complexity, the only person they'll be DoSing is themselves. (Well, and GitHub, but they'll get over it. Like they'd even notice one wedged runner.)

I'm not sure how critical it is to bend over backwards protecting people from themselves. If they aren't allowed to mess things up, they'll be cheated of the chance to learn from their mistakes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants