angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.81k stars 27.49k forks source link

XSS issues with server-rendered Angular templates #5601

Closed triskweline closed 10 years ago

triskweline commented 10 years ago

Dear Angular community,

we would greatly appreciate any comments on the issue below.

Overview of the issue:

When rendering Angular templates with a server-side templating engine like ERB or Haml it is easy to introduce XSS vulnerabilities. These vulnerabilities are enabled by AngularJS evaluating user-provided strings containing interpolation symbols (default symbols are {{ and }}).

The standard mitigation strategy by templating engines is to always escape meaningful symbols in strings (unless they are explicitely marked as "safe"). Unfortunately the inability to escape AngularJS interpolation symbols makes it hard to apply this strategy here.

Reproducing the issue:

We are using AngularJS for the frontend and Rails in the backend. While we have written full Single Page Applications with AngularJS, we are often embedding small snippets of AngularJS in server-rendered views. We love that AngularJS lets us replace jQuery spaghetti with simple code like this:

<div ng-show="detailsShown">
  <%= user.profile %>
</div>

<span ng-click="detailsShown = true">Show details</span>

The problem is that the code snippet above enables Cross Site Scripting (XSS). If the server-parsed expression user.profile contains AngularJS markup like "Foo {{signOut()}} Bar", it will trigger functions on the current scope, allowing an attacker to trigger side effects on another computer.

Proposed fix:

The standard mitigation strategy by templating engines is to always escape symbols that have meaning for downsteam parsers, unless they are explicitely marked as "safe". E. g. if the server-parsed expression user.profile in the code example above would yield the string "Foo <script>alert('owned')</script> Bar", Rails, Haml, etc. would automatically replace the angle brackets with HTML entities.

To allow HTML tags to be rendered without escaping, the developer needs to explicitely mark a string as "safe" like so:

<%= user.profile.html_safe %>

(AngularJS actually uses this approach itself, when inserting the results of an evaluated Angular expression into the DOM.)

A good default for people rendering Angular templates using server-side templating engines would be to make templating engines automatically escape Angular interpolation symbols unless an evaluated string is explictely flagged as "safe". A patch for engines like ERB or Haml would be easily created, but unfortunately Angular's $interpolate service does not recognize any kind of escape symbol.

We propose that the $interpolate service should recognize escaped interpolation symbols like this:

Foo \{\{signOut()\}\} Bar

... or maybe a second pair of start/end symbols.

When encountering escaped interpolation symbols, the service would not evaluate the enclosed expression. Instead it would strip the escape symbols from the string.

(Note that we first tried to replace the curly braces with their HTML entities, which would have the added bonus of being output-neutral outside of an ng-app element. However, entitized braces are still recognized and their contents evaluated by Angular. We're not sure if the entities are expanded by Angular or by the browser.)

Plea to not dismiss this issue

We understand that this issue is not an actual bug in AngularJS, but (like all XSS issues) is caused by combining multiple technologies that are unaware of each other. Still we believe that AngularJS' ability to co-exist with non-Angular code in the same DOM is one of AngularJS greatest strengths. It sets Angular apart from frameworks like Ember, which need to own the entire page.

By offering a way to escape interpolation symbols (or maybe some other solution we didn't think of), AngularJS could be used safely in combination with upstream templating engines.

What do you think?

caitp commented 10 years ago

So, there's a problem with this, and I'll tell you what it is, with an example:

http://jsfiddle.net/KKDAE/

When it comes to "actually" escaping things into HTML entity references, these are going into text nodes, and by the time the compiler looks at them, they are already going to be "dangerous".

What you can do is escape these into something meaningless which the browser won't turn into an expression which will be interpolated, thus preventing execution.

I'm sorry if it sounds like I'm dismissing the issue, but it sounds to me like you have the tools at your disposal to prevent unwanted expressions from getting rendered already (at the cost of putting unexpected/hard to read stuff in the view)

As you suggest, \{\{ wouldn't be interpolated (by default, unless you tell the interpolate service to deal with this), so that's an option for you --- it just makes stuff look a bit gross. http://jsfiddle.net/q8B4p/1/

tdierks commented 10 years ago

I definitely agree. Software authors need to be able to control such metadata; given the stream-expressed encoding of HTML, this requires the ability to escape metadata instructions to reduce the privilege level of potentially-harmful content.

caitp commented 10 years ago

Right, but you can really already do exactly that --- if you simply escape, or even totally remove interpolation expressions or untrusted attributes from your rendered templates, you avoid this issue entirely. I'm not sure what we can really do in angular-land to deal with unescaped templates, we have no idea that you aren't serving exactly what you intended to.

triskweline commented 10 years ago

Thanks for your comments and especially @caitp for for taking the time to write up the fiddles.

I agree that we can fix the security aspect of this by simply replacing the interpolation markers with something else. But then we have changed the contents of the view, which can produce other issues.

I understand that using HTML entities cannot be used since the browser will expand them before Angular starts interpolating. My idea was that since Angular can't know if an interpolation mark is safe to parse or not, there could be a way to tell Angular if an {{ is meant to be a verbatim {{ that should be rendered so.

One way to tell Angular to render a verbatim {{ could be to use a second pair of interpolation marks, which Angular would print as {{ instead of evaluate the contents.

caitp commented 10 years ago

The issue with that is that, if there was another interpolation expression in the text node, then this would work for only one digest, because the following digest it would no longer be escaped (if you want to render it as an unescaped string). Having a way to flag an expression as being ignored might be nice, but currently it doesn't really fit into the way things work, and would likely be a pretty big/complicated change.

For now, the best bet is to simply escape your stuff server-side, there really isn't much else to do about it :(

tdierks commented 10 years ago

I don't think it's possible to escape this server-side without changing the content. If a profile (in the example) happens to have "{{" in it, you can't serve that content safely without changing how it looks to the end-user (e.g. inserting a space into the middle). Since {{ could occur in other contexts that are harder to change (e.g. it could be part of an image URL), it's even more complex.

I don't know how to fix this, but I believe it's a real and significant issue.

caitp commented 10 years ago

Yeah, as said, we'll discuss this to see if we can make a change to the interpolate service / html compiler to resolve this.

But the issue is always going to come back --- once you escape {{ with {-{, what happens when you need {-{ in a template? Where does it end? At a certain point you need to constrain the content

tdierks commented 10 years ago

Sure, this is a solved problem for all escaping systems. You could make people send {{{ when they want to render {{; you'd just trim one { off of any string of {'s longer than 2. I agree that it's not obvious how to do this without adding a metadata tagging system to the in-memory copy of the page, but I don't know enough about the internals to make a specific implementation suggestion.

tbosch commented 10 years ago

Thanks for reporting this. How about using a character that looks like curly braces but is not? See http://stackoverflow.com/questions/13535172/list-of-all-unicodes-open-close-brackets

Normal braces: { Substitues: { : 0xFF5B ❴ : 0x2774

caitp commented 10 years ago

That's not a bad solution for them, and is probably significantly cheaper than re-arching the $interpolate service to keep track of escaped expressions or whatever.

triskweline commented 10 years ago

Is it true that {{ marks (once unescaped by an imaginary $interpolate service) would be evaluated as Angular expression by a following digest? How could I verify this with code? Because if that is the case the broader security implications of this scare me. Doesn't this me that any client-side integration that inserts unescaped text nodes into the DOM can make Angular run Javascript?

Some examples:

Unless we can make any DOM-changing Javascript library escape their curly braces, is there a way to remedy this? Would we need to recommend Angular users to never use libraries that insert uncontrolled text nodes into the DOM?

triskweline commented 10 years ago

@tbosch Thanks for the creative suggestion :) We tried something similar, but instead used &#8203;, a zero-width space, which is a thing that exists.

We abandoned the idea because it broke inlined Javascript. We were also worried it would confuse users who copy text from the page and use it for something else (e.g. this very page).

We're currently escaping {{ to { { (spaces left, middle and right) which felt less magic and preserves most inlined Javascript. We're still unhappy that we're changing the content at all.

caitp commented 10 years ago

Is it true that {{ marks (once unescaped by an imaginary $interpolate service) would be evaluated as Angular expression by a following digest?

well, it's sort of like this (I believe, and it's possible I'm wrong, due to not having touched the $interpolate service much)

If you give us something which the browser's HTML parser decides looks like {{, then during the HTML compilation phase, it's going to become a watched interpolation expression (it's possible that it does need the matching }} though, it wouldn't be hard to check) --- so, once the interpolation expression is built, then the string literals don't really matter (I believe), the actual text content of the node is never going to matter again, unless $interpolate is called again... so the original interpolation listener is basically going to replace the text node content every digest, and evaluate the interpolated expression every digest.

There are a few parts of this where I could be mistaken, so it may theoretically be possible to make this work well, but I'm not totally sure it's the right thing to do, I think @tbosch's proposal would make a lot more sense.

xrg commented 10 years ago

On Thursday 02 January 2014, Henning Koch wrote:

<%= user.profile %>

What about the ng-non-bindable directive?

<%= user['bobby.tables'].profile %>

http://docs.angularjs.org/api/ng.directive:ngNonBindable

Disclaimer waiver: When you send me an unencrypted email, you implicitly allow me, or any 3rd person reading our mails, to do anything I/they wish with your data (including presenting them in public). Your disclaimer, thus, is void. If you had wanted a private communication, you should have used encryption in the first place.

triskweline commented 10 years ago

@xrg: We experimented with ng-non-bindable. Unfortunately it's a rather blunt instrument since it applies to the element, all of its attributes, and all of its descendant elements. E.g. you can't disallow interpolation in attributes but allow it for descendants, or vice versa.

xrg commented 10 years ago

On Friday 03 January 2014, Henning Koch wrote:

@xrg: We experimented with ng-non-bindable. Unfortunately it's a rather blunt instrument since it applies to the element, all of its attributes, and all of its descendant elements. E.g. you can't disallow interpolation in attributes but allow it for descendants, or vice versa.

Why would you need to /allow/ any interpolation within your user-supplied string?

But, in case you do, a more fine-tuned 'non-bindable' might be the solution, after all.

Disclaimer waiver: When you send me an unencrypted email, you implicitly allow me, or any 3rd person reading our mails, to do anything I/they wish with your data (including presenting them in public). Your disclaimer, thus, is void. If you had wanted a private communication, you should have used encryption in the first place.

sukei commented 10 years ago

This issue is serious but the ng-non-bindable directive seems to be the perfect fix. It's certainly not powerful enough to cover your cases but should works for the others.

kratob commented 10 years ago

I don't think using ng-non-bindable is a good solution. While it fixes the XSS issue, it is hard to automate server-side:

In Rails (and other frameworks like Django), XSS protection is automatic, i.e.

<%= @user.profile %>

will be (HTML-)escaped by default, unless you do something explicit like

<%=raw @user.sanitized_profile %>

Escaping works by replacing < with &lt; etc...

This is a good thing, since it's so easy to forget manual escaping, but it cannot do elaborate things like wrapping something in a ng-non-bindable.

sukei commented 10 years ago

Others templating languages like Twig allow the developer to extend the grammar and the behaviour of the templating engine, you can easily add a new behaviour that wrap untrusted data. But it's maybe not possible for many other templating languages.

Moreover, developers should be aware of such a thing to activate the appropriate measures.

In my opinion, the best thing to do is to educate developers about this security issue. The famous "don't trust user inputs" applyed to angularjs.

wesleycho commented 10 years ago

I agree with @sukei - I feel like this proposal is not appropriate for two reasons.

  1. The server should not be trusting user inputs, and check for attempts at exploiting vulnerabilities such as with {{ and }} to force a function on $scope or the controller to fire
  2. There are times with server-side templating where someone might want to inject a string with an interpolation meant for Angular to hook into (although this may be a bit of an insane pattern, it is one allowed & should be supported IMO).

I'm not sure what the wisdom of enforcing this on the client-side would be, given that this sounds more like a backend problem to me. The result of trying to enforce it on the client-side would be more code, and it would not wipe out the vulnerability if the change was present in $interpolate alone since whatever workaround is done, the injection would be able to just use it to still abuse the vulnerability. If there is no way to whitelist the interpolation, then this would become a rigid breaking feature.

This cannot be whitelisted using the $sce service because there is no way to know what the injected HTML will look like in advance, and the interpolation happens via Angular's DOM traversal.

One implementation to satisfy this would be to add something to $interpolateProvider so that you could choose to have Angular automatically escape any such exploitation.

caitp commented 10 years ago

Even if you change the start/end interpolation markers, the HTML compiler converts {{ and }} to match the new values, for interoperability with external components. So there isn't really anything angular can do client-side to prevent this.

As you say, it comes down to being entirely a server-side issue, but a solution such as suggested by @tbosch should be acceptable. "It broke inlined javascript" well, I'm having a hard time seeing how this is a "problem", since you typically don't want inlined javascript in user data anyways.

I don't really think this is something to be fixed on the client-side

sukei commented 10 years ago

@caitp is right, there is nothing we can do on the client side. The server side templating engines escaping is done assuming that you will output html content. That's why it converts the <, > and some other html entities into entity names (&lt;, &gt;). It's a common solution that works fine for the html. If you need to escape 'angular entities', it's up to you to write a solution that does the job.

That's why I was talking about Twig and its extensibility that allows the developers to improve the escape function. I'm not sure that you can achieve such a thing in all other templating engine. Just keep in mind, that the solution should be set in the server and not the client.

triskweline commented 10 years ago

Inlined Javascript can be used for performance reasons to pre-populate an Angular scope without an additional AJAX request, but on second thought: Inlined Javascript is not subject to server-side escaping mechanisms, so there is no problem here.

The issue remains that we're changing content by replacing curly braces with something that looks like one, but isn't. I can't copy the changed text and use them for purposes where curly braces are expected (e.g. code snippets in this very page). I can't find those braces with a text search. And I will have a hard time to find out what's wrong, since the letters will look fine, but aren't.

triskweline commented 10 years ago

I feel this thread has moved away from the original proposal. I'm not asking Angular to solve this problem on its own. @sukei correctly said "If you need to escape 'angular entities', it's up to you to write a solution that does the job.". This is what I'm trying to do: Solve it on the backend. But I can't. It is not possible to escape Angular entities without changing the content.

The original proposal was to change $interpolate so it recognizes escaped interpolation marks. This would allow a server-side templating engine to solve this problem.

Here is a rough draft of a change to AngularJS that would add the ability to escape interpolation symbols using \{\{ and \}\}:

https://github.com/makandra/angular.js/commit/6c4490f77753c359d7ae871f1518ad0bf57ae53a

The commit linked above still has some minor issues:

... but that can be fixed easily.

What do you think?

IgorMinar commented 10 years ago

Let me sum this up before this thread goes too wild. I'll update this summary as needed with future updates:

Summary of the issue

Issue 1. mixing server-side and client-side templating could result in XSS

example code:

<div ng-show="detailsShown">
  <%= user.profile %>
</div>

<span ng-click="detailsShown = true">Show details</span>

if this template is evaluated by Rails then <%= user.profile %> is interpreted as server side interpolation. If this user.profile expression evaluates to something that contains {{ and }} (e.g. My name is Patrick and I like {{ double.curlies }}. then the template arrives into the browser as:

<div ng-show="detailsShown">
  My name is Patrick and I like {{ double.curlies }}.
</div>

<span ng-click="detailsShown = true">Show details</span>

Once Angular processes this template then it will generate DOM that looks most likely like this:

<div ng-show="detailsShown">
  My name is Patrick and I like .
</div>

<span ng-click="detailsShown = true">Show details</span>

Notice that the {{ double.curlies }} was replaced by an empty string. This is because Angular interpreted this substring as a binding and evaluated it, but the double.curlies expression yielded no value which results in an empty string being used in the DOM view.

Because the expression was evaluated by Angular, the server-side interpolation could force Angular to executed unexpected code via expressions. This would require the user input to be crafted in a way that is meaningful to the Angular app being exploited.

Solution

It is not recommended to mix server-side and client-side templating that uses user-provided input in this way. We discourage developers from doing so because it's hard to audit and fix all the possible corner-cases that could be exploited using the interaction of these two templating systems. This is not a new recommendation, it has been in place since the first Google security review in 2011.

If you really really have to though, there are some workarounds:

Workaround 1

Use ng-non-bindable or a custom terminal directive that will prevent Angular's template compiler from descending into the DOM subtree that is constructed on the server-side from user's input:

<div ng-show="detailsShown">
  <div ng-non-bindable>
    <%= user.profile %>
  </div>
</div>

<span ng-click="detailsShown = true">Show details</span>

Workaround 2

The server could use html escaping on the input (user.profile in our case) and render the input harmless by escaping it in a way that would make Angular treat it as being a string rather than some template with binding or other instructions. This approach works almost perfectly with rails, which does this kind of escaping automatically, except for the inability to escape interpolation markers ({{ and }}) - see the second part of this issue.

Issue 2. Inability to escape interpolation markers on the server so that Angular does not interpret them as bindings

When Angular see's interpolation markers {{ followed by }} in a textContent or attribute value, it interprets the string in between as an expression that should be used for data-binding. These interpolation markers are configurable, but double-curlies are the default. The problem is that a server can't currently escape and thus disable these markers in a template being sent to the client. This is only a problem in case when two different server-side and client-side tempating engines are being used to generate the same view (see the first part of this issue).

A naïve way to approach the escaping would be to use html entities to encode the marker characters in the template - just as is being done with html tags.

This approach doesn't work, because Angular consumes DOM templates and not template strings. This means that when a string (containing html entities) is sent from the server to the client, the browser will parse it and turn it into DOM tree. Angular then walks the DOM tree and looks for interesting things (like interpolation markers). The problem is that by the time Angular finds the interpolation marker in a textContent attribute value, it has already been converted from html entity into a regular character.

Another approach would be for the server to replace {{ foo }} with {\{ foo }}. This however mangles the user input which is not desirable.

Yet another approach would be for the server to replace regular { and } symbols with unicode characters that visually look similar, but from the computer (code point) perspective are different. Example of these characters are: { : 0xFF5B ❴ : 0x2774.

This approach almost works, except if a user tries to search for curlies in the text or tries to copy&paste the text from the view, then these pseudo-curlies could cause a lot of issues that are not obvious to understand and debug by the end user.

Solution

Since it's unlikely that we can find a way to escape interpolation markers using html entities or with a similar method, the server and client might need to cooperate to get the behavior just right.

One solution would be for the server to escape interpolation markers in a way that Angular understands and treats as escaped markers that should be unescaped but not interpreted as binding. Example:

  1. server escapes {{ user.profile }} as {{{{ user.profile }}}}
  2. angular is configured to treat {{{{ followed by }}}} (e.g. via additional arg to $interpolateProvider's startSymbol and endSymbol methods)
  3. when angular comes across {{{{ user.profile }}}}, it simply replaces it with {{ user.profile }} but does not treat it as binding.
sukei commented 10 years ago

when angular comes across {{{{ user.profile }}}}, it simply replaces it with {{ user.profile }} but does not treat it as binding.

\o/

IgorMinar commented 10 years ago

since we need to be explicit about the regular and escaped interpolation markers during configuration, it might be better to just add a second argument to the $interpolateProvider. So one could do:

$interpolateProvider.startSymbol('{{'. '{{{{');
$interpolateProvider.endSymbol('}}'. ''}}}}');

I'm not opposed to making this the default if we find a consensus on escaped markers that don't break anything out there. (this would be for 1.3 of course)

IgorMinar commented 10 years ago

PRs welcome!

caitp commented 10 years ago

Well there's my shot at it, if anyone cares to improve the algorithm on their own, or offer suggestions or improve test coverage, you're more than welcome

yuasakusa commented 10 years ago

(Disclaimer: I do not have any plan to mix server-side processing with Angular like this, but I am hypothetically imagining that I were doing that.)

I am a little bit concerned by the potential complexity in the server-side processing to use the proposed solution correctly.

Suppose that I have a database with user profile, which allows certain HTML elements (say, only <b> elements are allowed). To use this solution correctly, the server must process strings as follows:

So the server has to take into account how Angular would interpret the result, and I am afraid that this is error-prone. Is there any alternative such that the server-side processing is simpler?

caitp commented 10 years ago

@yuasakusa feel free to contribute a test to my current implementation to verify what would happen in this case, I'm fairly sure you'd see {{abc}}}}{{def}} rendered in the view without parsing in the first case, and {{abc<b>def</b>ghi}} rendered in the view without parsing in the third case (where the <b> is a literal child node due to not being escaped on the server). In the second case, the interpolate service won't parse it due to not finding the end symbol, I believe.

yuasakusa commented 10 years ago

@caitp My concerns are not about correctness of your patch or of the implementation of Angular in general. My concerns are that it may be difficult for the server to produce the correct output (which is the input to Angular) given the proposed behavior of Angular, and they are not addressed by adding tests to Angular. What would help is integration tests in each project that uses Angular in combination with server-side processing, but clearly we cannot write those tests as part of Angular.

If you are going to go with the current proposal, a sample code of using Angular with Ruby on Rails correctly would be probably very helpful. Unfortunately writing one is beyond my skills (to begin with, I have never used Ruby on Rails), but I am sure that some people can write one.

caitp commented 10 years ago

Right, but we want to deal with poorly escaped stuff too, presumably, to avoid mistakes. At the very least, if you can think of any arrangements of text which might be exploitable (in the sense of executing javascript which was not meant to be executed), it would be awesome to submit tests so we can try not to choke on them

triskweline commented 10 years ago

Thanks everyone for your work so far!

@yuasakusa A simpler rule would be to have Angular unescape individual {{{{ and }}}} strings, but not require the matching bracket. This way both the server and Angular could blindly replace individual strings without worrying about getting the context right.

caitp commented 10 years ago

@henning-koch unfortunately we can't really do a context-free replacement before doing the actual interpolation, because doing so would defeat the purpose of escaping entirely :( Unless we replaced them temporarily with some unusual unicode characters or something, but I don't really see that as being a good idea, and would probably hurt the typical case performance-wise. Hard to say without measurement though.

triskweline commented 10 years ago

@caitp I didn't mean to replace before the actual interpolation. I meant that we change the string parser in $interpolate to this (pseudo-code):

var pattern = /{{{{|}}}}|{{(.*?)}}/g;
var match;
while (match = text.exec(pattern)) {
  if (match[0] === '{{{{') {
    // we found escaped opening braces
    parts.push('{{');
  } else if (match[0] === '}}}}') {
    // we found escaped closing braces
    parts.push('}}');
  } else {
    // we found an Angular expression
    var expression = match[1]:
    parts.push($parse(expression));
  }
}

The code above scans text for the next interesting substring, interesting meaning one of the following:

(Code is still missing to process text between matches of course.)

triskweline commented 10 years ago

What would help is integration tests in each project that uses Angular in combination with server-side processing, but clearly we cannot write those tests as part of Angular.

We have already published a Ruby library angular_xss that configures popular templating engines to auto-escape Angular interpolation marks in unsafe Ruby Strings. Currently the library still works by replacing {{ by { { because it requires the changes suggested in this issue to do proper escaping.

If code from this issue is accepted into Angular, we would update the library with proper integration testing.

Other Rails projects would require this library, but not need to add their own tests.

yuasakusa commented 10 years ago

@henning-koch Great! With your proposal, the server just needs to replace all non-overlapping occurrences of {{ to {{{{ and (optionally) }} to }}}}, and does not have to worry about how exactly Angular will interpret the input. (That is assuming that the database on the server is sane and does not do anything like storing character { sometimes as { and sometimes as &#x7B;.)

Also it is great to know that there is already a standard library for encoding plain text for Angular. It is even better than just a sample code.

@caitp

At the very least, if you can think of any arrangements of text which might be exploitable (in the sense of executing javascript which was not meant to be executed), it would be awesome to submit tests so we can try not to choke on them

I am afraid that there are hundreds of ways to do it wrong. Even with the much simplified proposal by @henning-koch, I can see how other people may screw up (as I mentioned above, if the server stores character { sometimes as { and sometimes as &#x7B;, it is difficult to handle everything correctly). Providing the library for template engines is indeed a great solution.

caitp commented 10 years ago

Yes, I think we can agree that it would be good (not just for angular, it could probably be specced out and standardized for all of the clientside stuff which uses handlebars/moustache conventions).

However, it sounds like we aren't forcing this to be an entirely server-side issue, and need conventions on the client-side to handle mistakes, and enable such escaping on the server in the first place.

triskweline commented 10 years ago

@yuasakusa Can you elaborate on some complicated ways to express those interpolation symbols, so an escaping implementation might not recognize it?

What I can think of so far:

{
&lcub;
&lbrace;
&#x7b;
&#x000007b;
&#x000000000007b;
&#123;
&#000000123;
&#0000000000000123;

... and all uppercase/lowercase variations.

It's more than I expected (and I need to update angular_xss for this), but manageable by a single Regex so far.

triskweline commented 10 years ago

Regarding the proposal for a simpler escape/unescape grammar, I can try my hand at a pull request next week, building on @caitp's work.

yuasakusa commented 10 years ago

@henning-koch I think that the most tricky part about conversion like what angular_xss does is rather a conceptual issue that the correct thing to do depends on what formats the input and the output of the converter are in. If the input is plain text and not HTML, then &#x7B; literally means a string of six characters, and the converter must not treat it as an open brace. As a plaintext-to-plaintext conversion, or more precisely, conversion from plain text to the content of text node in the DOM for processing by Angular, the correct conversion (assuming your simpler rules on the Angular side) is replace {{ to {{{{ and }} to }}}}, and nothing else. But often the server stores some (restricted) HTML in the database, in which case &#x7B; means an open brace and therefore the situation is different. The point is that the programmer has to be aware of what the input really is. I doubt that there is a catch-all solution.

triskweline commented 10 years ago

@yuasakusa This means we might eventually have to change angular_xss to let developers express: "This string may contain HTML, but it may not contain Angular interpolations". Right now we only let developers choose between "this is a string safe for all purposes" and "this is a string unsafe for every interpretation whatsoever". So right now angular_xss might be a little inconvenient for developers who store actual HTML in the database, e.g. makers of CMS software.

But this change is entirely doable, and until then workarounds are available (like calling angular_xss before marking the string as safe). I believe the most important step right now is to make sure that the new default is to not allow accidental XSS. Err on the side of caution, and require explicit statements by the developer if the default should not be applied.

kratob commented 10 years ago

I was working with @henning-koch on the promised pull request. There is none yet, this is just an update:

We've run into some issues with the server-side part of our solution. Those are most likely solvable (at least for our own needs). Nevertheless, we don't want to make any changes to Angular before we've solved the full problem. Otherwise we might end up with something that doesn't even help ourselves.

There is also a strong possiblity we won't even need any Angular patch at all, by using a different escape mechanism:

This would allow us to automatically escape {{ to something that renders correctly, it does not require a patch, and it has no problems with double escaping and just looks a bit more robust.

We'll keep you posted, but it's gonna take a bit longer.

yuasakusa commented 10 years ago

@kratob That is a clever hack. I think that {{'{{'}} works equally with an advantage of not relying on any variables in scope. (Note that {{'}}'}} does not work, but I do not think that it is needed. Even if it is needed, you can write {{'}'+'}'}}.) I am not sure if people are entirely happy with these hacks as a long-term solution, but for now it is definitely useful to know some ways to correctly mix server-side and client-side processing which do not require any modification to Angular.

kratob commented 10 years ago

@yuasakusa The {{ DOUBLE_OPEN_BRACE }} seemed a bit safer than {{'{{'}}. I was mainly afraid of accidentally escaping

  <el attr='{{'>

to

    <el attr='{{'{{'}}'>

which would be invalid (and might allow XSS again). Maybe there is an alternative that is safe and doesn't require the constant.

yuasakusa commented 10 years ago

@kratob Good point! I did not think of the case of attributes at all. My apologies. I think that {{&apos;{{&apos;}} or {{&quot;{{&quot;}} works as long as it is sent to the browser (without further server-side processing) and processed by Angular. However, this is probably more fragile than your method if the server performs some additional processing, so your method seems like a much safer bet.

idupree commented 10 years ago

(I commented on the change - https://github.com/angular/angular.js/pull/7517#issuecomment-47135479 - comment copied here. I appreciate how tricky escaping is and I want us to get it right! I'd be happy to submit pull requests for docs and/or code, if you approve.)

While this change thankfully makes it possible to write text that expands to {{, it makes it impossible to write text that expands to \{\{, as best I can tell. You could require all backslashes in Angular-processed text to be escaped (i.e. Angular converting \\ to \), and then it would be possible, albeit ugly, to write any literal string.

Alternately, the suggested workarounds in https://github.com/angular/angular.js/issues/5601#issuecomment-32104581 could be documented as the official way to escape Angular's curly braces. They appear to me to be a sufficient escaping system that is no more fragile than this commit's way of backslash-escaping Angular's interpolation markers. It's true that {{'{{'}} riskily adds single-quotes, and {{ DOUBLE_OPEN_BRACE }} requires DOUBLE_OPEN_BRACE to be defined. So you could add, say, NG_DOUBLE_OPEN_BRACE to the default scope environment to make it a solution anyone can easily use. Or make something else within curly brackets that's not syntactical JavaScript magically expand to {{; say, {{\}}.

punam2085 commented 8 years ago

I am new to this. Please help in below code <!DOCTYPE html>

{{'a'.constructor.prototype.charAt=[].join;$eval('x=alert(1)');}} {{lastname}}

Use the ng-bind directive to bind the innerHTML of an element to a property in the data model.

here {{lastname}} is not giving any alert.

IgorMinar commented 8 years ago

Please read https://angularjs.blogspot.com/2016/09/angular-16-expression-sandbox-removal.html for the most recent update.