search

Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.04k stars 885 forks source link

[Improvement] Add integrity tags to all 3rd party assets #5558

Open tabacitu opened 1 month ago

tabacitu commented 1 month ago

A user just suggested on email that we should add integrity tags to our assets.

I think it's a small change that would go a long way towards preventing some attacks. A bang-for-buck change.

Especially now that supply chain attacks are more common (see polyfill incident in July 2024), we can do a little more to prevent this kind of attacks.

I suggest we go through all our repos, one by one, and add integrity tags. Afaik Basset already supports it, it's just a matter of actually copy-pasting them to our code as well.

What do you guys think, isn't this an easy and impactful thing we can finish by the end of the month?

tabacitu commented 1 month ago

This would also be a good opportunity for us to bump the dependency versions. It's been a while since we last did that.

amenk commented 1 month ago

@tabacitu thank you for opening the issue based on my email.

In addition to singing the includes I still woud appreciate if the old behavior could still be used via some kind of switch.

It is harder to review and ship such CDN contents than locally build packages.

amenk commented 3 weeks ago

@tabacitu What's the status on this? This blocks us from upgrading

tabacitu commented 1 day ago

Thanks for the reminder @amenk !

In addition to singing the includes I still woud appreciate if the old behavior could still be used via some kind of switch.

We do not plan on adding back the old behaviour - that would consider a HUGE breaking change, and a step back for us.

It is harder to review and ship such CDN contents than locally build packages.

I agree - we did plan a v2 for Basset that included importmaps and integrity checks, but we never got around to it.

Regarding actually implementing SRI in Basset, here are the possible solutions we are considering:

SOLUTION 1. Add the integrity hashes to all our assets, manually. For example instead of:

 @basset('https://cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css')

we can do:

 @basset('https://cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css', true, [
    'integrity' => 'GADhaOJCr6lsUqdHJnYcH/QaARzVT92beGzAYxLTSoxUorHjQZci1FW+X9BqbnE3%',
    'crossorigin' => 'anonymous',
])

This will output:

<link href="/storage/basset/cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css?187e21de716e"
      rel="stylesheet"
      type="text/css"
      integrity="GADhaOJCr6lsUqdHJnYcH/QaARzVT92beGzAYxLTSoxUorHjQZci1FW+X9BqbnE3%"
      crossorigin="anonymous" />

THE PROBLEM with this solution is that it's manual. It requires us to MANUALLY add the hash for each asset we use. And to update it, every time we update the asset. This isn't sustainable. And it would 100% break, when loading local assets from userland, that the developer can change (eg. /storage/basset/vendor/backpack/theme-tabler/resources/assets/css/colors.css) - but one could argue we shouldn't be using integrity tags on those anyway, they're local 🤔

Then again... we can speed this up by using a service like https://www.srihash.org/ that generates the hash... so it wouldn't be THAT much of a trouble the first time we do it. It would still be unsustainable long-term... but not a HUGE deal.

SOLUTION 2. NOT AN ACTUAL OPTION. We could generate a hash on-the-fly, when basset() is run and the asset internalized. We could use PHP to get a hash of that asset and use that as the integrity tag. BUT. That defeats the purpose of SRI. SRI is supposed to cover file integrity at the source. We would be "fetching anything from the CDN". Then adding "fake" integrity tags. No bueno.

SOLUTION 3. We could modify the php artisan basset:cache command to also get the hash and store it in the basset cache file (easily done with something like curl -s https://cdn.datatables.net/1.13.1/css/dataTables.bootstrap5.min.css | openssl dgst -sha384 -binary | openssl base64 -A). Then when basset() is called on an asset... basset would check if the hash is present in the cache file... and if so, use that hash as an integrity tag. The idea here is to basically not only store the URL in the basset cache file... but also the hash attribute, and use it whenever someone does basset() for that asset. This would be better than SOLUTION 2 because it doesn't happen on runtime. It happens on deployment. So yes, if an asset has been compromised at the source, between deployment and pageload... the SRI tag will be useful. But... what if the asset has been compromised before deployment? 🤷‍♂️

I would say:

That being said, SOLUTION 3 doesn't render SOLUTION 1 worthless. In fact, for the assets where the vendor has published a has in the docs page, it's preferrable to have that hash written along with the URL. Then Basset won't have to generate a hash for it, we just test against it.

What do you think @amenk ? Do you see SOLUTION 1 or SOLUTION 3 as being rock-solid? If not, any other solution you have in mind?

amenk commented 1 day ago

I actually see SOLUTION 1 as being quite okay. That's what I had in mind.

Yes it's manual - but that's the whole point: We have to consciously choose a version, eventually do a short review for known issues and then calculate the hash.

And such updates don't seem to happen so often in backpack - so this should be fine. (see https://github.com/Laravel-Backpack/CRUD/blame/db386bffdb55584ea2ed6dc738bb7aa771420828/src/resources/views/crud/list.blade.php#L152)

We can always add a artisan scaffolding tool

php artisan basset:generate-tag https://example.com/foo.js which generates the proper @basset() tag including SRI, later to simplify the process when adding new resources.

but one could argue we shouldn't be using integrity tags on those anyway, they're local 🤔

yes, I would say so - this is a much narrower attack vector. I think SRI is mostly meant to 3rd party stuff.