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

Jb1/zipslip performance fix upstream #15558

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ropwareJB
Copy link
Contributor

@ropwareJB ropwareJB commented Feb 8, 2024

Original: #13281 (with performance improvements)

Summary

  • Patched False negative case with original GH query (see bottom of post)
  • Patched False positive case with sanitizer wrappers (see below code example)

Changes

  • Declares String concatenation as an unsanitized path combination vector
  • Declares String interpolation as an unsanitized path combination vector
  • Creates a class AbstractWrapperSanitizerMethod which identifies the set of methods which act as a more restrictive subset of System.String.StartsWith which then acts as a SanitizerGuard.
  • Various helper subclasses / alias classes
  • Patched False negative case with original GH query

Info

An AbstractWrapperSanitizerMethod is a Method where

  • The Method has a MethodCall to a given class of Sanitizers
  • The Method can /only/ return true if it first passes through the RootSanitizerGuard
  • Or if it returns the resultant of the RootSanitizerGuard verbatim.
  • Or it simply wraps another AbstractWrapperSanitizerMethod

For example:

bool wrapperFn(a,b){
  if(guard(a,b))
    return true
  ....
  return false
}

bool wrapperFn(a,b){
  ...
  return guard(a,b)
}

However there is a defect with the existing Taint flow configuration's isSanitizer predicate: the stringCheckGuard predicate looks for String.StartsWith method calls with a qualifier /that did not directly/ come from a call to Path.Combine. Additionally, there is no check for path canonicalization with Path.GetFullPath. Therefore, code of the following stanza will be declared as a guard and assumed to be sanitized by the GH query.

image
Above: The stringCheckGuard predicate erroneously declares the String.StartsWith MethodCall as a valid sanitizer.

To address this, I declared a class RootSanitizerMethodCall, which will locate instances of String.StartsWith whose qualifiers directly flow from Path.GetFullPath or have a safe sequence of Path.Combine and then Path.GetFullPath permitting further mutations following.

@ropwareJB ropwareJB requested a review from a team as a code owner February 8, 2024 16:44
@github-actions github-actions bot added the C# label Feb 8, 2024
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

1 participant