Task #7031: Unsafe masking of a dynamic regex pattern - TYPO3.Flow - TYPO3 Forge

TYPO3 Flow Base Distribution TYPO3.Eel TYPO3.Flow TYPO3.Fluid TYPO3.Kickstart TYPO3.Welcome Packages Applications

Task #7031

Unsafe masking of a dynamic regex pattern

Added by Marcus Krause over 5 years ago. Updated almost 5 years ago.

Status:

Resolved

Start date:

2010-04-15

Priority:

Must have

Due date:

Assigned To:

Karsten Dambekalns

% Done:

100%

Category:

Security

Target version:

TYPO3 Flow Base Distribution - 1.0 alpha 8

Estimated time:

1.00 hour

Sprint:

Has patch:

PHP Version:

Complexity:

Description

F3\FLOW3\Security\RequestPattern\Uri has method matchRequest() that uses a dynamic regex pattern.

return (boolean)preg_match('/^' . str_replace('/', '\/', $this->uriPattern) . '$/', $request->getRequestUri()->getPath());

Masking is done by str_replace(). This is not sufficient; think of a pattern that contains with '\/'. There's a dedicated method preq_quote() that takes care of proper masking.

So masking itself should be

preg_quote($this->uriPattern, '/')

Please grep through the complete code to possibly find similiar code; I haven't done that (I am using forge repository interface right now).

Associated revisions

Revision 9ad22f0f
Added by Karsten Dambekalns over 5 years ago

[~TASK] FLOW3 (Security): Clarified what happens to the pattern in RequestPattern\Uri, resolves #7031.

History

#1 Updated by Karsten Dambekalns over 5 years ago

Category set to Security
Status changed from New to Needs Feedback
Assigned To set to Karsten Dambekalns
Start date deleted (~~2010-03-27~~)
Estimated time set to 1.00

Well, preg_quote() masks all characters that have a meaning in a regex. This is not wanted in this case, at least judging from the doc comments the pattern can be a full-fledged regex. Running that through preg_quote() would render it useless.

The question remains, whether the str_replace() is really helpful here. Either way the documentation needs to instruct the user on what happens to the pattern, so the right stuff can be fed into the class.

#2 Updated by Andreas Förthner over 5 years ago

yes, I'm all for leaving it as a real regex configuration option. But you are right that the current situation is somehow inconsistent. On the other hand it's just convenient not to have to escape slashes in a URI pattern. What do you think, should we just add a note to the documentation that slashes will be automatically escaped? Or should we drop this automatic escaping and make a real regex pattern out of this option?