fixup! feat: strict validation on block load

hamt.go

@@ -23,7 +23,7 @@ const defaultBitWidth = 8
 
 // ErrNotFound is returned when a Find operation fails to locate the specified
 // key in the HAMT
-var ErrNotFound = fmt.Errorf("not found")
+var ErrNotFound = fmt.Errorf("Not found")
 
 // ErrMaxDepth is returned when the HAMT spans further than the hash function
 // is capable of representing. This can occur when sufficient hash collisions
@@ -32,7 +32,7 @@ var ErrNotFound = fmt.Errorf("not found")
 // on extremely large (likely impractical) HAMTs that are unable to be
 // represented with the hash function used. Hash functions with larger byte
 // output increase the maximum theoretical depth of a HAMT.
-var ErrMaxDepth = fmt.Errorf("attempted to traverse hamt beyond max depth")
+var ErrMaxDepth = fmt.Errorf("Attempted to traverse HAMT beyond max-depth")
 
 // ErrMalformedHamt is returned whenever a block intended as a HAMT node does
 // not conform to the expected form that a block may take. This can occur
@@ -308,15 +308,10 @@ func (p *Pointer) loadChild(ctx context.Context, ns cbor.IpldStore, bitWidth int
 		return p.cache, nil
 	}
 
-	out, err := LoadNode(ctx, ns, p.Link)
+	out, err := loadNode(ctx, ns, p.Link, false, UseTreeBitWidth(bitWidth), UseHashFunction(hash))
 	if err != nil {
 		return nil, err
 	}
-	// these don't get set in LoadNode because we don't have the Options
-	// at this point but what is inherited from the parents may have varied
-	// from the defaults
-	out.bitWidth = bitWidth
-	out.hash = hash
 
 	p.cache = out
 	return out, nil
@@ -334,20 +329,12 @@ func (p *Pointer) loadChild(ctx context.Context, ns cbor.IpldStore, bitWidth int
 // of this library to remain the defaults long-term and have strategies in
 // place to manage variations.
 func LoadNode(ctx context.Context, cs cbor.IpldStore, c cid.Cid, options ...Option) (*Node, error) {
-	// TODO(rvagg): loaded nodes must be validated to make sure we have only
-	// the correct form of Nodes to avoid attacks from alternative implementations
-	// that feed us poorly formed data. Check that:
-	// 1. Pointers contains the correct number of elements defined by Bitfield
-	// 2. Pointers contain *only* a link or a bucket (this may already be done in
-	// the CBOR unmarshal but might be worth doing here so the check is all in
-	// one place)
-	// 3. Pointers with links have are DAG-CBOR multicodec
-	// 4. KV buckets contain strictly between 1 and bucketSize elements
-	// 5. KV buckets are ordered by key (bytewise comparison)
-	// 6. keys and values are valid (what are the rules? len(key)>0? can val be
-	// nul? etc.)
-	// 7. .. potentially we could validate the position of elements if we propagate
-	// the depth of this node so we know which bits to chomp off the hash digest.
+	return loadNode(ctx, cs, c, true, options...)
+}
+
+// internal version of loadNode that is aware of whether this is a root node or
+// not for the purpose of additional validation on non-root nodes.
+func loadNode(ctx context.Context, cs cbor.IpldStore, c cid.Cid, isRoot bool, options ...Option) (*Node, error) {
 	var out Node
 	if err := cs.Get(ctx, c, &out); err != nil {
 		return nil, err
@@ -368,12 +355,42 @@ func LoadNode(ctx context.Context, cs cbor.IpldStore, c cid.Cid, options ...Opti
 		return nil, ErrMalformedHamt
 	}
 
+	// the bifield is lying or the elements array is
+	if out.bitsSetCount() != len(out.Pointers) {
+		return nil, ErrMalformedHamt
+	}
+
 	for _, ch := range out.Pointers {
 		isLink := ch.isShard()
 		isBucket := ch.KVs != nil
 		if !((isLink && !isBucket) || (!isLink && isBucket)) {
 			return nil, ErrMalformedHamt
 		}
+		if isLink && ch.Link.Type() != 0x71 { // not dag-cbor
+			return nil, ErrMalformedHamt
+		}
+		if isBucket {
+			if len(ch.KVs) == 0 || len(ch.KVs) > bucketSize {
+				return nil, ErrMalformedHamt
+			}
+			for i := 1; i < len(ch.KVs); i++ {
+				if bytes.Compare(ch.KVs[i-1].Key, ch.KVs[i].Key) >= 0 {
+					return nil, ErrMalformedHamt
+				}
+			}
+		}
+	}
+
+	if !isRoot {
+		// the only valid empty node is a root node
+		if len(out.Pointers) == 0 {
+			return nil, ErrMalformedHamt
+		}
+		// a non-root node that contains <=bucketSize direct elements should not
+		// exist under compaction rules
+		if out.directChildCount() == 0 && out.directKVCount() <= bucketSize {
+			return nil, ErrMalformedHamt
+		}
 	}
 
 	return &out, nil
@@ -509,12 +526,7 @@ func (n *Node) cleanChild(chnd *Node, cindex byte) error {
 		return nil
 	}
 
-	l := len(chnd.Pointers)
-	if l == 0 {
-		return ErrMalformedHamt
-	}
-
-	if l == 1 {
+	if len(chnd.Pointers) == 1 {
 		// The case where the child node has a single bucket, which we know can
 		// only contain `bucketSize` elements (maximum), so we need to pull that
 		// bucket up into this node.