-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: Add taint models for the Data class #11345
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
I've made a few minor suggestions, none of them should be considered blocking.
let stringTainted = String(data: source3(), encoding: String.Encoding.utf8) | ||
|
||
sink(arg: stringClean!) | ||
sink(arg: stringTainted!) // $ MISSING: tainted=100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the .utf8
case earlier) is presumably a job for the String
model rather than Data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why I moved them to the string.swift
file (they were previously on data.swift
). They ought to be fixed by #11185.
let dataTainted20 = source() as! Data | ||
let pointerTainted20 = UnsafeMutablePointer<UInt8>.allocate(capacity: 0) | ||
dataTainted20.copyBytes(to: pointerTainted20, from: 0..<1) | ||
sink(arg: pointerTainted20) // $ MISSING: tainted=153 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any idea what's gone wrong in these two cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really :(
I think it may have to do with the type UnsafeMutablePointer
, but I'm surprised such a direct flow doesn't work regardless of the type of the tainted object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should create an issue to investigate this, though I'm a little reluctant to prioritize it above all the other stuff.
3b0a001
to
e2afeeb
Compare
b114dc4
to
fc7c66d
Compare
Rebased to remove the special-cased additional taint step for the |
arg = | ||
any(CallExpr ce | ce.getStaticTarget().(MethodDecl).hasQualifiedName("Data", "init(_:)")) | ||
.getArgument(0) | ||
or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to merge I think.
No description provided.