Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[lint] Capability field analyzer #176
base: master
Are you sure you want to change the base?
[lint] Capability field analyzer #176
Changes from 7 commits
c9706b6
9f05334
634cfcf
a3adda0
12562e9
d98af87
74665ad
8e5deb0
b14fe58
df8d514
fb685d0
c070b6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think the inner types of the restricted type would also need to be visited recursively.
e.g:
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.
Why analyze only structures, and not e.g. resources or contracts?
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.
Good question. My thought process was the following ( I mean I saw the idea of looking into structs in the Nozim's patch) -
Structures can be used as types to declare fields in contracts. So, in that way we would have a map of the structs that have a potentially dangerous public capabilities that might leak. I am referring to this test -
I think for contracts it doesn't make sense to have such a map. Or is it? I have my day off on Friday so can take a more in depth look.
For resources you are probably right as a resource might contain a public capability which links to the other resource. I need to check Cadence a bit as it's been a while since I looked into it.
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.
That makes sense. Given that a public capability fields is just as problematic in a resource or contract as it is in a struct, it might make sense to just detect it everywhere, i.e. remove the if-statement.
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.
good idea. For contracts it was a bit more tricky, I put a comment in - CollectCompositesWithPublicCapabilities.
May be should rename this function as it not only collects composites with pub capabilities but also field declarations to exclude.
I mean -
this test passes cause CollectCompositesWithPublicCapabilities puts
pub let my_capability : Capability?
in fieldsInComposite which allows to ignore it in
inspector.Preorder
. Kind of a main pass where leaks are detected.I've added a test with resources. Pls take a look
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.
Currently,
DetectCapabilityType
detects a type which is or includes theCapability
type purely on the syntax tree (ast.Type
). Maybe consider operating on the semantic level, i.e.sema.Type
.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.
What I found nice about
ast.Type
is that I could cover for "free" cases like arrays or dictionaries.I guess you saw a recursion in
DetectCapabilityType
:This way I could strip a type from
[]
or?
etc ...Thx for the suggestion. I'll take a look at
sema.Type
.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.
The same can be done for
sema.Type
, e.g. there'ssema.ConstantSizedType
,sema.DictionaryType
, etc.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.
I tried to take a look today. I see that usually ast is used when checks are simple like in reference_operator_analyser.go etc... I found in redundant_cast_analyser.go that sema is used.
The conversion from ast to sema is done like this -
redundantType := elaboration.StaticCastTypes(castingExpression)
where
castingExpression
is of*ast.CastingExpression
type. And resultingredundantType
has several fields like for example
redundantType.ExpectedType
ofsema.Type
.So, I went to elaboration.go but couldn't find any function like StaticCastTypes that would take
*ast.FieldDeclaration
as an argument. So, something like elaboration.DeclarationTypes. And I'd need to elaborate on a declaration.Do you think there is an easy way to get from
*ast.FieldDeclaration
to relevantsema.Type
s ?Also, I tried to search for
TypeAnnotation
in elaboration.go. With the idea that may beast.FieldDeclaration.TypeAnnotation
can be converted tosema.Type
by some method.Otherwise, ast works ok for this case , as far as "Capability" is reserved word by the language... But, I mean if there is a way to convert to sema types then indeed might be better.
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.
Hi @turbolent , so what do you think about my previous comment ? I couldn't find an existing function in elaboration that would allow me to convert so sema.
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.
Sorry for dropping the ball here Boris.
It's OK to leave it as-is for now. Given there's no shadowing allowed fore types, it should be sufficient to detect
Capability
purely syntactically (i.e. through the AST instead of sema).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.
We might have to do this using the sema-type, because the contract could be using types that are imported, and for them, the ast-node (
ast.Type
) might not be available at this stage.e.g:
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.
Skipping based on the name can be a bit problematic. For e.g. what if both the contract and the struct/resource have fields with the same name? Can you maybe add a test?
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.
true, I'll take a look at it today. Probably, a more elaborated key would be required like name+position or something of a sort.
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.
Or maybe instead of doing it in two iterations, perform this check in a single iteration.
i.e:
I haven't actually tried this at the code level, but just an idea that might be easier to implement.
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.
Sorry, finally didn't have time on Monday, now I am aiming Friday. (I work full time Tue, Wed, Thu)
As for using sema we had this discussion with Bastian, pls take a look at my comment
#176 (comment)
As for your proposal regarding one path.
"Once the result is found memoize it so it can be looked up if the same type is encountered again. (in the memoized mapping, Type could be the key, and the result/boolean could be the value)" - I think it's a valid optimization that would allow to limit number of runs of
DetectCapabilityType
. Although, may be there ll be difficulties in case of name collisions that you've mentioned in your previous comment. ( the trick with key - name+location won't work here I think)However, I needed 2 paths for a different reason.
1st path detects composites , so resources and structs with public capability type. However , it's not a bad pattern yet to have them declared like this. if this struct is only used as a private field in a contract that's fine.
2nd path well ignores pub capabilities inside of structs and resources but checks them in contracts and also takes structs/resources with pub capabilities from 1st path and checks if those are used to declare public fields.
Basically, pls take a look at this test -
it was reporting a false warning before I introduced 2 paths. ContractData - is only used to declare a private field so no warning should be issued. I just couldn't come up with a solution that uses one path and works for these kind of tests.
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.
In the algorithm I suggested, there will be no name collisions since field names are not looked up. Fields are visited in-place.
Here, the approach I suggested would not report any errors. Since it first (and only) visit the field
priv var contractData: ContractData
, sees that field is private, so skips.Suppose the field was public, then only it will go and check the
ContractData
type.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.
Like I mentioned in #176 (comment), we would only need to visit contract fields, no need to visit type definitions, unless it is encountered as a type of a public field.
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.
Also, how would it visit
ContractData
if needed ?May be there is a way to run
inspector.Preorder( ...
on a specific struct but I didn't find an example.Well, I am not saying that one path wont work but I just fail to see how to achieve it with my current knowledge.
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.
"Like I mentioned in #176 (comment), we would only need to visit contract fields, no need to visit type definitions, unless it is encountered as a type of a public field."
ah sorry I didn't understand that you proposed to ignore definitions.
I don't know how to ignore type definitions but I imagine there should be a way to filter them out together with resources. However, if I would need to analyze
ContractData
how I can make a path through it ?Is there a way to run an inspector on a specific struct definition ?
Other thing, the advantage of having two paths - all structs with pub capabilities are stored in a map so there ll be no need to analyze them on each encounter.
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.
pass.Program
already has theast
nodes as well as type info to look up. So when you know a type (say,ContractData
), it can be retrieved from thepass.Program
. Here you can either retrieve the ast-type-definition (frompass.Program.Program
) or the sema-type (frompass.Program.Elaboration
)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.
thx for the suggestion. For now I don't understand how to transit from
ast-type-definition (in
pass.Program.Program
) to the "reduced" inspector so to be able to runfunc CollectCompositesWithPublicCapabilities(inspector *ast.Inspector) (map[string]struct{}, map[ast.Identifier]struct{}) {
on an inspector that only is a result of this specific node. But I'll take a look.
Other note - anyway I think caching in the map is nice as this would allow not to re-analyze the same structs if used to declare several fields in different places of a contract.