Permissions system for comptime

[jfecher]

Problem

Related to #11625

One security issue for comptime code that would still be possible if #11625 is implemented would be:

1. A library helpful is released which encourages users to use its helpful attribute(s) to save on code
2. The library is later updated so that its previously helpful attribute(s) now maliciously edit functions

Happy Case

It is difficult to prevent this in general but one way which may make it more difficult is if users had to enable permissions for attributes they call. Attributes without the permission to, e.g. edit a function, would not be able to do so. This makes it more difficult for a attribute which only reads data to be updated to modify a function without the end-user knowing.

The first hurdle to this approach is the question of how granular these permissions should be. Most user-written attributes edit the item they're placed on to some degree so a simple read_only vs allow_mutation distinction may not be effective in practice.

One option would be to allow each comptime API builtin to be called individually. This would require users explicitly opt-into allow(set_body, set_parameters), etc. with possible aliases like allow(everything) for convenience but trading off on precision and thus security. Following is a list of each comptime builtin (at the time of writing) which may mutate a source code item:

            "function_def_add_attribute"
            "function_def_set_body"
            "function_def_set_parameters"
            "function_def_set_return_type"
            "function_def_set_return_public"
            "function_def_set_return_data"
            "function_def_set_unconstrained"
            "module_add_item"
            "type_def_add_attribute"
            "type_def_add_generic"
            "type_def_set_fields"

The list of functions which may read from an item is much larger. It isn't clear if there's any security concerns from only reading source code without being able to modify it or have access to IO. Following is an abbreviated list of methods which read from an item but do not modify it:

Read-only comptime builtin methods

            "function_def_as_typed_expr"
            "function_def_body"
            "function_def_eq"
            "function_def_has_named_attribute"
            "function_def_hash"
            "function_def_is_unconstrained"
            "function_def_module"
            "function_def_name"
            "function_def_parameters"
            "function_def_return_type"
            "function_def_visibility"
            "module_child_modules"
            "module_eq"
            "module_functions"
            "module_has_named_attribute"
            "module_hash"
            "module_is_contract"
            "module_name"
            "module_parent"
            "module_structs"
            "quoted_as_module"
            "quoted_as_trait_constraint"
            "quoted_as_type"
            "quoted_hash"
            "quoted_tokens"
            "trait_constraint_eq"
            "trait_constraint_hash"
            "trait_def_eq"
            "trait_def_hash"
            "trait_impl_methods"
            "trait_impl_trait_generic_args"
            "type_def_eq"
            "type_def_fields"
            "type_def_fields_as_written"
            "type_def_generics"
            "type_def_has_named_attribute"
            "type_def_hash"
            "type_def_module"
            "type_def_name"
            "type_eq"
            "type_get_trait_impl"
            "type_hash"
            "type_implements"
            "type_of"
            "typed_expr_as_function_definition"
            "typed_expr_get_type"

One possibility with explicitly allowing each modification may look like:

// The aforementioned  "helpful" attribute which is later updated to a malicious one
use helpful::helpful_attr;

#[allow(function_def_set_parameters)]
#[helpful_attr]
fn foo() {}

Above, helpful_attr would be allowed to modify the parameters but we'd get an error if it were updated to mutate the function body as well.

---

From the list of mutating functions function_def_set_body seems the main target for potential insertion of malicious code. The other candidate would be module_add_item as well as simply returning a Quoted value from the attribute to insert new code into the module. This could be used to insert a malicious trait implementation which gets called automatically for example. Given how common these three operations are in attributes, it is unclear how much value adding a fine-grained permissions simple would add. Permissions may end up devolving into "this may mutate" versus "this may not mutate" anyway:

#mut[helpful_attr]  // This may mutate foo, the current module, or submodules
fn foo() {}

#[helpful_attr]  // This may not mutate anything
fn bar() {}

With how commonly mutation may be allowed, nargo expand may still be the simplest tool to check for malicious code. Opting into such changes as in #11625 would at least help mark them, although users would still be vulnerable to malicious library updates.

Workaround

None

Workaround Description

No response

Additional Context

No response

Project Impact

None

Blocker Context

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

[vezenovm]

With how commonly mutation may be allowed, nargo expand may still be the simplest tool to check for malicious code. Opting into such changes as in #11625 would at least help mark them, although users would still be vulnerable to malicious library updates.

If we look at the most used Noir macros library, aztec-nr, we find this to be the case. Aztec macros need full mutation capabilities. Thus permissions do not prevent an attacker as full power needs to be granted to the macros library. As you mentioned everything would probably degrade into allowing everything:

#[allow(everything)]
#[aztec]
pub contract MyContract { ... }

This situation of course extends to any other extensive macro libraries.

The strongest defense here seems to be snapshot testing against nargo expand diffs. Users would have to check diffs on dependency upgrades. Expansion diffs should be treated like differences in the public API of the library being used by the project. This seems to be not only the simplest way to check for malicious code, but the only way when a library needs full permissions. I don't like the idea of requiring all production projects which depend on macro code needing to snapshot diffs but perhaps we can provide out-of-the-box tooling to make it simpler.

Aside libraries requiring full permissions I do see some value in a permissions system though. Not every macro needs potentially full mutation (god) capabilities. For macros not needing full power, permissions would reduce the noise of snapshot testing and reduce the risk of accidental mutations as well. If were are thinking about permissions I do think we might as well go all the way as make a full system rather than doing #11625. But perhaps #11625 can be a good first step.

[jfecher]

If were are thinking about permissions I do think we might as well go all the way as make a full system rather than doing #11625. But perhaps #11625 can be a good first step

I think #11625 is orthogonal to the permissions system described here. #11625 deals with "who is allowed to change this code / where are changes allowed to come from?" while this issue deals with "what changes are allowed to be made?" This issue notably doesn't change the example in #11625 of a root module editing a child one. The attribute on the root module would just need to have a #[allow(...)] or similar but the child would be written with no indication it may be edited.

Going back to this issue, it is worth noting a common pattern seems to be using some attributes to record data first (pushing to a global collection) then using another later on to actually perform the mutations. This is the pattern the stdlib uses with #[derive_via(_)] (registers) and #[derive(...)] (inserts items), and is also the pattern commonly used by aztec-nr. It follows then that most attributes in aztec-nr wouldn't actually be marked as mutating even though they'd be collecting items to be externally mutated. I would think we'd the allow attribute would need to guard the FunctionDefinition given somehow so that it can't be mutated somewhere it wasn't allowed to be - otherwise users would be able to store it in a map somewhere then retrieve it later and mutate it anyway in another attribute that is allowed to mutate. #11625 would help prevent this by guarding against the location of the mutation but I mostly bring this up as a common practice we should think about while designing the system.

I'm still mostly in the opinion that #11625 is more useful than this issue since I think it is at least generally accepted that if you have an attribute directly on an item, that item may be modified arbitrarily.

[vezenovm]

"who is allowed to change this code / where are changes allowed to come from?" while this issue deals with "what changes are allowed to be made?" This issue notably doesn't change the example in #11625 of a root module editing a child one. The attribute on the root module would just need to have a #[allow(...)] or similar but the child would be written with no indication it may be edited.

Fair point, agreed they are orthogonal.

I do agree generally then that #11625 is more useful (and less intrusive of a language feature) as its focuses on the location of the mutation.