False Positive - cs/zipslip

[ropwareJB]

Fixes false positive whereby there is a barrier that correctly sanitizes the path and throws on failure:

        /**
        * Negative - no extraction, only sanitization
        */
        static void fp_throw_sanitizer_valid(string file, string root){
            string destinationOnDisk = Path.GetFullPath(file);
            string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
            if (!destinationOnDisk.StartsWith(fullRoot)){
                throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename.");
            }
        }

        /**
        * Negative - dangerous path terminates early due to throw in fp_throw_sanitizer_valid
        */
        static void fp_throw_nested_exception_uncaught(ZipArchive archive, string root){
            foreach (var entry in archive.Entries){
                string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
                string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
                fp_throw_sanitizer_valid(destinationOnDisk, fullRoot);
                entry.ExtractToFile(destinationOnDisk, true);
            }
        }

[MathiasVP]

The changes LGTM! Should we do a DCA run on it as well?

[MathiasVP]

So this is only checking one level. So if you had a method which validated a path by forwarding to another method then this wouldn't work, right? It's comforting to know that we could easily make this recursive if we start getting FP reports on more complicated interprocedural barriers.

Sidenote: I know GitHub has recently done some work on making this Just Work when you define a barrier guard. Here's the PR that does it for Java, and I hope that the C# one is also coming soon 🤞 That would make these complexities unnecessary.

[ropwareJB]

Yes, just one level! Up until now I have only seen false positive instances of one-level of santiziation, and no wrappers-of-wrappers.

Oh that PR you've linked is awesome, will have to keep an eye out for that in C#

[ropwareJB]

The changes LGTM! Should we do a DCA run on it as well?

We can run DCA on it with the PR on the main repository, but there is a known performance issue with this query already that I could use some pointers on. This PR at least satisfies the false positives that have been submitted, and I was going to follow on with a subsequent PR addressing performance