Domain is passed as a REGEX pattern, not a literal string.#12937
Domain is passed as a REGEX pattern, not a literal string.#12937daviftorres wants to merge 8 commits intoapache:mainfrom
Conversation
Pulling upstream.
This change ensures consistency with how paths are parsed when updating a domain path. The modified line was passing the domain name as a literal string, but it is actually interpreted as a regular expression internally. I couldn’t find a way to exploit this issue, but it could still cause data corruption if a domain name accidentally contains regex metacharacters. Note that this same technique is already used in a similar situation on line 1118. A common example is when an organization uses its DNS name as the "domain" (tenant), like `company.com`. In this case, the `.` (dot) is treated as a regex wildcard, meaning it can match any character...
| if (newParentDomainResourceLimit == Resource.RESOURCE_UNLIMITED) { | ||
| return; | ||
| } | ||
|
|
||
| if (currentDomainResourceCount + newParentDomainResourceCount > newParentDomainResourceLimit) { |
There was a problem hiding this comment.
@daviftorres is this what you mean with the line already interpreting the domain as a regex? I do not see what you mean.
There was a problem hiding this comment.
You are correct @DaanHoogland . I also pointed out another part of the code where some character scaping is done for that reason, but incomplete.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #12937 +/- ##
=============================================
- Coverage 18.01% 3.52% -14.49%
=============================================
Files 5968 464 -5504
Lines 537218 40063 -497155
Branches 65977 7534 -58443
=============================================
- Hits 96801 1414 -95387
+ Misses 429498 38461 -391037
+ Partials 10919 188 -10731
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updated regex pattern to escape only the '/' character while noting potential risks with the wildcard '.' character.
| DomainVO domainWithResourceUsedByDomainToBeMoved = _domainDao.findById(entry.getKey()); | ||
|
|
||
| Pattern pattern = Pattern.compile(domainWithResourceUsedByDomainToBeMoved.getPath().replace("/", "\\/").concat(".*")); | ||
| Pattern pattern = Pattern.compile(domainWithResourceUsedByDomainToBeMoved.getPath().replace("/", "\\/").concat(".*")); // This only scaped one Regex metacharacter (/). The wildcard `.` is more common and dangerous in my opinion. |
There was a problem hiding this comment.
I did not propose a fix because I need help for that.
|
I have no idea if this is a sensible change @daviftorres , I cannot see the issue and do not see what the change entails in behavioural change. |
If a domain name contains regex metacharacters, renaming it can fail because the regex gets interpreted incorrectly. By the way, I’m fine if you decide not to proceed. I just wanted to point out an issue I came across, which could potentially lead to data corruption if certain conditions are met. |
2 participants
Description
This change ensures consistency with how paths are parsed when updating a domain path.
The modified line was passing the domain name as a literal string, but it is actually interpreted as a regular expression internally.
I couldn’t find a way to exploit this issue, but it could still cause data corruption if a domain name accidentally contains regex metacharacters.
Note that this same technique is already used in a similar situation on line 1118.
A common example is when an organization uses its DNS name as the "domain" (tenant), like
company.com.In this case, the
.(dot) is treated as a regex wildcard, meaning it can match any character...Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
This fix was not tested. I am not a Java developer, and I have no skill for such a thing.
How did you try to break this feature and the system with this change?
I could trigger, but I could not exploit this bug.