Task #7031

Unsafe masking of a dynamic regex pattern

Added by Marcus Krause over 11 years ago. Updated about 11 years ago.

Status:
Resolved
Priority:
Must have
Category:
Security
Start date:
2010-04-15
Due date:
% Done:

100%

Estimated time:
1.00 h
Sprint:
PHP Version:
Has patch:
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).

#1

Updated by Karsten Dambekalns over 11 years ago

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

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 11 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?

#3

Updated by Karsten Dambekalns over 11 years ago

I am for leaving it like it is, but making clear what happens in the documentation.

#4

Updated by Karsten Dambekalns over 11 years ago

  • Due date set to 2010-04-15
  • Target version set to 1.0 alpha 8
#5

Updated by Karsten Dambekalns over 11 years ago

  • Due date deleted (2010-04-15)
  • Start date set to 2010-04-15
#6

Updated by Karsten Dambekalns over 11 years ago

  • Tracker changed from Bug to Task
#7

Updated by Karsten Dambekalns over 11 years ago

  • Status changed from Needs Feedback to Accepted
#8

Updated by Karsten Dambekalns over 11 years ago

  • Status changed from Accepted to Resolved
  • % Done changed from 0 to 100

Applied in changeset r4155.

Also available in: Atom PDF