Hello SonarQube Community.
We are using SonarQube Enterprise 9.9.1 deployed in a Docker container.
Currently I’m analysing SonarQubes ability to find DOM based XSS Vulnerabilities in Typescript Code. Especially in Angular Applications. There are already some rules that cover those types of vulnerabilities I am after:
- S5696 DOM updates should not lead to cross-site scripting (XSS) attacks
- S5334 Dynamic code execution should not be vulnerable to injection attacks
My analysis show, that those rules from the “tssecurity” Repository try to prevent as much false positives as possible. Since they are of type vulnerability this is perfectly fine. But this also means, that the chance to miss out a actually vulnerable line of code is pretty high.
I created a quick test project using Angular, that contains a lot of those vulnerabilities, and scanned it with SonarQube. And I was really surprised, that not a single vulnerability was dedected by those rules. I started digging deeper and played around a bit with the code, and found out that this was due to the way I wrote my code. In Angular it is pretty common to encapsulate code into services and Inject it into Components or other Services. I used the same approach to structure my code. This was the reason, why the vulnerabilities where not dedected.
Examples
Lets have a look at a few examples. All of them use the following DataService that has a single “location” method. This method accesses the location hash, that is controllable by the user. And could lead to XSS, if used in a unsafe context.
@Injectable({
providedIn: 'root',
})
export class DataService {
location(): string {
return location.hash.substring(1);
}
}
S5334 setTimeout, setInterval
@Component({
selector: "app-root",
templateUrl: "./app.component.html",
styleUrls: ["./app.component.scss"],
})
export class AppComponent implements OnInit, AfterViewInit {
constructor(public dataService: DataService) {}
ngOnInit(): void {
setTimeout(this.dataService.location(), 1000); // not detected by S5334
setTimeout(location.hash.substring(1), 1000); // detected by S5334
}
}
Both setTimeout are invoced with the exact same string parameter. But the rule does not detect the first call. Because it does not treat this.dataService.location() as a unsafe source.
S5696 innerHTML
@Component({
selector: "app-root",
templateUrl: "./app.component.html",
styleUrls: ["./app.component.scss"],
})
export class AppComponent implements OnInit, AfterViewInit {
@ViewChild('playground', { static: true })
playground?: ElementRef<HTMLDivElement>;
constructor(public dataService: DataService) {}
ngAfterViewInit(): void {
const div1 = document.createElement("div");
div1.innerHTML = this.dataService.location(); // Not detected by S5696
document.body.appendChild(div1);
const div2 = document.createElement("div");
div2.innerHTML = location.hash.substring(1); // Detected by S5696
document.body.appendChild(div2);
const element: HTMLDivElement = this.playground.nativeElement;
element.innerHTML = location.hash.substring(1); // Not detected by S5696
}
}
Same as before. div1.innerHTML is not detected because the dataService is not treated as unsafe source. Also element.innerHTML is not dtected. The rule might not realise, that we call innerHTML on an actual DOM element as it comes from an Angular @ViewChild.
Expectation
I would like to have all cases of dangerous calls or assignments to be reported by SonarQube. Regardless of the values assigned to them (With the exception of literal strings. Those should be fine). I think Security Hotspots are the perfect fit for those kind of vulnerabilities. A Developer has to review the code, and decide if there is a problem or not. Especially when using Angular or other modern frameworks, there should not be the need to use those unsafe constructs very often. And if you do, there is probalby something wrong with the code.
The examples above are very simple. But in a real live scenario, the data might be loaded from a server, and then be passed to a unsafe sink like innerHTML. No tool would be able to say, if the data returned from the server contains user controlled data or not. In that case, a Security Hotspot is the perfect solution.
I would like to hear your opinion on this topic.
Is it already knwon, that the rules have those limitation?
Are there any plans to extend those rules?
Or maybe the exiting Rules that find vulnerabilities for certain, and Hotspot generating Rules can coexist?
I would also be happy to contribute some ESLint rules to the SonarJS repository when desired.
