XSS Flaw

We also use Noggin for a couple of sites and would love to see it fixed. But I really would not know what to do more than following the described steps.

If there is a security issue with Noggin I have not been notified of it nor can I find a detailed description of the problem in the issue que. I use it on 4 sites. I am unable to find a replacement theme that gives a satisfactory visual design.

It's distressing to see that the security team is ignoring the problem and preventing community members from moving forward with assigning a new maintainer or to fix the problem. Distressing but not unexpected.

Could you point me at an explanation of the security problem? From the look of it, this issue is for the DEV branch. I'm not sure a development version problem should be allowed to kill the whole project.

I have running code but I will admit I had a struggle to find the correct version for MY sites.

I'd be glad to help fix it -- but I need to know just what the problem is.

@JamesOakley I see in the original post you mentioned contacting the team. Sorry if you never got a reply on that. I think there was a big number of inquiries that week and some were overlooked. If you want to become a co-maintainer of the module please be sure to follow the process for that.

Now that 2 months have passed since this was unsupported our policy is that it can be published, so here are the details:

----original report-------:

This module has a XSS vulnerability.

You can see this vulnerability by:
1. Enabling the module
2. As a user with the Administer themes (administer themes) permission go to the theme configuration form (/admin/appearance/settings/bartik)
3. Enter the following values and save the form:
CSS header selector
</style><script>alert('XSS Selector');</script>
Background color
<script>alert('XSS Background color');</script>
Header height
<script>alert('XSS Header height');</script>
Additional CSS attributes
<script>alert('XSS Attributes');</script>
4. Open the homepage. Multiple alerts will be displayed.

For the selector does check_plain make more sense or would something like drupal_clean_css_identifier https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_... make more sense?

check_plain blocks xss in some cases, whereas drupal_clean_css_identifier ensures it's a valid css identifier AND protects against XSS.

I think the patch looks pretty good.

I have one slight reservation: @JamesOakley said in #6

The exception is the image filename, but that has validation in place at entry time.

It's true that if a Custom image is to be used, the value for header_path is validated on form submit:

  // If the user provided a path for a logo or favicon file, make sure a file   
  // exists at that path.                                                          
  if ($form_state['values']['header_path']) {                                      
    $path = _system_theme_settings_validate_path($form_state['values']['header_path']);
    if (!$path) {                                                                  
      form_set_error('header_path', t('The custom header path is invalid.'));   
    }                                                                              
  }

However, as the comment suggests, _system_theme_settings_validate_path() is really just checking if a file exists at the path given.

It's possible to create files with dangerous names (depending on OS and filesystem etc..). For example:

drupal-7.x/sites/default/files$ ls *alert*
'<script>alert(123)'

...and that file path passes the existing validation.

AFAICS the functionality for supplying a custom image path is actually broken, so this doesn't lead to an exploit :) I couldn't get it to work with a path to an innocent image. I think there may be an issue with the position of the logic to handle header_path in noggin_theme_settings_submit() such that the noggin:header_path variable doesn't get set if you select "Custom image" and enter a path (without uploading a file). I could be wrong on that though.

Long story short; it looks to me like that user-submitted value should probably be sanitised as well (e.g. in case that functionality is subsequently fixed).

So did everyone just forget about this item?

It is continually giving me warnings AND TRYING TO GET ME TO UPDATE MY FILES. But there exists NO known alternative. Looking at this issue I can't see it as significant as you need administer privileges to change the theme. If you have administer privileges you can easily destroy he site. You font need complicated exploits to to that. So I can't see this as a real issue.

As I understand matters a patch has been developed and submitter for review and the SECURITY TEAM IS REFUSING TO REVIEW IT ? I mean a month passing buy and NO action? I could accept a couple of weeks but a month? Good grief! It has not even been addigned to anyone for review...

I may have to just rename the module to stop the errobious error messages flooding my mail box. I get so many I can't tell the real ones from the bogus ones.

@Bill Lea did you review the patch?

The patch is public (comment #9), and reviewed by mcdruid and set back to "needs work" (comment #10). JamesOakley responded to the review by disagreeing about it needing work ("I don't think this is actually a problem.") and setting it back to "needs review" (comments #11 & #12).

I haven't made a stab at reviewing it myself, as such a review would not be of much value. I am not a member of the security team, and does not necessarily have the qualifications to review such a complex security issue.

I may be wrong, but to me, it looks like the security team is still waiting for JamesOakley to provide a new patch addressing the concerns expressed in comment #10, while JamesOakley is still waiting for the security team to conduct a new review of the patch in comment #9, taking into account his objections to the previous review.

But that is just me speculating. I think it would be nice if both the security team and JamesOakley could state explicitly what they think is blocking this from proceeding, and what they think can be done to resolve it.

JamesOakley,
you're right, the one to review is in #12. My apologies for overlooking that.

Here is my two cents worth. Quoting from #16:

I investigate this, and discover that the filename is sanitised at output time, so this is not actually an issue.

While this is true, this is IMHO not a recommended security practice. The recommended practice, AFAIK, it to always leave input as-is, and in the case of an XSS vulnerability: sanitize the output.

I leave it to the security team to make the call on this (so I am not pushing it back to "needs work"), but my recommendation would be to rework the patch and make sure that filename from untrusted input is stored as-is, and then ensure that the output is sanitized.

Actually, my bad – it says "output". Second gaffe this morning.

Note to self: Coffee before typing anything.

Gentlemen:

I have used NOGGIN for many years with NO Problem.

No I have not run the tests suggested because IMHO the security problem is moot. I believe this because not everyone should have access to the theme layer of a site. It should only be available to a trusted role or to a system admin. Neither of these folks should be willing to destroy their own site. In the case of a large firm the IT department should remove access to a disgruntled employee before firing. The whole thing is moot.

I also believe that a site admin should have the ability of override the security team when they overstep their mandate and propose a moot flaw. I ma a cem engineer and all our alarm system have an acknowledge button to turn off alarms while they are dealt with. Even in cases where failure to deal with the alarm will destroy equipment and potentially kill people. Drupal should have the same sort of provision.

The problem with NOGGIN that is REAL is that it will not properly respect the aspect ratio of the header image. In the full scale Image I use the header needs to be 390px. I have e to set this and It is fixed even when the width is reduced to the iPhone width. I haven't dealt with this because I really don't care to try and show my images of such a small screen.

I note also that it's been said that the file path to the headers is broken. But I haven't verified this. I can select from any number of header images as long as they are in the right file folder. I'll see if this is a real problem but I don't think it is a thing affecting the version I am running.

I have found that some versions of NOGGIN which were published did not work and I have gone to the trouble of using an old version that does work - AT LEAST FOR ME. I note that this version is no longer listed on the noggin page under versions and I am not sure why it was removed. I only know that newer versions did not work with Pixturereloded theme and I settled on the one tat provided some functionality.

I was going to wait till you finished fiddling with the module to test it. But maybe I should compare my current version with your current version with its patch. Bur I will probably only look for fixes to the aspect ratio problem and the "broken file path" problem.

Even with all his work Mr Oakley is still not acknowledged as a maintainer and the module is still listed as abandoned and unavailable for download. At least on my sites status report. I will have to check on the noggin page if it still exists.

My appologies if I seem crankey or upset but how long can this take?

I went into the code today and I believe I have fixed the problem with the file path not being recognized. It was never saved as a variable.
So I suppose it needs some sort of sanitation.

I have not yet looked at the sanitation in detail. Its next on my agenda. You should note that I am using the code 7.x-1.1+4-dev dated datestamp = "1487558592".

. All the image functions are now working. I will look into the patches proposed by Mr Oakley later tonight. I'm hazy on making patches so I am attaching the entire revised module file if any one is interested in a work in progress.
My plan at the moment is to apply Mr Oakley's proposed patches by hand and then see if the code still runs.

Drat forgot to attqach the file.

so there is is a problem that has not been addressed heretofore in this discussion. There are TWO branches fof the nogin module. The main brance and the dev branch. jef Burns stated that only the dev branch will work with his theme pixturereloaded and that other users should continue to use the main branch. I use jeff's theme pixturereloaded. Thus I am working with the dev code. Mr Oakley's patch fails when run against the dev branch. Of course I just may be doing it wrong. As I said I am hazy about patching. It's made more difficult since the GIT repository does not contain a dev branch so I'm not sure howto create one correctly. I have all the files. reading the patch file does not really enlighten me as to the changes needed. As I said before, the module file I uploaded is fully functional on my system with the exception of the xxs changes. It will probably need its own separate patch and there will also need to be TWO versions of the noggin module maintained if the min branch is indeed incomparable with pixturereloaded and the dev is indeed incomparable with other themes. it should be noted that I am taking jeff's word on this incompatibility I have not tested it by trying to run the main branch under my site. I may look at that later in the week.

I have been looking at the downloadable release and the dev release. The dev release works well with the pixture reloaded theme while the 7.1.1 release does not. They appear to be quite different sets of code. I have found the problem with the path not working. it needs to be saved in the dev code. I haven't found that to be the case in the 7.1.1 code.

What theme are you running? Is it availiable freely so that I could test it?

My struggles with GIT are probably outside the scope of this chat but it makes if difficult for me to patch things. I can not reliably apply a patch nor generate one. I will try your suggestion. I put the patch in the repository and used the v option. The patch failed when I tried to run it. I will re-clone and try again.

Today I examined the behavior of the two versions. I tested the theme performance against Bartek and Garland the dev seemed to work although the results were visually distressing to me. The menu appeared in the center of the header image.

I will tyr patching it again both the 7.1.1 and the dev and see what the result is. As far as I can tell I have fixed the path problem with the dev version - they simply forgot to save it. I will also look and see if I can figure out what you did to the release and do likewise to the dev version.

Thanks for the prompt reply.

James:

I have successfully applied your patch 12 to the module. I tested it with the four given criterion and it appears to pass. At least no warning or alert messages appear. My earlier problems were due to the fact that I had cloned the wrong code and was trying to apply patch 9. Sorry.
I do flail about when I do this, which is one reason I don't often engage in maintenance activities.

In the matter of the path not working I found that it was not being saved. I added a variable_set statement and that problem goes away. I am not so sure the logic of the data input is clear. If you have a entry in the path you can still upload an image and the file path is not cleared. If the file path is cleared and no file is uploaded a blank header background is displayed. This is just annoying behavior and will not stop the program from being usable.

I am attaching a patch with my fix to the header path problem for your review. I guess that is appropriate.
I first applied your patch 12 and then fixed the path problem. It's a simple fix. Can't speak as to the other problems.

What I can not understand is the length of time it's taking the Drupal guys to approve you as the maintainer of this module. It's excessive even for a volunteer organization.

Thank you for your perseverance everyone.

@JamesOakley re. #11 and #12 yes, I think you're correct that in most cases the value of a custom header image will be escaped / encoded by file_create_url().

You can manipulate your way around that somewhat, but probably not in a way that opens up an obvious exploit.

For example (this is with noggin 7.x-1.x as it currently stands, and a clean vanilla install of D7.77):

$ drush -y site-install && drush -y en noggin

$ drush vset --format=json theme_settings '{"header_image":"custom"}'
theme_settings was set to
array (
  'header_image' => 'custom',
).

$ drush vset 'noggin:header_path' '/<script>alert(123)'
noggin:header_path was set to "/<script>alert(123)".

$ drush ev '$vars = array(); noggin_preprocess_page($vars); print_r(drupal_add_css()[0]);'
Array
(
    [type] => inline
    [group] => 100
    [weight] => 99.013
    [every_page] =>
    [media] => all
    [preprocess] => 1
    [data] =>  {background:url('/<script>alert(123)')     ;height:auto;-o-background-size: auto;-webkit-background-size: auto;-khtml-background-size: auto;-moz-background-size: auto;background-size: auto;background-origin:border-box;}
    [browsers] => Array
        (
            [IE] => 1
            [!IE] => 1
        )

)

As mentioned, there are a couple of reasons why this may not be a big problem; first if there's validation elsewhere that the header_path variable matches an existing file, prepending the path with a slash will likely make that fail (I've not tested this though).

Also, by default the inline CSS here is output as CDATA e.g.:

<style type="text/css" media="all">
<!--/*--><![CDATA[/*><!--*/
{background:url('/<script>alert(123)');height:auto;-o-background-size:auto;-webkit-background-size:auto;-khtml-background-size:auto;-moz-background-size:auto;background-size:auto;background-origin:border-box;}

/*]]>*/-->

So that particular value (probably) won't result in the browser running the JS.

There may be other ways to get this to work though; I do still wonder whether the value should be directly run through something like filter_xss() for good measure.

OK. AFAIK the patches work and produce no alerts.
I was not familiar with the coordination procedure so I will make the update.

I am confused by Mcdruid's last post. I do not use DRUSH for anything therefore I am not sure what he is doing. All my testing iis done thru the theme management input forms.

My test site does not contain any customization other than a change I made to file_field_paths so it would run with my ZIFIMAGE module. Actually I do not have ZIFIMAGE switched on at this time. I can't see how my site configuration would be a factor. I run pixture_reloaded in a vanilla state. I have looked at other themes and and as stated in the Noggin readme they do not seem to support the header height spec. Garland, Bartik, and Best_Responsive do not respect the header height value. This puts the menus in the interior of the header image .

I can't get past the requirement to have theme level access permission to access these forms. I believe that is protection enough. Is DRUSH applicable of bypassing the security system? Theme level access is not something you would give to just anyone. Indeed, to use some aspects you would need SSH/FTP access to thew HOST file system. If you have THAT you can install any malware you want and no sanitation will be effective.

JamesOakley,
if somebody untrusted has access to the CLI (including drush), they've got all the keys to the kingdom. You cannot defend against that. You just have to make sure nobody you don't trust can access the CLI.

However, what I believe McDruid is doing here is to use drush as a shortcut to get the exploit into the database. Whether this is doable without the use of drush is moot. To determine that one way or another would require tons of static code analysis and white box testing – probably a lot more work than anybody here would be able to commit to.

Instead, goes the reasoning: Let us just assume that somebody discovers a way to get the exploit into the database. Is the code still secure? Thinking like that, and then finding a way to make the project secure even if an exploit exists in the database, is basically good housekeeping. I think you should go along with that type of reasoning, rather than speculating about whether getting the exploit into the database is a possibility.

The way I read his comment #28, he says that the preprocess funcition (noggin_preprocess_page()) will produce an unescaped background:url('/<script>alert(123)');. He then concludes that while this probably cannot be exploited – but …

There may be other ways to get this to work though; I do still wonder whether the value should be directly run through something like filter_xss() for good measure.

My interpretation of that statement is that he thinks that we cannot be 100% certain that file_create_url() will provide the necessary sanitaszion in all possible use cases where this data may be output. While using filter_xss() to explicitly sanitize this data may be unecessary, as we do not have the imagination to foresee how the project will be used, including its use by other site builders. Therefore, providing an extra layer of security to safeguard the data is recommended.

In #33 @gisle's interpretation of the point I was trying to make is spot on.

Forget about drush; I was just using it as a quick way to set a D7 site up for testing; you could achieve almost all of the same things by clicking around in the UI, but enumerating / following every step of that process is more onerous than using a couple of drush commands.

we cannot be 100% certain that file_create_url() will provide the necessary sanitaszion in all possible ... cases

Yup, exactly; if an attacker can get a malicious value into the noggin:header_path variable, and the value begins with a slash then file_create_url() will not escape / encode that value and it will be output without any sanitisation.

You might say that's not likely to happen, and that's probably correct. IIRC there's validation in the admin UI which checks that header_path with file_exists() so perhaps that wouldn't allow values beginning with a slash. However, there's no guarantee that functionality won't be removed / altered during subsequent development of the module or by custom theming / coding on a site etc.. Does it work exactly the same way on Windows servers?

Perhaps it seems unlikely to the point of being implausible that this could ever be exploited, but there's that saying when it comes to security that "the attacker only has to win once".

@JamesOakley is right that file_create_url() will almost certainly encode this value such that it's safe, but I don't think that's guaranteed to be always be the case in every conceivable circumstance.

Hence why I think we may as well run the value through a sanitisation function which is designed for that specific purpose and is guaranteed to produce predictably safe results. Doing so ought to add very little cost, and should provide assurance that this functionality cannot be abused.

Oh, sorry - I didn't intentionally change the status, but I suppose it's appropriate for me to "un-RTBC" given the comment.

Pretty sure you'd need file_create_url() plus a sanitisation function.

Thanks for your patience.

@JamesOakley pinged me to say he's prepared a 7.x-1.2 tag.

I've reviewed that and am happy that it fixes the reported XSS issue that led to this project being marked as unsupported.

I'm making this issue as Fixed, and we should then be able to get the module restored to security coverage with a new release.

I applied the revised Noggin module to my test site today. It no longer works. Noggin will no longer recognition the uploaded files in the header-files folder. You can see them select them but It will not let you USE them. The module is for all intents and purposes UNUSABLE in its present t state. I don't know What you did to the file naming process but it appears to have BROKEN it. Please fix it properly.

I am continuing to test the modified noggin code. I have reverted to a known working code version and have located one problem.
when you attempt to sanitize the color attribute one of the valid color inputs is #FFFFFF this is rendered as FFFFFF which seems to break the system.

I will continue to try and add your sanitation functions to my working code one by one till I see if anything else breaks the code. I will also see if removing the sanitation from your code will fix the system.

I note in passing that it appears the path is not saved and thus will remain broken in this code. Also I am working full time on this because I want to get to the bottom on it and I do have the time. I will keep you posted on my results

The software continues to jumble my site. I use a large image so I need to be able to set the header height to say '426px'' the current code
somehow doe not respect this height. That is the header is expanded and but it is then overlayed by the menus content and sidebars.
It's just a big jumble.

Do you intend to fix the path problem, or just delete that functionality?

Thanks for your reply. It is a pleasure to work with a maintainer who responds so quickly and thoroughly.

Your proposed scheme is fine with me.

You should know, however, that I have what is now a forked version of noiggin running on my sites. I picked up most of the changes you made and fixed the path problem. It works but is probably much different from your code. If you wish Ill send you a copy or put it up on github. All of my thinking on noggin is blogged on williamglea.com. At the current time I am considering whether or not it is worth the effort to fix the image management file system. My thoughts about that are blogged on williamglea.com as well. No need to spam the site here

I recently tried 7.1.3 and it to does not respect the header height specification. I have no idea why. Sinbce I have working code I have not spent the time needed to debug it. I'm going to be distracted for thte next few weeks so I might be slow to respond.
But I'd line to express my appreciation for your efforts on behalf of the NOGGIN users.

JamesOakley,
please don't change the status of an issue from "Fixed" to "Closed (fixed)".

An issue with the status "Fixed" will be automatically closed as fixed and removed from the list of open issues after 14 days. It should remain open for 14 days after being "Fixed" to cater for regressions and other unintended consequences of the fix. Source: Issue Status Field.

In general, as a project maintainer here, you may want learn about Issue Etiquette.

Sigh:
I think you are wrong.

The 7.x-1.0 branch has been known not to work with pixture reloaded since 2017. I have had NO problems with it that would require me to fiddle with the theme code.

Here is a patch based on the DEV branch that in the words of Ted Howard "Just works".
Pleas check it out.

I known not what course others may take but I will use this code.