NOTE: This was reported to the security team and I was advised to post it here
The description value of installation profiles is not run through filter_xss_admin() which makes it possible for a coder of questionable morals to be able to automagically select their installation profile. The attached example code will do this, to test it, drop it into profiles/nomorals/nomorals.profile of a D6 platform and try install install a new site.
I know this is a relatively minor issue, but I think it should be fixed, especially given the growth in installation profiles for Drupal.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 938156-profile-description-filter-d7-5.diff | 738 bytes | skwashd |
| #2 | 938156-profile-description-filter.diff | 692 bytes | skwashd |
| nomorals.profile.txt | 820 bytes | skwashd |
Comments
Comment #1
damien tournoud commentedThis probably applies to Drupal 7 as well.
Comment #2
skwashd commentedThe attached patch, against latest 6 cvs, fixes the problem. I tested it with a few different installation profiles and it didn't seem to break anything, including Acquia's embedding of an image in the description field for Drupal Commons. The fix has the added advantage of showing all users if a developer tries to exploit this vulnerability.
Comment #3
damien tournoud commentedComment #5
skwashd commentedThis version of the patch is for D7. The previously attached version is for D6, which is why it failed.
Comment #6
skwashd commentedI had a quick look at the Drupal 7 test docs and couldn't find any info on how to write a test for the installer, sorry if I missed it.
Comment #7
gregglesIs there really a good reason to fix this?
If someone can put javascript in your profile description they can also put php into the .profile file. In general: people should only run .profiles which they trust.
Comment #8
skwashd commentedI agree that people should only run profiles they trust, but not everyone who downloads something from the net knows what to look for. I am sure there will be at least 1 developer who thinks it is a "cool hack" to do something similar to the example code.
There has been growing interest in Drupal distros and installation profiles over the last year or so. The list of installation profiles available from d.o is steadily increasing, and I suspect D7 will accelerate this trend. At least one module had "dial home" js injected into it, I am sure many people trusted that module too.
I think that there is a general perception that if code is downloaded from d.o it can be trusted. The people who have this attitude are unlikely to have the skills to know what to look for. Yes, it is possible to exploit a server using PHP code far more effectively than this JS code, but I believe that we should reduce the number of attack vectors. I don't think forcing an installation profile on a user is a very good introduction.
If the issue was a module "automagically" installing itself via a similar hole in the module management page be considered not worth fixing? Once again any code can be put into a module, released from d.o and installed by a user.
Comment #9
gregglesBut if we remove the ability to do dial home js, someone will just use dial home php.
(BTW, are you describing kaltura or was there another?)
And yes, IMO, xss in the module name is not really worth fixing either other than to reduce false "I found xss" reports/fud.
So I guess I'm OK with us fixing this but we should be clear that it doesn't really make anyone magically safer.
Comment #10
skwashd commented@greggles I know it is very low hanging fruit. I know that there are many other ways to exploit the Drupal installation process. I'm not saying this will magically make the installer safe - but it does make one aspect of it a little bit safer.
I was referencing kaltura, but I didn't mention it by name because we have no way of knowing how many other modules are there out in the wild doing similar things.
If there is a strong feeling from more senior people in the community that this can be ignored, I will accept their decision.
Comment #11
moshe weitzman commentedI'm fine with fixing this, but it really is absurd. You have already run their php code. Trust has already been accepted. It doesn't make anything safer.
Comment #12
heine commentedSense this does not make. Indeed.