Skip to content

Constrain preimage request lengths below state-key discriminators - #542

Open
philoniare wants to merge 3 commits into
gavofyork:mainfrom
philoniare:blob-length-collision
Open

Constrain preimage request lengths below state-key discriminators#542
philoniare wants to merge 3 commits into
gavofyork:mainfrom
philoniare:blob-length-collision

Conversation

@philoniare

Copy link
Copy Markdown
Contributor

Summary

Request lengths l ∈ 𝔹_L admit the values 2³²−1 and 2³²−2, which are the
state-key discriminators for the storage and preimages rows. A request
⟨h, 2³²−1⟩ therefore produces a C-input byte-identical to storage key
k = h, giving T(σ) two values for one key — ill-typed state with no
deterministic resolution, reachable via a single solicit. We reproduced the
collision with our client implementation, which accepts the sentinel lengths
and derives the identical trie key.

Changes

  • accounts.tex: restrict the request length domain from 𝔹_L to ℕ_{2³²−2}.
  • pvm_invocations.tex: enforce the bound in solicit, forget, query,
    provide (HUH guard tightened) and new (HUH case added; code-hash
    guard tightened, since the created account solicits its own code as ⟨c, l⟩).

Behavioral note

new previously panicked for l ≥ 2³²; it now returns HUH, matching the
other preimage host calls.

@zdave-parity

Copy link
Copy Markdown
Collaborator

We should name the set of allowed preimage lengths rather than repeating N_{2^32 - 2} everywhere. \bloblength is now only used in one place AFAICT so probably is unnecessary, maybe rename it to \preimagelen and use that?

@philoniare

Copy link
Copy Markdown
Contributor Author

@zdave-parity Done — added \preimagelen for the set. I kept \bloblength rather than renaming it, since it's still the type of the bundle-length field in the availability spec (reporting_assurance.tex), which isn't subject to the state-key discriminator bound. So \preimagelen is defined as a subset of it, next to the service account definition in accounts.tex.

@zdave-parity zdave-parity left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just a few minor nits.

Comment thread text/pvm_invocations.tex Outdated
\using \sq{o, l, \sa¬minaccgas, \sa¬minmemogas, \sa¬gratis, \local¬desiredid} &= \registers\subrange{7}{6} \\
\using \sa¬codehash &= \begin{cases}
\memory\subrange{o}{32} &\when \Nrange{o}{32} \subseteq \readable{\memory} \wedge l \in \Nbits{32} \\
\memory\subrange{o}{32} &\when \Nrange{o}{32} \subseteq \readable{\memory} \wedge l \in \preimagelen \\

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the l check here can just be dropped as it is now covered below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped l

Comment thread text/definitions.tex Outdated
\item[$\balance$] The set of balance values. Equivalent to $\Nbits{64}$. See equation \ref{eq:balance}.
\item[$\gas$] The set of unsigned gas values. Equivalent to $\Nbits{64}$. See equation \ref{eq:gasregentry}.
\item[$\bloblength$] The set of blob length values. Equivalent to $\Nbits{32}$. See section \ref{sec:numbers}.
\item[$\preimagelen$] The set of lengths under which a preimage may be solicited. A subset of $\bloblength$, equivalent to $\Nmax{2^{32} - 2}$. See equation \ref{eq:serviceaccounts}.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each line in a \begin{align} gets a separate equation number, so the ref here is not quite correct; we should add a separate label for the \preimagelen equation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment thread text/accounts.tex Outdated
As we already noted, a service in \Jam is somewhat analogous to a smart contract in Ethereum in that it includes amongst other items, a code component, a storage component and a balance. Unlike Ethereum, the code is split over two isolated entry-points each with their own environmental conditions; one, \emph{Refinement}, is essentially stateless and happens in-core, and the other, \emph{Accumulation}, which is stateful and happens on-chain. It is the latter which we will concern ourselves with now.

Service accounts are held in state under $\accounts$, a partial mapping from a service identifier $\serviceid$ into a tuple of named elements which specify the attributes of the service relevant to the \Jam protocol. Formally:
Service accounts are held in state under $\accounts$, a partial mapping from a service identifier $\serviceid$ into a tuple of named elements which specify the attributes of the service relevant to the \Jam protocol. We further name $\preimagelen$, the set of lengths under which a preimage may be solicited; this is the subset of $\bloblength$ omitting its two greatest values, since these are reserved as the discriminators of storage and preimage items respectively in the state-key derivation of section \ref{sec:statemerklization}. Formally:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the \preimagelen description and definition further down, beside or after \serviceaccount? They seem out of place here. You could also reference the T equation directly rather than just referencing the section?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved below \serviceaccount and included the reference to the equation

@philoniare

Copy link
Copy Markdown
Contributor Author

Thanks, just a few minor nits.

Thanks for the review, addressed all of the points

@zdave-parity zdave-parity left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants