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

tainting: Introduce a special kind of sanitizer #4033

Merged

Conversation

IagoAbal
Copy link
Collaborator

In PA-402 we made all sanitizers "not-conflicting" so that one could
declare $F(...) as a sanitizer without conflicting with sources and
sinks. That makes sense in that particular use case, but it partially
breaks our ability to simulate sanitizers by side effect. For example:

pattern-sanitizers:
  - patterns:
      - pattern-inside: |
          $JWT.verify($TOKEN, ...)
          ...
      - pattern: $TOKEN
pattern-sinks:
  - patterns:
      - pattern: $JWT.decode($TOKEN, ...)
      - pattern: $TOKEN
pattern-sources:
  - pattern: $TOKEN

no longer works on this example:

jwt.verify(token, key);
// ok: test
jwt.decode(token, true);

Since the second token is both matched by a sanitizer and a sink (and
a source), the sanitizer is now "disabled" due to the changes made by
0b35ab5 in connection with PA-402. This is not at all desirable, but
we still want the ability to declare $F(...) as a sanitizer. Then the
simplest solution seems to be to support two different kinds of
sanitizers.

Closes: PA-455
Changes: 0b35ab5 ("tainting: Filter out sanitizers that conflict with sources/sinks (#3958)")

test plan:
make test # tests included

PR checklist:

  • Documentation is up-to-date
  • Changelog is up-to-date
  • Change has no security implications (otherwise, ping security team)

@linear
Copy link

linear bot commented Oct 11, 2021

PA-455 Re-think "sanitizers cannot conflict with sources/sinks"

Consider

rules:
  - id: test
    languages:
      - javascript
      - typescript
    message: Test
    mode: taint
    pattern-sanitizers:
      - patterns:
          - pattern-inside: |
              $JWT.verify($TOKEN, ...)
              ...
          - pattern: $TOKEN
    pattern-sinks:
      - patterns:
          - pattern: $JWT.decode($TOKEN, ...)
          - pattern: $TOKEN
    pattern-sources:
      - pattern: $TOKEN
    severity: WARNING

and

jwt.verify(token, key);
// ok: test
jwt.decode(token, true);

Here we get a match because token is already a source and a sink, so it cannot be a sanitizer... This makes more sense when we talk about $F(...) but here I'm not so sure this is intuitive.

In PA-402 we made all sanitizers "not-conflicting" so that one could
declare `$F(...)` as a sanitizer without conflicting with sources and
sinks. That makes sense in that particular use case, but it partially
breaks our ability to simulate sanitizers by side effect. For example:

    pattern-sanitizers:
      - patterns:
          - pattern-inside: |
              $JWT.verify($TOKEN, ...)
              ...
          - pattern: $TOKEN
    pattern-sinks:
      - patterns:
          - pattern: $JWT.decode($TOKEN, ...)
          - pattern: $TOKEN
    pattern-sources:
      - pattern: $TOKEN

no longer works on this example:

    jwt.verify(token, key);
    // ok: test
    jwt.decode(token, true);

Since the second `token` is both matched by a sanitizer and a sink (and
a source), the sanitizer is now "disabled" due to the changes made by
0b35ab5 in connection with PA-402. This is not at all desirable, but
we still want the ability to declare `$F(...)` as a sanitizer. Then the
simplest solution seems to be to support two different kinds of
sanitizers.

Closes: PA-455
Changes: 0b35ab5 ("tainting: Filter out sanitizers that conflict with sources/sinks (#3958)")

test plan:
make test # tests included
@IagoAbal IagoAbal force-pushed the iago/pa-455-re-think-sanitizers-cannot-conflict-with branch from 8bdfb0c to 18e6c27 Compare October 12, 2021 09:13
@IagoAbal IagoAbal merged commit 4f61945 into develop Oct 12, 2021
@IagoAbal IagoAbal deleted the iago/pa-455-re-think-sanitizers-cannot-conflict-with branch October 12, 2021 09:34
ievans pushed a commit that referenced this pull request Oct 19, 2021
In PA-402 we made all sanitizers "not-conflicting" so that one could
declare `$F(...)` as a sanitizer without conflicting with sources and
sinks. That makes sense in that particular use case, but it partially
breaks our ability to simulate sanitizers by side effect. For example:

    pattern-sanitizers:
      - patterns:
          - pattern-inside: |
              $JWT.verify($TOKEN, ...)
              ...
          - pattern: $TOKEN
    pattern-sinks:
      - patterns:
          - pattern: $JWT.decode($TOKEN, ...)
          - pattern: $TOKEN
    pattern-sources:
      - pattern: $TOKEN

no longer works on this example:

    jwt.verify(token, key);
    // ok: test
    jwt.decode(token, true);

Since the second `token` is both matched by a sanitizer and a sink (and
a source), the sanitizer is now "disabled" due to the changes made by
0b35ab5 in connection with PA-402. This is not at all desirable, but
we still want the ability to declare `$F(...)` as a sanitizer. Then the
simplest solution seems to be to support two different kinds of
sanitizers.

Closes: PA-455
Changes: 0b35ab5 ("tainting: Filter out sanitizers that conflict with sources/sinks (#3958)")

test plan:
make test # tests included
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants