False positive for rule typescript:S4036

Rules and Languages Report False-positive / False-negative...
1
  • What language is this for: typescript
  • Which rule: typescript:S4036
  • Why do you believe it’s a false-positive/false-negative:
    The rule states that I need ‘Make sure the “PATH” used to find this command included only what you intend’. I believe having the PATH overridden in the command call itself should be enough to fulfill that rule but it demands that I define only one path to the command inline in the call.
  • SonarQube Server / Community Build version 2025.1
  • How can we reproduce the problem? Give us a self-contained snippet of code:
import { execSync } from "child_process";
                                              
function getBranchName(): string {            
  try {                                       
    return execSync("git rev-parse --abbrev-ref HEAD", {
      env: {                                             
        PATH: "/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin"
      }                                                                         
    }).toString();                                                              
  } catch(error: unknown) {                                                     
    if(                                                                         
      error &&                                                                  
      typeof error === "object" &&                                              
      "stderr" in error &&                                                      
      error.stderr instanceof Buffer                                            
    ) {                                                                         
      throw new Error(error.stderr.toString());                    
    }                                                            
                                                                 
    throw new Error("Unknown Error"); 
  }
}

I also think this should be acceptable:

import { execSync } from "child_process";
                                              
function getBranchName(): string {            
  try {                                       
    return execSync("PATH='/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin' git rev-parse --abbrev-ref HEAD").toString();                                                              
  } catch(error: unknown) {                                                     
    if(                                                                         
      error &&                                                                  
      typeof error === "object" &&                                              
      "stderr" in error &&                                                      
      error.stderr instanceof Buffer                                            
    ) {                                                                         
      throw new Error(error.stderr.toString());                    
    }                                                            
                                                                 
    throw new Error("Unknown Error"); 
  }
}
6

Hi Lukas, and welcome to our community.

I think we totally need to modify our text and descriptions, because it’s not clear what we are trying to achieve when we give you this issue.
Your use case is a 100% relevant and we should add it.

Here is the intention we want to convey and with which I’ll update the rule:

  1. This issue a security hotspot, which could be interpreted as a “potential source of weakness in your code”, while if the issue was a vulnerability, it would be interpreted as “There is a vulnerability, we are 100% sure of it.”. So it does not mean that it should be modified, just that you should take a step back and think about the environment around this piece of code.
  2. With this rule, we want to help you protect your code against the presence of rogue executables in your PATH. And if you hardcode your path, it’s great because you are somehow protected against potential PATH alteration, but: what if malicious executables are also within this new PATH?

So, for your piece of code in particular, here is (approximately) the course of action that we try to encourage:

  1. Code raises a hotspot for S4036
  2. Check the PATH, what paths does it contain? (even better because it’s clearer if you overrode the PATH within the code)
  3. For each paths within the PATH variable:
    1. Are they restricted in writing to only the UNIX user running this typescript code? For example node
    2. Is this user also shared with another backend? If yes, this means that if a malicious user of the other app (not UNIX this time) drops a malicious executable onto one of these folders, it can impact this TS code.

If you determine that these folders cannot be altered, and that you are sure that the git program you intended to use will be used, then you can determine that the risks are under your control and you can set the status of your hotspot as “safe”.

However, if these steps are too long to do, or business logic of your organization blocks you from following them, then a very straightforward solution can be used: calling git with its absolute path. Into the production environment this means

$ whereis git
git: /usr/bin/git /usr/share/man/man1/git.1.gz
$ ls -l /usr/bin/git
-rwxr-xr-x 1 root root 3376112 Jan 28 10:13 /usr/bin/git

and using /usr/bin/git, and making sure it is not writeable by someone other than root, and you are good to go.

I am adding all of that in the rule description. And try to make it concise so that people actually read it :laughing: Thanks a lot!

Cheers,

Loris