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

Swift: Fix defaultImplicitTaintRead on fields #14661

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Nov 1, 2023

Fix a bug in the defaultImplicitTaintRead mechanism for reading from field access paths at sinks. Assuming source() is a taint source and Mid.field is a sink (say, defined through models-as-data), consider:

class Base { }

class Mid : Base {
  var field : String
}

class Derived : Mid {
}

...

base.field = source() // case 1
mid.field = source() // case 2
derived.field = source() // case 3
  • case 1 is impossible, as Base does not have a field.
  • case 2 is the normal case. Taint flows from source() to the access of mid with an access path for .field. defaultImplicitTaintRead recognizes that the sink is an access to Mid and permits any field of Mid to be stripped off the access path, and we get a result.
  • in case 3, once again taint flows from source() to the access of derived with an access path for .field. But this time defaultImplicitTaintRead sees that the sink is an access to Derived and permits any field of Derived to be stripped off the access path. That's not good enough, it needs to consider fields of base classes of Derived as well in order to find Mid.field.

In fact we had logic for this, but it was implemented the wrong way around.

TL;DR: getABaseType*() was in the wrong place. Moving it fixes some sinks.

I definitely want to do a DCA run on this one.

@geoffw0 geoffw0 added the Swift label Nov 1, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner November 1, 2023 19:37
@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 6, 2023

DCA run LGTM. There's an analysis speedup on the run, which is plausible given the nature of this PR - but more likely to be just wobble.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This LGTM!

@MathiasVP MathiasVP merged commit 68e7f84 into github:main Nov 8, 2023
19 checks passed
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.

2 participants