alpinejs / alpine

A rugged, minimal framework for composing JavaScript behavior in your markup.
https://alpinejs.dev
MIT License
27.78k stars 1.21k forks source link

Improve Security Awareness #237

Closed WebReflection closed 4 years ago

WebReflection commented 4 years ago
for (const el of document.querySelectorAll('*')) {
  const attributes = [...el.attributes].filter(attr => '@click' === attr.name);
  if (attributes.length)
    attributes[0].value = 'alert("seriously? in 2020?")';
}

You can test it in codepen, and the attack regards any script that might be injected in a page that enabled evaluation for alpine.

I think the architecture behind alpine relies entirely on evaluation and deprecated ECMAScript features, and I am a libraries author myself, but I think it's about the time security becomes a major Web concern.

I am not sure how to fix this, as with(...) statement and evaluation of only the right thing would blow the size of this library, as it would require a proper AST crawling per each attribute content, but I hope this issue would be at least documented properly on top of the README page, as it's way too easy to inject arbitrary code when this library is trusted, and around.

Thanks for making the Web a better place.

SimoTod commented 4 years ago

@WebReflection

I may be missing something but what the difference between your example and

<div>
    <button>Open Dropdown (whatever lib we use)</button>
</div>
<script> <!-- type="evil" -->
for (const el of document.querySelectorAll('button')) {
  el.addEventListener("click", function() {
    alert("seriously? in 2020?")
  })
}
</script>

The attack you describe assumes you can inject random code in the page. If you have that power nobody will stop you from doing more sophisticated things, right? Shouldn't the exploit describe how you get to that point? Maybe using input with model binding on stuff like that.

WebReflection commented 4 years ago

The attack you describe assumes you can inject random code in the page.

That's how every page that enables CSP evaluation is at risk indeed, any third party code would be able to do whatever they want.

However, the issue here is that alpine must be trusted, to run evaluated code, but it cannot guarantee the evaluated code comes from alpine itself.

That means that alpine enables an indirect evaluation channel through its privileges, as trusted library, and that's not really what CSP is about, right?

If you have that power nobody will stop you from doing more sophisticated things, right?

Yes, if you have scripts from 3rd party you've no idea about, nobody is safe when it comes to clicks, I completely agree, but here the scenario I am wondering about:

Can you see that alipne here is responsible for evaluating code that doesn't belong to it?

Shouldn't the exploit describe how you get to that point?

My third parts tracking script check for these kind of attributes and augment their code to provide more tracking while the user is surfing ... you wouldn't do that with everything clickable, you would do that with reactive components, because these move data and actions around, links just "fan out".

On top of that, these scripts might decide to change the behavior of the UI, which is not something you can do just adding extra listeners, as you need to be hooked within alpine logic, properties, and evaluation stack.

So, as starting point, alpine exposes its internal behavior through a likely private scoped evaluation, and from that point on, it's big party for undesired scripts, as these would have access to the whole exposed internal stack.

Is it really difficult to see that using publicly modifiable attributes as your evaluation channel is something this library really should not do?

WebReflection commented 4 years ago

Another example, ads.js knows that the site is tracking links to ads and the reactive nature of alpine is in charge of updating stats behind the scene ... ads.js searches for special @click attributes and changes their behavior ... does the host know about what ads.js does? not at all, it's in charge of showing ads.

Same goes with shopping charts, a malicious script wants you to fail only of you click a specific item, and the whole logic is hooked within alpine ... selectItem = false as attribute value, and nothing would ever happen.

In few words, in a Web world where "first come, first serve", in terms of listeners, alpine is enabling any library to make the site look like it's acting normally, hence unobtrusive, while potentially taking over the whole logic.

These are just few of the reasons the Web doesn't like eval and Function, where arguably the latter one is safer, as it doesn't expose the whole internal scope, and yet there are reasons evaluation, even via Function are not allowed by default in CSP.

Would it be too much to ask to at least specify in your README that CSP must have special care because otherwise nothing works in here?

There's not a single entry about security and CSP needs, which is not how the Web should be in these days, imho.

WebReflection commented 4 years ago

some extra hint

el.innerHTML = this.evaluateReturnExpression(el, expression, extraVars)

evaluateReturnExpression passes through saferEval, meaning it's possible to change the updating UI behind the scene (or maybe script tags for legacy browsers?)

I also have a hint for the evaluation itself ... instead of this:

var result; with($data) { result = ${expression} }; return result

you could just go:

with($data) return ${expression}

That'd work too.

However, at least it's only data in a Function that you are passing, but the returned value could be a Proxy.

Once you have one of those, the control over handled data is easily taken over.

Again, all I am asking is to put at least some security related information in the README, because there's no reference whatsoever to:

It would be really awesome if you could address at least one of these concerns in the README šŸ‘‹

WebReflection commented 4 years ago

P.S. I have also changed the title to Improve Security Awareness as I believe at least that should be done, while the previous title wasn't asking for any action.

SimoTod commented 4 years ago

Thanks for your detailed explanation @WebReflection. I replied to know your point of view since I was interested (I asked another guy im the past but he never got back to me). I'm not the owner of the library but I'm sure it will be considered by Caleb.

Since it's an open source peojwct, you can peobably PR the disclaimer yourself if you like.

Random considerations...

I'm not a fan of the x-html directive myself and I agree that it shouldn't even be there.

Alpine requires unsafe-eval so obviously I wouldn't use it to build a banking solution, it will never pass a pentest. I believe its natural use case is for personal blogs and similar where people don't want to go mad with the virtual DOM only to have a dropdown or a fancier modal window.

Thanks again.

WebReflection commented 4 years ago

FWIW, I've never used, nor needed, virtual DOM on the Web, and I understand the easy entry point use case for alipne, however this might get just more popular, so that some awareness regarding its constrains, or architecture model, might help people make a more conscious decision, so I hope Caleb will think about it.

nyariv commented 4 years ago

I agree with @WebReflection, alpine js bypasses any protection you try to implement with CSP. The whole idea of CSP is to give blanket protection because it is near impossible to plug all current and future vulnerabilities in your tech stack that could introduce any kind of html injection.

Unfortunately using with() or proxies is not going to help much and are pretty trivial to bypass if the expression goes through eval or Function. The only way to really protect yourself is using a custom evaluation function, or wait for Realms to be adopted by ECMAScript.

I too was inspired by the ideas behind alpine js and started creating my own js eval function (sandboxjs) to begin solving that problem. It currently works only on single expressions without code blocks, but allows blacklisting globals or anything in the prototype chain. Unfortunately any kind of solution like this would bloat alpine js considerably which goes against its premise.

WebReflection commented 4 years ago

worth saying, that for what I could tell, expressions are always like thingy = value, which is a very simple thing to .split(/\s*=\s*/), to take thingy as property to use to assign value to data, and value is eventually the only thing that might need parsing, if not accepted as JSON right away.

All I am saying is that likely there's no need for a whole sandbox / parser here, but I also don't know this library enough.

P.S. the example with a whole fetch request as attribute content though ... well, that invalidates my proposal already .... but why would anyone do that, write so much JS as node attribute, I don't understand.

nyariv commented 4 years ago

its very easy to obfuscate js, so regex checking is not going to help. You could, for example, put = sign in a string.

WebReflection commented 4 years ago

true that, although if code gets obfuscated here the logic break, as the with($data) statement requires exact names ... the split can also be $data[slit[0].trim()] = split.slice(1).join('='); but again, I don't think this works, 'cause this library really asks for evaluated code in attributes.

trusktr commented 4 years ago

@WebReflection Do any of these concerns also apply to the similar Mavo library by @LeaVerou? https://mavo.io

Mavo has expressions, but I don't really see any actual JS evaluation. Looks like the accessible functions in the expressions must first be defined in JavaScript (where code is not injected, but I haven't tried it fully, maybe that's actual JS).

This might be something for Alpine to consider: how to allow users to specify functions for use in markup, although it would be a breaking change.

If Mavo relies on globals, or a namespace somewhere in global scope, then I imagine the issue would be just the same as here: any 3rd party code could override the functions that it detects are being called in the markup.

Plus, any 3rd party code can practically override any methods of any global class, so that's already a big doorway, Alpine or not, Mavo or not.

WebReflection commented 4 years ago

@trusktr I don't want to speculate on @LeaVerou's work here for at least two reasons:

That being said, since you pointed me at Lea's work, it looks like mavo uses a specialized syntax that has nothing to do with JS, examples:

[count(done)] done out of [count(task)] total
[readable(to(filename(image), '.'))]

these are not eval-able expressions, as I believe mavo knew better than just polluting the global scope with functions such as to, readable, etcetera.

However, if mavo is based on eval or Function, and its eval'd data comes from the shared document, or its view (window), mavo might be as well a vector to inject arbitrary code.

As reminder, CSP might allow specific libraries and their code to be trusted. If these trusted libraries expose to third party scripts a way to evaluate arbitrary code via regular, and always allowed, DOM manipulation, then any of these libraries should put a warning on top of their readme, about the risk such library exposes in any public site using it, as we, Web developers, don't get to control which other scripts could land in a page, and this is the far-west when it comes to ads injectors (I just happen to work in the field that tries to circumvent those scripts).

nyariv commented 4 years ago

Mavo does use Function using with() https://github.com/mavoweb/mavo/blob/bfe96875e58f8954c7ca2b487df96b6abde8df65/src/mavoscript.js#L792

WebReflection commented 4 years ago

@nyariv at least somebody has been honest in there: https://github.com/mavoweb/mavo/blob/bfe96875e58f8954c7ca2b487df96b6abde8df65/src/mavoscript.js#L782-L785

// Yes this is a horrible, horrible hack and Iā€™m truly ashamed. // If you understand the reasons and can think of a better way, be my guest!

here there's not even a comment about being a bad hack

tlgreg commented 4 years ago

A notice that the library relies on unsafe-eval would indeed be good in the readme.

The evaluation code came from Vue 2, which solves this issue by having the option to pre-compile the template and only ship the runtime to the client.

Vue 1.0 (when there was no pre-compilation yet) solved this issue by using notevil to evaluate expressions on a separate CSP build.

bep commented 4 years ago

I would assume that it would be possible in a future version to add a "safe option" that used a simpler eval, which would allow:

<div x-data="{ tab: 'foo' }">
    <button :class="{ 'active': tab === 'foo' }" @click="tab = 'foo'">Foo</button>
    <button :class="{ 'active': tab === 'bar' }" @click="tab = 'bar'">Bar</button>

    <div x-show="tab === 'foo'">Tab Foo</div>
    <div x-show="tab === 'bar'">Tab Bar</div>
</div>

But would error on something like:

<div x-data="{ open: false }">
    <button
        @mouseenter.once="
            fetch('/dropdown-partial.html')
                .then(response => response.text())
                .then(html => { $refs.dropdown.innerHTML = html })
        "
        @click="open = true"
    >Show Dropdown</button>

    <div x-ref="dropdown" x-show="open" @click.away="open = false">
        Loading Spinner...
    </div>
</div>
WebReflection commented 4 years ago

@bep yes, if expressions are simple enough to be parse-able, the former wouldn't need any eval/Function, but the later one would require a hell of a parser

tlgreg commented 4 years ago

Was just thinking if notevil could be a solution, but probably only similarly to vue1, as a csp build.

notevil is more than double the size of alpinejs itself because it includes esprima to parse the javascript expression (alpinejs 6.6kB vs notevil 15.4kB min+gzip).

Other downsides:

SimoTod commented 4 years ago

Acorn could be a better alternative (it should be smaller and faster) but we would have to write our own parser. I assume we could impose some limitations, too. Something like: Only x-data can access the global scope, directives should be a single expression (assignment to a variable or call to a function defined in x-dara), etc. That would probably "annoy" a lot of people though.

I believe that, if the parser replicates what eval does, even if we have "fixed" the csp settings, it will still be suffer from the same security issues.

WebReflection commented 4 years ago

@SimoTod yes, a parser that ends up evaluating would have exact same result, and so would globally defined callbacks.

One way to go would be to register once callbacks, i.e.

alpine.register("drop-partial-fetch", $refs => {
  fetch('/dropdown-partial.html')
    .then(response => response.text())
    .then(html => { $refs.dropdown.innerHTML = html })
});

After that, all one should do is to write:

<div x-data="{ open: false }">
    <button
        @mouseenter.once="drop-partial-fetch"
        @click="open = true"
    >Show Dropdown</button>

    <div x-ref="dropdown" x-show="open" @click.away="open = false">
        Loading Spinner...
    </div>
</div>

That would at least grant that nobody can inject non-registered callbacks or, more important, change their action.

Callbacks are kept private, and no CSP exception is needed. Other attributes with open = true can be easily parsed as key = value expression without needing eval.

Anyone can still attach arbitrary listeners, eventually, but at least the library wouldn't be the evaluation channel for any kind of good or malicious script.

bep commented 4 years ago

If I was to think a little simpler and for the sake of discussion argued that I would need to have a CSP config like this:

Content-Security-Policy = "script-src myhost.com 'unsafe-eval'"

The above will allow AlpineJS to work, but would disallow constructs like this:

<script>
for (const el of document.querySelectorAll('*')) {
  const attributes = [...el.attributes].filter(attr => '@click' === attr.name);
  if (attributes.length)
    attributes[0].value = 'alert("seriously? in 2020?")';
}
</script>

But then, if you're receiving user-provided content (discussion forum, a "dev blog", whatever) you're worried about people injecting x-data components into your DOM ...

What if Alpine added an Alpine.Init(dataElementFilter) that restricted Alpine components to live in some defined areas of the page?

You would not solve the CSP problem, but you would strongly reduce the attack surface while still easily support the core uses for Alpine in these situations.

SimoTod commented 4 years ago

@bep Yeah, in my opinion eval is not necessary evil by itself but requires discipline to stop other scripts from exploiting it (I might be wrong and there could be sophisticated attacks that i have't considered, which is the reason why I'm not comfortable with writing the disclaimer by myself).

I think it's the original point from @WebReflection, if the library needs it, it should, as a first step, just say something in the readme to "educate" and make users aware of it.

Your example is correct but if you have a lot of code, you probably want to put everything in an inline script, like

<script>
    function data() {
        return {
            ...
        }
    }
<script>

At that point, people without much CSP experience could be tempted to add unsafe-inline to the policy where they should really generate a nonce token server side for each request and use that one to trust their safe inline scripts.

Or your policy needs an external library and, if possible, it would be wiser to generate the sha254 of the script and trust that exact implementation rather than whitelisting the external domain and trusting everything that comes from it.

bep commented 4 years ago

@SimoTod I think it's fairly easy to avoid unsafe-inline with Alpine, and unlike other JS frameworks out there, Alpine does encourage "writing the code yourself" rather than pulling in some random plugin, so that itself limits the attack surface. A way to limit the scope of what DOM nodes can have x-data would limit it even further.

iquito commented 4 years ago

Just wanted to throw in that as a new user to Alpine I would have appreciated a heads-up that Alpine relies on eval, as I use a very strict Content-Security-Policy for all my websites, so I cannot use Alpine, but I only found that out after reading through a lot of documentation and finding this issue.

Even better would be a way to use Alpine without eval, as I feel a strict Content-Security-Policy is one of the best ways of making a website secure "by default", so it is a pity when libraries can only be used by turning off these reasonable security features.

ryangjchandler commented 4 years ago

Just wanted to throw in that as a new user to Alpine I would have appreciated a heads-up that Alpine relies on eval, as I use a very strict Content-Security-Policy for all my websites, so I cannot use Alpine, but I only found that out after reading through a lot of documentation and finding this issue.

Even better would be a way to use Alpine without eval, as I feel a strict Content-Security-Policy is one of the best ways of making a website secure "by default", so it is a pity when libraries can only be used by turning off these reasonable security features.

Well strictly speaking it doesn't use eval. It uses a safer eval, that runs through a constructed function.

SimoTod commented 4 years ago

I'm slowly writing a JS sandboxed interpreter which would work with Alpine and remove the need of unsafe-eval. It might not make it to the core but, in that case, I'm happy to maintain a fork or a plugin for those who need to use Alpine with a strict CSP if there's enough interest in it. It's going to be based on Acorn (inspired by this article -> https://blog.bitsrc.io/build-a-js-interpreter-in-javascript-using-acorn-as-a-parser-5487bb53390c) and it would add about 30KB (gzipped) to the library. Initially it will only support a subset of all the possible JS instructions but it should be usable. I'll try to put everything on Github soon (Hopefully this weekend) in case anyone wants to collaborate. I'll post an update once ready.

nyariv commented 4 years ago

When writing the parser, some considerations that should be taken:

  1. prototype pollution: [].constructor.prototype.filter = () => alert("!")
  2. infinite loops: while(new Date().getTime() > 0);
  3. global variables access: window['alert']("!")
  4. slow regex
  5. dom injection el.addEventListener('click', (e) => e.target.closest('body').innerHTML = '!')

These are just a few of the issues. But like i mentioned in my initial comment here, i have worked on a library that solves these by protecting access on the prototype level and completely forbidding code blocks and loops. I can help out on the parser you are writing and borrow some concepts from sandboxjs

SimoTod commented 4 years ago

Sure, i had totally forgotten about your initial message. We can maybe use sandboxjs directly. I'll have a look at the code in the next few days.

SimoTod commented 4 years ago

@nyariv I think you need to recompile the dist folder. Your package js defines "main": "dist/Sandbox.mjs", as an entry point but it's the only file that hasn't been updated for 1 month and doesn't match the latest documentation so when I import the Sandbox object, I get an old version.

nyariv commented 4 years ago

@SimoTod you are right, it is fixed now.

HugoDF commented 4 years ago

@SimoTod @calebporzio since we merged that disclaimer about CSPs should we close this until we want to create a CSP-friendly build.

SimoTod commented 4 years ago

I assume we can close it and people can open a new issue if they have further concerns.

For those who follow, v3 will have a more modular approach which should allow for a csp friendly build. Unfortunately, this is not possible in v2 without introducing breaking changes.

The readme has now a section about Alpine requiring unsafe eval when using CSP and other security considerations. It's open to PRs if anybody feels something important is missing.

matthewp commented 3 years ago

@SimoTod @HugoDF Now that v3 is out, I haven't been unable to find any documentation on this issue. Is it documented any where?

HugoDF commented 3 years ago

@SimoTod @HugoDF Now that v3 is out, I haven't been unable to find any documentation on this issue. Is it documented any where?

There's a CSP compatible build in v3 using Alpine.data

matthewp commented 3 years ago

Good to hear, is this in the docs some where?

matthewp commented 3 years ago

Found it! https://alpinejs.dev/advanced/csp

matthewp commented 3 years ago

You might be aware, but @alpinejs/csp hasn't been published yet.

WebReflection commented 3 years ago

FWIW, as initial "blamer" of the issue, I am super happy about the current increased awareness and solution: it's elegant, easy to work around, and much cleaner than the default behavior.

Thanks for moving, thinking, and implementing, this forward šŸ‘ ... but if it's still not published, as @matthewp suggested, please do so as soon as you can!

jtallinger commented 3 years ago

Any chance this could go on to npm?

Quite challenging to manually download and build and then import in a docker environment (and manage the dependencies with for an example webpack).

tolexy commented 3 years ago

Closely following the CSP-friendly version of this. Is there a general timeline we can expect to easily utilize this much anticipated CSP version?

andreseduardop commented 3 years ago

The CSP version will take Alpinejs to the next level. :sparkle:

tomb1n0 commented 3 years ago

Any news on if the CSP build will be published to NPM soon? I could really do with this on a project šŸ˜…

lursyy commented 2 years ago

I am building a chrome extension using the current manifest v3, which disallows unsafe-eval for extension pages. Without this, I am forced to abandon AlpineJS for now :(

HugoDF commented 2 years ago

I am building a chrome extension using the current manifest v3, which disallows unsafe-eval for extension pages. Without this, I am forced to abandon AlpineJS for now :(

Alpine v3 should have a CSP friendly build where you define your JS in JS so there is no eval.

mansoorkhan96 commented 2 years ago

I am building a chrome extension using the current manifest v3, which disallows unsafe-eval for extension pages. Without this, I am forced to abandon AlpineJS for now :(

Alpine v3 should have a CSP friendly build where you define your JS in JS so there is no eval.

How can we install the CSP build? npm install @alpinejs/csp does not seem to be working.

ryangjchandler commented 2 years ago

@mansoorkhan96 The CSP build hasn't been officially released & published yet.

mansoorkhan96 commented 2 years ago

Thanks @ryangjchandler for the prompt response. Any workaround? I need to use alpine in a chrome extension and the v3 manifest does not allow "unsafe-eval".

ryangjchandler commented 2 years ago

@mansoorkhan96 You could potentially use a submodule of the Alpine repo and build the CSP version from source?

mansoorkhan96 commented 2 years ago

@mansoorkhan96 You could potentially use a submodule of the Alpine repo and build the CSP version from source?

@ryangjchandler I am not sure how to do that. But i am willing to learn, any resource/article where i can get started and create a submodule.