Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

fix: bound delegation depth in verify_response #241

Closed
wants to merge 1 commit into from

Conversation

mraszyk
Copy link
Contributor

@mraszyk mraszyk commented Oct 18, 2023

The current definition of verify_response for replica-signed queries allows for nested delegations in which case the canister ranges fetched from the delegation might not be signed by the root subnet. This PR fixes this security vulnerability by limiting the delegation depth to one.

There's a separate PR bounding the delegation depth to one globally. This PR is void if the delegation depth is bounded globally.

@mraszyk mraszyk requested a review from a team as a code owner October 18, 2023 16:26
@oggy-dfin
Copy link
Member

For the read_state request, in the informal part we already have this condition:

certificate (blob): A certificate (see Certification).

If this certificate includes subnet delegations (possibly nested), then the effective_canister_id must be included in each delegation's canister id range (see Delegation).

Since the formal part states that the certificate is obtained in a separate read_state request, I'd assume that the condition is basically checked there?

@mraszyk

This comment was marked as off-topic.

@mraszyk mraszyk marked this pull request as draft October 21, 2023 12:07
@mraszyk
Copy link
Contributor Author

mraszyk commented Nov 24, 2023

For the read_state request, in the informal part we already have this condition:

certificate (blob): A certificate (see Certification).
If this certificate includes subnet delegations (possibly nested), then the effective_canister_id must be included in each delegation's canister id range (see Delegation).

Since the formal part states that the certificate is obtained in a separate read_state request, I'd assume that the condition is basically checked there?

We need to obtain the subnet_id for subsequent certificate lookup so we need this code explicitly in verify_response. However, I agree that security-wise the read_state verification checks that the certificate is authorized for the given effective_canister_id and thus we don't need to bound the delegation chain to be of length one here.

@mraszyk mraszyk closed this Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants