Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.17.5 Upgrade #16744

Closed
wants to merge 139 commits into from
Closed

Conversation

dilanbhalla
Copy link
Contributor

No description provided.

Dilan Bhalla and others added 30 commits April 24, 2023 14:46
Manual Merge: C# ZipSlip Conflict
Compatible with the latest released version of the CodeQL CLI
This pr is auto merged as it contains a mandatory file and is opened for more than 10 days.
Compatible with the latest released version of the CodeQL CLI
Compatible with the latest released version of the CodeQL CLI
Compatible with the latest released version of the CodeQL CLI
Compatible with the latest released version of the CodeQL CLI
Compatible with the latest released version of the CodeQL CLI
Copy link
Contributor

QHelp previews:

csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp

Arbitrary file access during archive extraction ("Zip Slip")

Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated.

Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (..). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

For example, if a zip file contains a file entry ..\sneaky-file, and the zip file is extracted to the directory c:\output, then naively combining the paths would result in an output file path of c:\output\..\sneaky-file, which would cause the file to be written to c:\sneaky-file.

Recommendation

Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.

The recommended way of writing an output file from a zip archive entry is to conduct the following in sequence:

  1. Use Path.Combine(destinationDirectory, archiveEntry.FullName) to determine the raw output path.
  2. Use Path.GetFullPath(..) on the raw output path to resolve any directory traversal elements.
  3. Use Path.GetFullPath(destinationDirectory + Path.DirectorySeparatorChar) to determine the fully resolved path of the destination directory.
  4. Validate that the resolved output path StartsWith the resolved destination directory, aborting if this is not true.
    Another alternative is to validate archive entries against a whitelist of expected files.

Example

In this example, a file path taken from a zip archive item entry is combined with a destination directory. The result is used as the destination file path without verifying that the result is within the destination directory. If provided with a zip file containing an archive path like ..\sneaky-file, then this file would be written outside the destination directory.

using System.IO;
using System.IO.Compression;

class Bad
{
    public static void WriteToDirectory(ZipArchiveEntry entry,
                                        string destDirectory)
    {
        string destFileName = Path.Combine(destDirectory, entry.FullName);
        entry.ExtractToFile(destFileName);
    }
}

To fix this vulnerability, we need to make three changes. Firstly, we need to resolve any directory traversal or other special characters in the path by using Path.GetFullPath. Secondly, we need to identify the destination output directory, again using Path.GetFullPath, this time on the output directory. Finally, we need to ensure that the resolved output starts with the resolved destination directory, and throw an exception if this is not the case.

using System.IO;
using System.IO.Compression;

class Good
{
    public static void WriteToDirectory(ZipArchiveEntry entry,
                                        string destDirectory)
    {
        string destFileName = Path.GetFullPath(Path.Combine(destDirectory, entry.FullName));
        string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
        if (!destFileName.StartsWith(fullDestDirPath)) {
            throw new System.InvalidOperationException("Entry is outside the target dir: " +
                                                                                 destFileName);
        }
        entry.ExtractToFile(destFileName);
    }
}

References

TaintTrackingConfiguration() { this = "ZipSlipTaintTracking" }
class RootSanitizerMethodCall extends SanitizerMethodCall {
RootSanitizerMethodCall() {
exists(MethodSystemStringStartsWith sm | this.getTarget() = sm) and

Check warning

Code scanning / CodeQL

Expression can be replaced with a cast Warning

The assignment to
sm
in the exists(..) can replaced with an instanceof expression.
module GetFullPathToQualifierTT =
TaintTracking::Global<GetFullPathToQualifierTaintTrackingConfiguration>;

private module GetFullPathToQualifierTaintTrackingConfiguration implements DataFlow::ConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming Warning

Modules implementing a data flow configuration should end in Config.
*/
abstract class Sanitizer extends DataFlow::ExprNode { }
private module PathCombinerToGetFullPathTaintTrackingConfiguration implements DataFlow::ConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming Warning

Modules implementing a data flow configuration should end in Config.
* ...
* }
*/
private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::ConfigSig {

Check warning

Code scanning / CodeQL

Data flow configuration module naming Warning

Modules implementing a data flow configuration should end in Config.
exists(Expr q, AbstractValue v |
this.getQualifier() = q and
v.(AbstractValues::BooleanValue).getValue() = true and
exists(MethodCallGetFullPath mcGetFullPath | safeCombineGetFullPathSequence(mcGetFullPath, q))

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
not exists(node.getASuccessor())
}

/** A FlowStack encapsulates flows between a source and a sink, and all the pathways inbetween (possibly multiple) */

Check warning

Code scanning / CodeQL

Misspelling Warning

This comment contains the common misspelling 'inbetween', which should instead be 'between'.
Comment on lines +145 to +147
/**
* Get the first frame in the DataFlowStack, irregardless of whether or not it has a parent.
*/

Check warning

Code scanning / CodeQL

Misspelling Warning

This comment contains the common misspelling 'irregardless', which should instead be 'regardless'.
Comment on lines +342 to +344
/**
* A user-supplied predicate which given a Stack Frame, returns some Node associated with it.
*/

Check warning

Code scanning / CodeQL

Predicate QLDoc style. Warning

The QLDoc for a predicate with a result should start with 'Gets'.
@@ -115,6 +115,10 @@
(result.matches("RSA") implies not f.getName().toUpperCase().matches("%UNIVERSAL%")) and
//rsaz functions deemed to be too low level, and can be ignored
not f.getLocation().getFile().getBaseName().matches("rsaz_exp.c") and
// SHA false positives
(result.matches("SHA") implies not f.getName().toUpperCase().matches("%SHAKE%")) and

Check notice

Code scanning / CodeQL

Use of regexp to match a set of constant string Note

Use string comparison instead of regexp to compare against a constant set of string.
// SHA false positives
(result.matches("SHA") implies not f.getName().toUpperCase().matches("%SHAKE%")) and
// CAST false positives
(result.matches("CAST") implies not f.getName().toUpperCase().matches(["%UPCAST%", "%DOWNCAST%"])) and

Check notice

Code scanning / CodeQL

Use of regexp to match a set of constant string Note

Use string comparison instead of regexp to compare against a constant set of string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants