Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
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
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
| 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 CreditAttribution: willzyx commentedFirst, tests should fail. Add some real world cases to MachineNameControllerTest::providerTestMachineNameController() is a good idea
Comment #3
willzyx CreditAttribution: willzyx commentedwrong branch..
Comment #5
cilefen CreditAttribution: cilefen commentedComment #6
cilefen CreditAttribution: cilefen commentedComment #7
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedAh, hmm - so we should not have used preg_quote() perhaps, but just have quoted the pattern delimiter?
Comment #8
pwolanin CreditAttribution: pwolanin as a volunteer commentedI think this would fix the bug?
Comment #9
willzyx CreditAttribution: 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 CreditAttribution: pwolanin as a volunteer commentedPossibly a more robust fix, plus one more test case.
Comment #12
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting 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 CreditAttribution: pwolanin as a volunteer 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 CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting 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 CreditAttribution: pwolanin as a volunteer commentedI added one with "@" already.
Comment #16
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedNo, I mean for the replace_pattern ...
Comment #17
willzyx CreditAttribution: willzyx commentedAdded a test with \e and a test with @ in replace_pattern.
Comment #18
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, that should be fine for now.
Comment #19
pwolanin CreditAttribution: pwolanin as a volunteer commentedhmm, no - the \0 is in single quotes, so it's not actually a null byte in the pattern.
Comment #20
willzyx CreditAttribution: 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 CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting 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 CreditAttribution: pwolanin as a volunteer 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 CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting 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 CreditAttribution: pwolanin as a volunteer commentedtrying harder to verify code is not called - the bogus call
fail()would fatal.Comment #25
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting 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.