Skip to content

fix: make sure that stripping backslashes for notification urls cannot cause catastophic backtracking (ReDOS) #5573

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

Merged
merged 8 commits into from
Jan 26, 2025

Conversation

ShiyuBanzhou
Copy link
Contributor

@ShiyuBanzhou ShiyuBanzhou commented Jan 25, 2025

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #5574
By modifying the regular expression matching rule to /(?<!/)/*$/, we prevent the matching process from starting with a slash. This change ensures that the matching can only start from the first slash and proceed toward the end. For each subsequent slash, the regular expression checks if the previous character is a slash; if so, it will not backtrack further.

.replace(/(?<!\/)\/*$/, "")

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

noredos

ShiyuBanzhou and others added 5 commits January 22, 2025 09:54
@CommanderStorm CommanderStorm added P1-high High Priority A:notifications Issues or PRs related to notifications labels Jan 25, 2025
@CommanderStorm CommanderStorm changed the title Bug Fix: fix regular item matching rules fix: make sure that stripping backslashes for notification urls cannot cause catastophic backtracking (ReDOS) Jan 25, 2025
ShiyuBanzhou and others added 2 commits January 26, 2025 01:15
More concise and reasonable regular expression fix

Co-authored-by: Frank Elsinga <frank@elsinga.de>
More concise and reasonable regular expression fix
@ShiyuBanzhou
Copy link
Contributor Author

/home/runner/work/uptime-kuma/uptime-kuma/server/notification-providers/pushdeer.js
Error: 15:53 error Unnecessary escape character: / no-useless-escape

/home/runner/work/uptime-kuma/uptime-kuma/server/notification-providers/whapi.js
Error: 27:93 error Unnecessary escape character: / no-useless-escape

@CommanderStorm
Copy link
Collaborator

Could you fix the linting issues?
We can merge afterwards.

@ShiyuBanzhou
Copy link
Contributor Author

I have fixed the issue with the regularization term, please check it out

@ShiyuBanzhou
Copy link
Contributor Author

Hello, sorry to bother you. I would like to ask if I want to apply for a CVE for this vulnerability, what should I do? Can I submit the vulnerability through the Security module in your warehouse? I submitted a document about this vulnerability not long ago. Application, but there has been no reply yet. I look forward to hearing from you. Thank you very much.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Only @louislam can manage the security reports.
I have no access there.
But for this one (i.e. not https://github.com/louislam/uptime-kuma/security/advisories/GHSA-5rg9-2hjc-q924, as that turned out to not be a vulnerability), I think we can publish one.

@CommanderStorm CommanderStorm merged commit 7a91917 into louislam:master Jan 26, 2025
19 checks passed
@ShiyuBanzhou
Copy link
Contributor Author

I will release a security vulnerability related to these two regularization terms. Could you please help me contact the author and ask them to pay attention as soon as possible? Thank you very much for your help

@ShiyuBanzhou
Copy link
Contributor Author

this is the security url:GHSA-hx7h-9vf7-5xhg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:notifications Issues or PRs related to notifications P1-high High Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug fix : correction of Regular Item Matching Rules
2 participants