[JS] 38 while_break testability pattern

[ManuManu97]

The source code of pattern number 38 is the following:

var http = require('http');
var fs = require('fs');
var route = require('url');
const querystring = require('querystring');

function handleServer(req, res){
    var path = route.parse(req.url, true);

    if(req.url === '/'){
        res.writeHead(200, {"Content-Type" : "text/html"});
        fs.createReadStream('./index.html').pipe(res);
    }else if(path.pathname === '/query/'){
        console.log(req.method);

        //PATTERN CODE {1} 
        const parsed = route.parse(req.url);
        const query  = querystring.parse(parsed.query);
        let b = query.name;

        res.writeHead(200, {"Content-Type" : "text/html"});
        res.write(F(b));
        res.end();

    }else{
        res.writeHead(404, {"Content-Type": "text/plain"});
        res.end('Page not found');
    }
}

http.createServer(handleServer).listen(8080);
console.log('Server running on port 8080.');

//PATTERN CODE {2}
function F(val){
    let return_value='returned_value';
    let index = 0;
    while(true){
        index ++;
        if(index === 1){
            break;
        }
        //dead code
        return_value = val;
    }
    return return_value;
}

I have some doubts regarding the following:

1. In the description file/section there isn't an explanation of this tarpit, so which is the tarpit here? Because I think that the tarpit could be:
   • a break inside a while loop.
   • a break inside if which is inside while loop.
   • a break in general
2. In this case since the code is broken (dead code) and the vuln is unreachable, is there source and sink? Should I fix the code in this instance? or should I leave this code untouched?
3. I didn't really understand if this case is the injection-skeleton-broken=true, but if the skeleton is broken... isn't it better to fix the code instead of changing the boolean value?
4. I wrote the following discovery rule based on what I understood:

   Discovery rule -> "cpg.break.inAst.isControlStructure.controlStructureType("IF").inAst.isControlStructure.controlStructureType("WHILE").toJson"

   but could be a problem if joern with the ControlStructure doesn't give the line number in the query response?

5. If I will change the code of the function F() in the following way:

   function F(val){
   let return_value='returned_value';
   let index = 0;
   while(true){
       index ++;
   
       return_value = val;
       if(index === 1){
           break;
       }
   }
   return return_value;

   I think the discovery rule that I quoted in point 4 found also this case, but in the README.md the "correct" result is "Not vulnerable" and changing in this way the code becomes clearly vulnerable, in this case, what should I do?

[SoheilKhodayari]

In the description file/section there isn't an explanation of this tarpit, so which is the tarpit here?

@ManuManu97 I agree, I I think in general the description of the patterns that we have either does not exist in the repo or is very minimal and only describes the JS feature used by the program, instead of the type of problem the pattern creates for SAST tools, and we should fix them.

In this case, SAST tools may wrongly assume that there exists an execution of the program in which function F(v) returns v (i.e., identity function), resulting in an attacker-controllable data flow. However, because of the break statement that execution flow never happens. In general, the problematic JS feature for this testability pattern is the use of any break statements inside a while loop.

In this case since the code is broken (dead code) and the vuln is unreachable, is there source and sink? Should I fix the code in this instance? or should I leave this code untouched?

I think there is no problem with the code snippet. Yes, the vulnerable code statement is unreachable and a manual look at the code can reveal that, but when this example program is given to SAST tools, it may result in false positives and they may report a vulnerability that does not actually exists (i.e., this is a negative example). Accordingly, the source is req.url, sink is res.write() and the tarpit is the while-break inside the function F(v).

I didn't really understand if this case is the injection-skeleton-broken=true

Theinjection-skeleton-broken flag should be false clearly because the source-tarpit-sink pattern is there.

I wrote the following discovery rule

I think we can remove the if part from the query. Not sure exactly what's the problem with the line number, but I suspect you should write .location.toJson at the end instead of only toJson, no? Maybe @pr0me knows more.

change the code of the function F()

I think we should not change this example (it is a negative case). Instead, we can create a new instance/example containing a positive case where the vulnerability actually exists, if we don't have that example already.

[SoheilKhodayari]

@ManuManu97 btw, it seems that reviewing this pattern is assigned to CISPA (see, i.e., this issue). To avoid merge conflicts later, kindly check the issue in the sast-tp-framework repo for the updated list of the patterns you are expected to review, thanks 😉

[ManuManu97]

Yes, with the new assignment is assigned to CISPA, but first I saw that you had made the 39 instead of the 38 so I started working on the 38. No problem, I apologize and I'll start working on the others that have been assigned to me, these are still some doubts that can also be found in other instances, thanks anyway for your answer!