-1

Is this Java method good for protecting against cross-site scripting? It is a Servlet running in Jetty. The input comes from the user's HTTP request (we need to do stuff with the URI, parameter values, header values, and body), we clean it, and the output is a reflection of the cleaned input as HTTP response for the user's browser.

String clean(String tainted) {
    String cleaned = "";
    for (int i = 0; i < tainted.length(); i++) {
        char c = tainted.charAt(i);
        switch (c) {
            case '&': cleaned += "&amp;"; break;
            case '<': cleaned += "&lt;"; break;
            case '>': cleaned += "&gt;"; break;
            default:
                if (Character.isISOControl(c) && !Character.isWhitespace(c)) {
                    cleaned += '?';
                } else {
                    cleaned += c;
                }
        }
    }
    return cleaned;
}

String doStuff(String clean) {...}

return "<!DOCTYPE html><html><body>" + doStuff(clean(userInput)) + "</body></html>";
Hackme
  • 1
  • 1
  • `doStuff()` cannot be black box if the question is whether the code is vulnerable to XSS. It may be vulnerable, if `doStuff()` does certain things. But the main thing is, even if `doStuff()` is harmless, the code above will still lead to XSS eventually on a complex enough codebase in a complex enough organization. – Gabor Lengyel Feb 16 '18 at 00:23
  • @GaborLengyel, the question is not whether doStuff() is vulnerable. The question is whether clean() is vulnerable, and how. To make it simpler, suppose doStuff() does nothing but echo the input. Or even simpler, suppose doStuff() doesn't even exist, remove it, suppose only clean() exists. What's the vulnerability? – Hackme Feb 16 '18 at 02:29
  • @GaborLengyel, I hadn't read the second part of your answer. Supposing the method clean() doesn't change over time, how will the code above "lead to XSS eventually"? Do you have an example? And how does "a complex enough codebase" and "complex enough organization" will make it vulnerable? Do you have details? – Hackme Feb 16 '18 at 03:52

1 Answers1

2

Security is a difficult battle best left to people whose job it is to keep up to date on such matters. OWASP provides a Java encoding library which would take care of such matters for you (you can find it with a quick Google search).

alephtwo
  • 322
  • 2
  • 10
  • Yes, I agree, there are libraries like OWASP ESAPI, Jsoup, etc. However, the question is about the code above. Is it good enough? Can it be attacked? My developers are arguing it is good. I need to give them evidence if otherwise. – Hackme Feb 15 '18 at 22:16
  • 1
    OWASP also provides a cheat sheet for things like DOM based XSS prevention. I would cross check it with that and see if there are any obvious failure points. When in doubt, write some tests. – alephtwo Feb 15 '18 at 22:18
  • 1
    I read it, did some tests, couldn't attack it, hence posting it here for others to attack, if vulnerable. – Hackme Feb 15 '18 at 22:22
  • 1
    @Hackme - Here's the thing. If you ask some random folks on the internet (StackOverflow) for a **critical** security advice ... how do you know that the advice you get is valid? IMO, you will be better off doing your own research on this, and if you don't feel competent, then employing a reputable independent security consultant. (And in this case, it seems that you are preferring the advice of "randoms" over the advice of your in-house (semi-)experts .... your developers.) – Stephen C Feb 15 '18 at 22:26
  • @StephenC, Not really. If there is a vulnerability, then it's easy to prove the developers wrong. If the "randoms" here on StackOverflow can't find an attack, it does not mean the code is not vulnerable, it just means I have to keep researching. – Hackme Feb 15 '18 at 22:34
  • 1
    @Hackme - so regardless of the answer you'll need to keep researching. Why not use a library that has been tested and viewed by many people? As an observation, the code you show would replace a legitimate "&" with "&amp;" making the URL invalid. – stdunbar Feb 16 '18 at 00:02
  • @stdunbar, no, if there's an attack possible, then there's no need to keep researching. And yes, the code is buggy for your scenario. Yet, we don't know if it's vulnerable to XSS in the context of an HTML element content, or not. – Hackme Feb 16 '18 at 01:55
  • @stdunbar, I forgot to answer your other question about why not use a library. I had answered that to alephtwo earlier. The developers of this code are saying the code is protected against XSS for our scenario, albeit buggy as you pointed out. I have a huntch that it's vulnerable, but I don't know how. I'm looking for the description of an attack, to convince the developers of the code that their code is vulnerable, and to learn; we have many teams doing the same type of code. – Hackme Feb 16 '18 at 02:33