6

So as a rule of thumb I once learned that adding or removing HTML with JavaScript/JQuery (.html(),.append(), etc) leaves yourself wide open for DOM Based XSS Attacks. It is now my understanding that this is not 100% true. Supposedly there is a correct and safe way to add/remove HTML with JavaScript. I am hoping to learn some on what this "correct way" may be.

So as an example lets say I have an input filed that allows a user to append an item to a list. In this case the input would also be added to an array to be sent in future requests. Additionally this list would have a button to remove said item from that list. In an insecure environment we might do something like the following (negating array):

var list = $("#my_list");

$("#add_btn").on("click", function(){
    let input = $("#input_field").val();
    list.append(
        '<li>'+input+' <button>Remove</button></li>'
    );
});


$("#my_list").on("click", "button", function(){
    $(this).closest("li").remove();
});

How might one do the same but without the threat of XSS?

Link to JSFiddle demonstrating different suggested solutions

FamousAv8er
  • 299
  • 1
  • 8
  • 1
    Hey, just to let you know, it's not recommended to [cross post](https://softwareengineering.stackexchange.com/questions/410300/dom-based-xss-and-adding-html-elements) on multiple Stack Exchange sites. [See this FAQ for more](https://meta.stackexchange.com/questions/64068/is-cross-posting-a-question-on-multiple-stack-exchange-sites-permitted-if-the-qu) – Fire Quacker May 18 '20 at 17:10
  • @FireQuacker I will remember this going forward, thank you. – FamousAv8er May 18 '20 at 17:13
  • has nothing to do with javascript per say, but I would research something like anti-forgery tokens, which makes some cross site attacks more difficult to perform... https://docs.microsoft.com/en-us/aspnet/web-api/overview/security/preventing-cross-site-request-forgery-csrf-attacks – pcalkins May 18 '20 at 22:08
  • @pcalkins I believe you may be confusing XSS for CSRF – FamousAv8er May 19 '20 at 16:39
  • yes, not all that familiar with terminology... but I believe there are XSS attacks that perform CSRF attacks.... (if a user visits a phishing site or has some malware plugin installed... that kind of thing... ) – pcalkins May 19 '20 at 17:23
  • This is why you don't cross post. You are trying to combine responses from different sites. Don't do that. – schroeder May 19 '20 at 19:19

1 Answers1

5

Creating the DOM as string and using append or html indeed leads to DOM-based XSS. What you should do instead is to built the DOM elements using the proper functions and only inserting user-supplied input as text. Example:

<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<ul id=my_list class=my_list></ul>
<input type=text id=input_field value="<img src=x onerror=alert(1)>">
<button id=add_btn></button>

<script>
var list = $("#my_list");
$("#add_btn").on("click", function(){
    let input = $("#input_field").val();

    // this is insecure:
    //list.append('<li>'+input+' <button>Remove</button></li>');

    // this is secure:
    var listentry = document.createElement('li');
    listentry.innerText = input;
    list.append(listentry);
});
</script>
tim
  • 29,018
  • 7
  • 95
  • 119
  • Would manipulating the DOM by removing elements open any attack vectors that could be mitigated in a similar manner? Something like `$(this).closest("li").remove();` – FamousAv8er May 19 '20 at 14:45
  • 1
    @FamousAv8er Not as far as I know (and it really shouldn't happen, as you are not building DOM elements with user input, but just calling functions on some DOM element which we don't know anything about; if that *would* for some reason trigger an XSS payload, I'd consider it a vulnerability in jQuery). – tim May 19 '20 at 15:41
  • 1
    @dandavis It doesn't. `text` transforms the input to a string (which it already is). But when you then insert it along with the other string (`
    • `) into the DOM via `append`, it will be converted to DOM elements, thus triggering any JavaScript it may contain. – tim May 19 '20 at 18:31
  • @dandavis In that example, `$([userinput])` already triggers the XSS (jQuery selectors are vulnerable to XSS if the first character is user-controlled and/or `<`). I think I now understand what you intended to do. Your answer was basically `list.append('
  • '+ $($("#input_field").val().bold()).text() +'
    • ');`, which will trigger the payload as a user-defined string is given to `append`. It seems that you instead intended to use `list.append('
  • '+ $(input_field.value.bold()).text() +'
    • ');`, but that will still trigger an XSS because of the selector injection. – tim May 20 '20 at 06:35
  • But I think the important part here is that IMHO your solution is looking at the problem the wrong way. The approach shouldn't be to parse the input as HTML and then somehow filter it using jquery, the approach should be to treat is as string, not HTML. This avoids any possibility of XSS *and* keeps the value intact (users may want to use tags in their string after all). so using safe accessors like `innerText` - which treats the data as string - is preferable over using accessors like `html` or `append` which treat it as data to be interpreted. – tim May 20 '20 at 06:41
  • what i intended was exactly what you code does, only shorter. one of my pet peeves is typing out _document.createElement_... ;) the code was actually an aside, the answer is just "focus on backend cleanup"... – dandavis May 20 '20 at 15:21
  • @dandavis You're right, clean code is also important; in this case my focus was only on security. If you want to avoid `createElement` you can do it via jquery, eg `var li = $('
  • ');li.text(input);li.appendTo(list);`. The important issues are just to not pass user-controlled content to the beginning of `$()` or directly to `append`, etc.
    • – tim May 20 '20 at 16:05