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

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

Closed
tabacitu opened this issue Jul 10, 2024 · 6 comments
Closed

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

tabacitu opened this issue Jul 10, 2024 · 6 comments
Assignees
Labels

Comments

@tabacitu
Copy link
Member

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
Copy link
Member Author

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
Copy link

amenk commented Jul 11, 2024

@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
Copy link

amenk commented Jul 30, 2024

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

@tabacitu
Copy link
Member Author

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:

  • SOLUTION 1 is a "hack" that is not sustainable long-term;
  • SOLUTION 2 is not ok;
  • SOLUTION 3 might be the way to go here long-term; but I'm not 100% sure it's a complete solution either;

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
Copy link

amenk commented Aug 21, 2024

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.

@pxpm
Copy link
Contributor

pxpm commented Dec 4, 2024

Thanks for the suggestion @amenk

We are implementing SRI hashes for 3rd party content in backpack for the next version 👍

Will be closing this as most of the work is already done! Stay tuned 🥳

Cheers

@pxpm pxpm closed this as completed Dec 4, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in This week Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

3 participants