Merge with audit-remediations

docs/mapstore.md

@@ -1,12 +1,12 @@
-# MapStore
-
-<!-- toc -->
+# MapStore <!-- omit in toc -->
 
+- [Introduction](#introduction)
 - [Implementations](#implementations)
-  * [SimpleMap](#simplemap)
-  * [BadgerV4](#badgerv4)
+  - [SimpleMap](#simplemap)
+  - [BadgerV4](#badgerv4)
+- [Note On External Writability](#note-on-external-writability)
 
-<!-- tocstop -->
+## Introduction
 
 The `MapStore` is a simple interface used by the SM(S)T to store, delete and
 retrieve key-value pairs. It is intentionally simple and minimalistic so as to
@@ -31,11 +31,15 @@ See [simplemap.go](../kvstore/simplemap/simplemap.go) for more details.
 
 ### BadgerV4
 
-This library provides a wrapper around [dgraph-io/badger][badgerv4] to adhere
-to the `MapStore` interface. See the [full documentation](./badger-store.md)
-for additional functionality and implementation details.
+This library provides a wrapper around [dgraph-io/badger][https://github.com/dgraph-io/badger] to adhere to
+the `MapStore` interface. See the [full documentation](./badger-store.md) for
+additional functionality and implementation details.
+
+See: [badger](../kvstore/badger/) for more details on the implementation of this
+submodule.
 
-See: [badger](../kvstore/badger/) for more details on the implementation of
-this submodule.
+## Note On External Writability
 
-[badgerv4]: https://github.com/dgraph-io/badger
+Any key-value store used by the tries should **not** be able to be externally
+writeable in production. This opens the possibility to attacks where the writer
+can modify the trie database and prove values that were not inserted.

docs/smt.md

@@ -1,23 +1,12 @@
-# smt
-
-<!-- toc -->
-
-- [smt](#smt)
-  - [Overview](#overview)
-  - [Implementation](#implementation)
-    - [Leaf Nodes](#leaf-nodes)
-    - [Inner Nodes](#inner-nodes)
-    - [Extension Nodes](#extension-nodes)
-    - [Lazy Nodes](#lazy-nodes)
-    - [Lazy Loading](#lazy-loading)
-    - [Visualisations](#visualisations)
-      - [General Trie Structure](#general-trie-structure)
-      - [Lazy Nodes](#lazy-nodes-1)
+# smt <!-- omit in toc -->
+
+- [nodes concatenated hashes.](#nodes-concatenated-hashes) - [Extension Nodes](#extension-nodes) - [Lazy Nodes](#lazy-nodes) - [Lazy Loading](#lazy-loading) - [Visualisations](#visualisations) - [General Trie Structure](#general-trie-structure) - [Lazy Nodes](#lazy-nodes-1)
   - [Paths](#paths)
     - [Visualisation](#visualisation)
   - [Values](#values)
     - [Nil values](#nil-values)
   - [Hashers \& Digests](#hashers--digests)
+    - [Hash Function Recommendations](#hash-function-recommendations)
   - [Roots](#roots)
   - [Proofs](#proofs)
     - [Verification](#verification)
@@ -32,8 +21,6 @@
     - [Data Loss](#data-loss)
   - [Sparse Merkle Sum Trie](#sparse-merkle-sum-trie)
 
-<!-- tocstop -->
-
 ## Overview
 
 Sparse Merkle Tries (SMTs) are efficient and secure data structures for storing
@@ -77,10 +64,9 @@ constructor.
 
 ### Inner Nodes
 
-Inner nodes represent a branch in the trie with two **non-nil** child nodes.
-
-The inner node has an internal `digest` which represents the hash of the child
-nodes concatenated hashes.
+Inner nodes represent a branch in the trie with two **non-nil** child nodes. The
+inner node has an internal `digest` which represents the hash of the child nodes
+concatenated hashes.
 
 - _Prefix_: `[]byte{1}`
 - _Digest_: `hash([]byte{1} + leftChild.digest + rightChild.digest)`
@@ -319,8 +305,8 @@ described in the [implementation](#implementation) section.
 The following diagram represents the creation of a leaf node in an abstracted
 and simplified manner.
 
-_Note: This diagram is not entirely accurate regarding the process of creating
-a leaf node, but is a good representation of the process._
+_Note: This diagram is not entirely accurate regarding the process of creating a
+leaf node, but is a good representation of the process._
 
 ```mermaid
 graph TD
@@ -355,20 +341,34 @@ graph TD
 	VH --ValueHash-->L
 ```
 
+### Hash Function Recommendations
+
+Although any hash function that satisfies the `hash.Hash` interface can be used
+to construct the trie, it is **strongly recommended** to use a hashing function
+that provides the following properties:
+
+- **Collision resistance**: The hash function must be collision resistant. This
+  is needed in order for the inputs of the SMT to be unique.
+- **Preimage resistance**: The hash function must be preimage resistant. This
+  is needed to protect against the Merkle tree construction attacks where
+  the attacker can modify unknown data.
+- **Efficiency**: The hash function must be efficient, as it is used to compute
+  the hash of many nodes in the trie.
+
 ## Roots
 
 The root of the tree is a slice of bytes. `MerkleRoot` is an alias for `[]byte`.
-This design enables easily passing around the data (e.g. on-chain)
-while maintaining primitive usage in different use cases (e.g. proofs).
+This design enables easily passing around the data (e.g. on-chain) while
+maintaining primitive usage in different use cases (e.g. proofs).
 
 `MerkleRoot` provides helpers, such as retrieving the `Sum() uint64` to
-interface with data it captures. However, for the SMT it **always** panics,
-as there is no sum.
+interface with data it captures. However, for the SMT it **always** panics, as
+there is no sum.
 
 ## Proofs
 
-The `SparseMerkleProof` type contains the information required for inclusion
-and exclusion proofs, depending on the key provided to the trie method
+The `SparseMerkleProof` type contains the information required for inclusion and
+exclusion proofs, depending on the key provided to the trie method
 `Prove(key []byte)` either an inclusion or exclusion proof will be generated.
 
 _NOTE: The inclusion and exclusion proof are the same type, just constructed
@@ -409,29 +409,29 @@ using the `VerifyClosestProof` function which requires the proof and root hash
 of the trie.
 
 Since the `ClosestProof` method takes a hash as input, it is possible to place a
-leaf in the trie according to the hash's path, if it is known. Depending on
-the use case of this function this may expose a vulnerability. **It is not
-intendend to be used as a general purpose proof mechanism**, but instead as a
-**Commit and Reveal** mechanism, as detailed below.
+leaf in the trie according to the hash's path, if it is known. Depending on the
+use case of this function this may expose a vulnerability. **It is not intendend
+to be used as a general purpose proof mechanism**, but instead as a **Commit and
+Reveal** mechanism, as detailed below.
 
 #### Closest Proof Use Cases
 
 The `CloestProof` function is intended for use as a `commit & reveal` mechanism.
 Where there are two actors involved, the **prover** and **verifier**.
 
-_NOTE: Throughout this document, `commitment` of the the trie's root hash is also
-referred to as closing the trie, such that no more updates are made to it once
-committed._
+_NOTE: Throughout this document, `commitment` of the the trie's root hash is
+also referred to as closing the trie, such that no more updates are made to it
+once committed._
 
 Consider the following attack vector (**without** a commit prior to a reveal)
 into consideration:
 
 1. The **verifier** picks the hash (i.e. a single branch) they intend to check
-1. The **prover** inserts a leaf (i.e. a value) whose key (determined via the
+2. The **prover** inserts a leaf (i.e. a value) whose key (determined via the
    hasher) has a longer common prefix than any other leaf in the trie.
-1. Due to the deterministic nature of the `ClosestProof`, method this leaf will
+3. Due to the deterministic nature of the `ClosestProof`, method this leaf will
    **always** be returned given the identified hash.
-1. The **verifier** then verifies the revealed `ClosestProof`, which returns a
+4. The **verifier** then verifies the revealed `ClosestProof`, which returns a
    branch the **prover** inserted after knowing which leaf was going to be
    checked.
 
@@ -440,16 +440,16 @@ Consider the following normal flow (**with** a commit prior to reveal) as
 1. The **prover** commits to the state of their trie by publishes their root
    hash, thereby _closing_ their trie and not being able to make further
    changes.
-1. The **verifier** selects a hash to be used in the `commit & reveal` process
+2. The **verifier** selects a hash to be used in the `commit & reveal` process
    that the **prover** must provide a closest proof for.
-1. The **prover** utilises this hash and computes the `ClosestProof` on their
+3. The **prover** utilises this hash and computes the `ClosestProof` on their
    _closed_ trie, producing a `ClosestProof`, thus revealing a deterministic,
    pseudo-random leaf that existed in the tree prior to commitment, yet
-1. The **verifier** verifies the proof, in turn, verifying the commitment
-   made by the **prover** to the state of the trie in the first step.
-1. The **prover** had no opportunity to insert a new leaf into the trie
-   after learning which hash the **verifier** was going to require a
-   `ClosestProof` for.
+4. The **verifier** verifies the proof, in turn, verifying the commitment made
+   by the **prover** to the state of the trie in the first step.
+5. The **prover** had no opportunity to insert a new leaf into the trie after
+   learning which hash the **verifier** was going to require a `ClosestProof`
+   for.
 
 ### Compression
 
@@ -509,7 +509,8 @@ database. It's interface exposes numerous extra methods not used by the trie,
 However it can still be used as a node-store with both in-memory and persistent
 options.
 
-See [badger-store.md](./badger-store.md.md) for the details of the implementation.
+See [badger-store.md](./badger-store.md.md) for the details of the
+implementation.
 
 ### Data Loss
 

errors.go

@@ -9,4 +9,7 @@ var (
 	ErrBadProof = errors.New("bad proof")
 	// ErrKeyNotFound is returned when a key is not found in the tree.
 	ErrKeyNotFound = errors.New("key not found")
+	// ErrInvalidClosestPath is returned when the path used in the ClosestProof
+	// method does not match the size of the trie's PathHasher
+	ErrInvalidClosestPath = errors.New("invalid path does not match path hasher size")
 )

hasher.go

@@ -27,6 +27,8 @@ type PathHasher interface {
 type ValueHasher interface {
 	// HashValue hashes value data to produce the digest stored in leaf node.
 	HashValue([]byte) []byte
+	// ValueHashSize returns the length (in bytes) of digests produced by this hasher.
+	ValueHashSize() int
 }
 
 // trieHasher is a common hasher for all trie hashers (paths & values).
@@ -56,8 +58,8 @@ func NewTrieHasher(hasher hash.Hash) *trieHasher {
 	return &th
 }
 
-func NewNilPathHasher(hasher hash.Hash) PathHasher {
-	return &nilPathHasher{hashSize: hasher.Size()}
+func NewNilPathHasher(hasherSize int) PathHasher {
+	return &nilPathHasher{hashSize: hasherSize}
 }
 
 // Path returns the digest of a key produced by the path hasher
@@ -76,6 +78,14 @@ func (vh *valueHasher) HashValue(data []byte) []byte {
 	return vh.digestData(data)
 }
 
+// ValueHashSize returns the length (in bytes) of digests produced by the value hasher
+func (vh *valueHasher) ValueHashSize() int {
+	if vh.hasher == nil {
+		return 0
+	}
+	return vh.hasher.Size()
+}
+
 // Path satisfies the PathHasher#Path interface
 func (n *nilPathHasher) Path(key []byte) []byte {
 	return key[:n.hashSize]

options.go

@@ -27,7 +27,7 @@ func NoHasherSpec(hasher hash.Hash, sumTrie bool) *TrieSpec {
 	spec := newTrieSpec(hasher, sumTrie)
 
 	// Set a nil path hasher
-	opt := WithPathHasher(NewNilPathHasher(hasher))
+	opt := WithPathHasher(NewNilPathHasher(hasher.Size()))
 	opt(&spec)
 
 	// Set a nil value hasher