[lint] Capability field analyzer

[kozlovb]

Add's a public capability field analyzer

Closes ##6

this is a joint work with @nozim

with his authorization I took code from two of his PRs

#163
#164

I took the tests and his idea to separately analyze the structs.

[turbolent]

@kozlovb Just to clarify: Does that mean that this PR subsumes #163 and #164, i.e., should those two other PRs be closed?

[kozlovb]

@turbolent thx for getting back to us. Yes, I believe so. @nozim is a co-author of this one. @nozim pls confirm that those can be closed as this one has your work merged with mine.

[nozim]

Yes I believe so, @kozlovb already implemented those cases in his branch.

[turbolent]

Nice!

[turbolent]

Currently, DetectCapabilityType detects a type which is or includes the Capability type purely on the syntax tree (ast.Type). Maybe consider operating on the semantic level, i.e. sema.Type.

[kozlovb]

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 :

	case *ast.ConstantSizedType:
		return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities)
	case *ast.DictionaryType:
		return DetectCapabilityType(downcastedType.KeyType, structWithPubCapabilities) || DetectCapabilityType(downcastedType.ValueType, structWithPubCapabilities)
	case *ast.FunctionType:
		return false

This way I could strip a type from [] or ? etc ...

Thx for the suggestion. I'll take a look at sema.Type.

[turbolent]

The same can be done for sema.Type, e.g. there's sema.ConstantSizedType, sema.DictionaryType, etc.

[kozlovb]

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 resulting redundantType
has several fields like for example redundantType.ExpectedType of sema.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 relevant sema.Types ?

Also, I tried to search for TypeAnnotation in elaboration.go. With the idea that may be ast.FieldDeclaration.TypeAnnotation can be converted to sema.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.

[kozlovb]

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.

[turbolent]

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).

[SupunS]

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:

import ImprtedContract from 0x01

pub contract MyContract {

    // Assume `ImprtedContract.ContractData` has a public capability field.
    // It won't be able to report an error here only by looking at the syntax-tree
    pub var contractData: ImprtedContract.ContractData

[turbolent]

Why analyze only structures, and not e.g. resources or contracts?

[kozlovb]

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 -

pub contract MyContract {
   pub resource Counter {
     priv var count: Int

     init(count: Int) {
       self.count = count
     }
   }

   pub struct ContractData {
	   pub var owner: Capability
       init(cap: Capability) {
           self.owner = cap
       }
   }

	pub struct ContractDataIgnored {
	   pub var owner: Capability
       init(cap: Capability) {
           self.owner = cap
       }
   }

   pub var contractData: ContractData

   init(){
       self.contractData = ContractData(cap:
           self.account.getCapability<&Counter>(/public/counter)
       )
   }
}

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.

[turbolent]

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.

[kozlovb]

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 -

pub resource MyResource {
    pub let my_capability : Capability?
    init() {
        self.my_capability = nil
    }
}

pub contract MyContract {

priv var myResource: @MyResource?
    init() {
        self.myResource <- nil
    }
}`

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

[kozlovb]

@turbolent thx for the review. I'll take a more in depth look on Friday. Otherwise, I had 2 questions and for everything else I'll just apply your suggestions.

[kozlovb]

@turbolent I think I addressed all suggestions. Please take another look.

[SupunS]

I think the inner types of the restricted type would also need to be visited recursively.
e.g:

pub contract MyContract {
	
    pub var contractData: ContractData{SomeInterface}  //  <-- A restricted type, whose inner type has a public capability field.

    pub struct ContractData: SomeInterface {
        pub var owner: Capability

        init(cap: Capability) {
            self.owner = cap
        }
    }

    pub struct interface SomeInterface { 
        pub var owner: Capability
    }

}

[SupunS]

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?

[kozlovb]

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.

[SupunS]

Or maybe instead of doing it in two iterations, perform this check in a single iteration.
i.e:

• Visit each public field of the contract
• Check if the type of the field has public capability fields:
  • For that, visit the type then and there, recursively.
  • 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)
• It might be easier to use the sema-type instead.

I haven't actually tried this at the code level, but just an idea that might be easier to implement.

[kozlovb]

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 -

`
pub contract MyContract {
    pub resource Counter {
        priv var count: Int
 			
       init(count: Int) {
            self.count = count
 	}
     }
 		
    pub struct ContractData {
        pub var owner: Capability
            init(cap: Capability) {
                 self.owner = cap
            }
     }
 		
    priv var contractData: ContractData
 		
    init(){
        self.contractData = ContractData(cap:
            self.account.getCapability<&Counter>(/public/counter)
         )
    }
}
`

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.

[SupunS]

may be there ll be difficulties in case of name collisions

In the algorithm I suggested, there will be no name collisions since field names are not looked up. Fields are visited in-place.

it was reporting a false warning before I introduced 2 paths. I just couldn't come up with a solution that uses one path.

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.

[SupunS]

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.

[kozlovb]

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.

[kozlovb]

"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.

[SupunS]

pass.Program already has the ast nodes as well as type info to look up. So when you know a type (say, ContractData), it can be retrieved from the pass.Program. Here you can either retrieve the ast-type-definition (from pass.Program.Program) or the sema-type (from pass.Program.Elaboration)

[kozlovb]

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 run

func 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.

[kozlovb]

Hi @SupunS , I worked today on a solution with one pass. Indeed, I could use pass.Program.Program.CompositeDeclarations() to get declarations, may be later I could use sema as well.

However, I dont understand how to ignore definitions while performing
func (in *Inspector) Preorder(types []Element, f func(Element))

What did you have in mind when you commented -

"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."

So, how to avoid to visit field declaration inside of type/struct definitions ?
Shell I try to use something else than Preorder ?