1

I have a function to which I pass a file name suppose the file name is of the following format 'abc-zyx-anony-detect-v1.zip' , it can contain special characters as _ or -

Now when we run the semgrep scan on it ,the scan shows an error as

  javascript.lang.security.audit.detect-non-literal-fs-filename.detect-non-literal-fs-filename
        Detected function `artFacts` enter fs module. An attacker could  potentially control the
        location of this file, to include going  backwards in the directory with '../'. To address
        this,  ensure that user-controlled variables in file paths are validated.

to fix this , I read through few articles and tried the methods specified to fix the issue,the things that I tried out are

1.tried to remove ../../ added before the path, but no luck this didnt work

    public async artFacts(artfact:Artfact): Promise<Artfact> {
           const artfactName = artfact.getName();
           const getName = artfactName.replace(/^(\.\.(\/|\\|$))+/, '')

           fspromise.readFile(path.join(`${envConfig.artfacts_container_path}`,getName)){
               //some logic
           }
    
       }

 const artfactName = artfact.getName();
           const getName = artifactName.replace(/^(\.\.(\/|\\|$))+/, '')
           const realPath = await fspromise.realpath(path.join(`${envConfig.artfacts_container_path}`,getName));
           fspromise.readFile(path.join(`${envConfig.artfacts_container_path}`,realPath)){
               //some logic
           }
  1. In the above approach tried to canonicalise the path but no luck with this approach as well

can someone please guide me as to how to fix this semgrep scan issue, thanks in advance for help.

Swarup
  • 11
  • 4

1 Answers1

1

The detect-non-literal-fs-filename semgrep rule keep track of whether a function parameter is passed to a fs file handling function, such as readFile. This is indeed the case in your code example.

This rule does not specify any sanitizers, which would make semgrep recognize that the value is safe. So no matter how you manipulate artfactName, semgrep will still trigger on your code. In this case, the semgrep rule is not smart enough to detect that you have solved the issue. This semgrep rule is meant to point out a possible vulnerability to you, and you are supposed to look into it yourself to determine whether it is still a problem or not.

artfactName.replace(/^(\.\.(\/|\\|$))+/, '')

This removes ../, ..\, and .. at the end of the line. This is not sufficient to solve the issue. Consider what happens with the filename ....//somefile.txt; ../ is removed, and the result is ../somefile.txt.

Perhaps a better approach is to throw an error when detecting ../, or take everything after the last /.

Sjoerd
  • 74,049
  • 16
  • 131
  • 175