Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross-site scripting vulnerability #1297

Closed
Nohac opened this issue Mar 22, 2018 · 9 comments
Closed

Cross-site scripting vulnerability #1297

Nohac opened this issue Mar 22, 2018 · 9 comments
Labels

Comments

@Nohac
Copy link

Nohac commented Mar 22, 2018

Bug report

Relational columns (select) does not escape html which allows scripts to be executed!
https://github.com/Laravel-Backpack/CRUD/blob/master/src/resources/views/columns/select.blade.php

What I did:

$this->crud->setColumnDetails('team_id', [
    'label' => 'Team',
    'type' => "select",
    'name' => 'team_id',
    'entity' => 'team',
    'attribute' => "name",
    'model' => "App\Team",
]);

What I expected to happen:

The column should escape all html, and render it as text.

What happened:

If the team name contains html and javascript, e.g:

<script>alert('pwnd')</script>

Then the alert will pop up if that column is loaded.

What I've already tried to fix it:

src/resources/views/columns/select.blade.php

<span>
    <?php
        $attributes = $crud->getModelAttributeFromRelation($entry, $column['entity'], $column['attribute']);
        if (count($attributes)) {
-            echo implode(', ', $attributes);
+            echo htmlspecialchars(implode(', ', $attributes));
        } else {
            echo '-';
        }
    ?>
</span>

Backpack, Laravel, PHP, DB version:

{
    "backpack/base": "^0.9.0",
    "backpack/crud": "^3.4",
    "laravel/framework": "5.5.*"
}
PHP 7.2.3 (cli) (built: Mar  1 2018 16:50:09) ( NTS )
5.7.21 - MySQL Community Server (GPL) 
@welcome
Copy link

welcome bot commented Mar 22, 2018

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication mediums:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

@tabacitu
Copy link
Member

Hi @noh4ck ,

Thanks for revealing this.

I do agree with the small change, since I don't see anybody using the name to store HTML.

But IMHO, the <script> should have no way to get into the db in the first place. And Laravel already takes care of that. All CRUD forms use the standard Laravel validation using Form Requests, which cleans the input. If scripts can get into the db, then you'll find a bunch of attributes there are echoed as-is. Think $entry->content or things like that. Sure, your solution helps too - never hurts to be cautious. But IMHO, a better solution is not to have those scripts in the DB in the first place. And I think we already have that going for us :-) How did you get your <script> in the db, in your test?

Cheers!

PS. I may be wrong on "best practice in Laravel" here. I know developers are always devided about XSS - wether it's best to filter input or output. So I expect this to be a controversial topic :-) Brace yourselves! :-)

@Nohac
Copy link
Author

Nohac commented Mar 23, 2018

@tabacitu The XSS primarily comes in via a frontend/website, not trough the backpack panel (that seems to not matter btw. writing html directly in backpack does the same thing). As far as I know, when it comes to XSS best practices in laravel, the only way XSS can be a problem is if you use {!! ... !!} in your blade template (https://laravel.com/docs/5.6/blade > Displaying Unescaped Data) the same goes for other frameworks like React, where the only way to render html is to use dangerouslySetInnerHTML, which is why I don't escape html on input, because this should not be a problem unless one of these two methods is used.

As I mentioned in my bug report, this only happens when using select in columns, displaying the value directly (in the Team crud in my situation), it works as expected. The solution I proposed was not meant to be a fix, just to highlight the problem.

@tabacitu
Copy link
Member

Got it, thank you! I think the best option here would be to use the e() helper. Since Laravel has it, why not use it? :-)

Will follow up with a PR in a few minutes.

@tabacitu
Copy link
Member

Fixed in 8b6bd0a

Will tag later today. Thank you @noh4ck !

@mgralikowski
Copy link
Contributor

Hi,

how about make it optional? For example i would like to add column like phpmyadmin has to handle foreign keys.

        $this->crud->addColumn([
            'label' => "Added by",
            'type' => "select",
            'name' => 'creator_id', 
            'entity' => 'creator',
            'attribute' => "admin_link",
            'model' => "App\Models\Creator",
        ]);

Ale User (creator) model:

    public function getAdminLinksAttribute()
    {
        return '<a href="'. route('crud.user.show', $this) .'" class="btn btn-xs btn-default" target="_blank"><i class="fa fa-arrow-circle-up"></i> Show creator in admin panel</a>';
    }

@tabacitu
Copy link
Member

@mgralikowski take a look at the model_function and model_function_attribute columns - we created them specifically for this need.

@mgralikowski
Copy link
Contributor

mgralikowski commented Jul 19, 2018

Yes, i know this functions and use in project but in this case.. Let's try:

        $this->crud->addColumn([
            'label' => "Added by",
            'type' => "model_function",
            'function_name' => "getCreatorLinks",
            'limit' => 500
        ]);
    public function getCreatorLinks()
    {
        return $this->creator ? '<a href="'. route('crud.user.show', $this->creator->id) .'" class="btn btn-xs btn-default" target="_blank"><i class="fa fa-arrow-circle-up"></i> Show him</a>'  : '-';
    }
  1. The most important - this version call select query for every row (N + 1 query problem)
  2. I need to add separate methods to all models with user/creator (or use Trait or BaseModel)
  3. I need add limit attribute to prevent breaking my html code
  4. More code (need to check relation exists and i need to use $this->{entityName}->id vs simply $this->id where $this is User model

Suggestions?

@tabacitu
Copy link
Member

@mgralikowski I think I understand what you're saying - it's pretty inconvenient, I agree. We might be able to create a series of different column types for links, but there would be just as many as the select column types, since there are a bunch of different use cases here: no relationship, 1-1 relationship, 1-n relationship, n-n relationship. In each of those cases, the process of getting the TEXT to show in the link and the LINK would be different. But here's the kicker, and why we haven't created them so far: you, as a developer, would still need to create a link accessor on your model. Otherwise there's no way to know what the link is. So our logic so far has been that, since you're already creating an accessor, why complicate things with a bunch of column types, when you can have total customization by writing them in your accessor.

If I'm wrong and you see a different way to do this, please open a separate issue, we've been going further away from the topic here.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants