merddyin / ADDeploy

Used to deploy components to support an ESAE forest and RBAC model via native control.
MIT License
2 stars 0 forks source link

New-ADDTaskGroup contains unsafe regex replace #17

Closed PatrickOnGit closed 1 year ago

PatrickOnGit commented 1 year ago

The function New-ADDTaskGroup is using regex to replace some DN parts (which by iteself is not a good idea, as this is not "visible" in SQLite DB. Change the regex to use positiv lookbehind assertion to avoid replacing more than expected.

# Regex replace is not safe and my replace Domain DN parts
# using positiv lookbehind assertion to ensure the string gets only replaces if OU= is before  (?<=OU=)
#$DestinationDN = ($Destination -replace (Join-String -Strings $OrgElements -Separator "|"),$OUGlobal) -replace $FocusID,$FocusHash["Admin"]
$DestinationDN = ($Destination -replace "(?<=OU=)($(Join-String -Strings $OrgElements -Separator '|'))",$OUGlobal) -replace "(?<=OU=)$FocusID",$FocusHash['Admin']
merddyin commented 1 year ago

The Org elements are stored within the SQLite DB and are pulled at runtime. The structure is very proscriptive, as is the naming convention for the Task Delegation Groups. Unless someone deviated from the structure approach, it is highly unlikely for unexpected value replacement to occur. That said, I'll look into your suggestions.

PatrickOnGit commented 1 year ago

No it isn't. The regex is not safe as it does expand into the DC=domain,DC=net RDNs. Therefore, dependend on the domain name, it may replace half of the DN. It happened to me, otherwise I wouldn't have noticed ;-)

merddyin commented 1 year ago

Sorry...was looking at a lot of these yesterday, and I think I didn't look closely enough at this one. Yes, you are correct in that, if a domain contained the consecutive letters used for a Scope layer OU, then those letters would get replaced in the DN as well. It just so happens that I actually ran into this issue in the field, and the updated version of the code actually uses more explicit regex, though it does not use a positive look behind. The solution in place forces a prefix of 'OU=' for scope and focus, and it also now logs a debug in the event that the final result doesn't look accurate (for example, if the domain DN doesn't match the actual DN). It also has an additional bit of code that, in the event the scope layer isn't replicated in the Admin Focus, it automatically switches the path to use the value for the Global OU as specified in the OU_Core table.

You may be pleased to know that this new version has been uploaded as the Dev branch...not ready to PR it yet, as there are a number of known issues to be fixed, but at least now you can access the latest and greatest. Will leave this open until I do the PR.

PatrickOnGit commented 1 year ago

Thank you