After #2508735: Code injection via preg_replace() the special characters in machine name input field are not longer replaced properly.
The preg_quote() addition in MachineNameController::transliterate() seems to create some troubles with the regular expressions and prevents preg_replace() to do its job.
If you fill the name field with a dirty string the machine name this is the result
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | increment.txt | 1.13 KB | pwolanin |
| #24 | 2525870-24.patch | 2.38 KB | pwolanin |
| #20 | 2525870-20.patch | 2.23 KB | willzyx |
| #20 | interdiff.txt | 1.87 KB | willzyx |
| #17 | interdiff.txt | 1.78 KB | willzyx |
Comments
Comment #1
willzyx commentedFirst, tests should fail. Add some real world cases to MachineNameControllerTest::providerTestMachineNameController() is a good idea
Comment #3
willzyx commentedwrong branch..
Comment #5
cilefen commentedComment #6
cilefen commentedComment #7
pwolanin commentedAh, hmm - so we should not have used preg_quote() perhaps, but just have quoted the pattern delimiter?
Comment #8
pwolanin commentedI think this would fix the bug?
Comment #9
willzyx commentedoh probably it does but I think we need to improve a bit the test and add more real-world cases
Comment #11
pwolanin commentedPossibly a more robust fix, plus one more test case.
Comment #12
fabianx commentedWhy do we allow user-input for that anyway?
Is there not just a fixed list of possible patterns that could be managed by a pattern manager or such?
Comment #13
pwolanin commented@Fabianx - I'm not sure - it doess seem overly flexible. So I think we need to fix the regression here quickly and then actually fix the code better.
Comment #14
fabianx commentedCould we at least add some test cases using '@' and '\0' to the list?
It is making me nervous to change a security critical bug to something else without testing that it regresses.
Comment #15
pwolanin commentedI added one with "@" already.
Comment #16
fabianx commentedNo, I mean for the replace_pattern ...
Comment #17
willzyx commentedAdded a test with \e and a test with @ in replace_pattern.
Comment #18
fabianx commentedRTBC, that should be fine for now.
Comment #19
pwolanin commentedhmm, no - the \0 is in single quotes, so it's not actually a null byte in the pattern.
Comment #20
willzyx commentedThe tests that I added for null byte in #17 was wrong. The null byte should be in replace_pattern, not in text.
Added some comments and taken in account #17.
Comment #21
fabianx commented#20: Note, testing those is very very fast, I would rather have a few more lines added here, than to just add 3 new ones.
I think you can just merge your new examples with the old patch.
Comment #22
pwolanin commentedThe null byte should indeed be in the replace pattern to simulate an attack. It's not clear to me that that added test case would alert us to the vuln, however.
Trying to test locally, I actually see that I can't make it fail on 5.4, since PHP added protection against NULL bytes in the regex pattern in 5.4 itself. e.g. see:
https://github.com/php/php-src/blob/PHP-5.4/ext/pcre/tests/null_bytes.phpt
http://stackoverflow.com/questions/21842773/php-is-this-a-safe-way-to-al...
Comment #23
fabianx commentedI found in the SO link:
"The null-byte protection actually is necessary. PHP only checks for null bytes before the ending delimiter. – Xeoncross Feb 18 '14 at 17:06"
Comment #24
pwolanin commentedtrying harder to verify code is not called - the bogus call
fail()would fatal.Comment #25
fabianx commentedRTBC - if green.
Comment #26
webchickNice catch. Thanks for the expanded test coverage, too!
Committed and pushed to 8.0.x. Thanks!
Comment #29
xjmThe patch for this issue wasn't actually committed -- https://www.drupal.org/commitlog/commit/2/227f243a901d12200ed69cf7470471... is actually the patch from #2527638: MigrateDrupal7TestBase never installs Drupal 7 migrations a second time. Oops! I've committed and pushed #24 now for realsies.
Comment #30
xjmEr.