Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Malleability] Automatic malleability checker #7069

Open
wants to merge 15 commits into
base: feature/malleability
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions model/flow/epoch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,30 @@ import (
"github.com/onflow/flow-go/utils/unittest"
)

// TestMalleability performs sanity checks to ensure that epoch related entities are not malleable.
func TestMalleability(t *testing.T) {
t.Run("EpochSetup", func(t *testing.T) {
unittest.RequireEntityNonMalleable(t, unittest.EpochSetupFixture())
})
t.Run("EpochCommit-v1", func(t *testing.T) {
unittest.RequireEntityNonMalleable(t, unittest.EpochCommitFixture())
})

checker := unittest.NewMalleabilityChecker(unittest.WithCustomType(flow.DKGIndexMap{}, func() any {
return flow.DKGIndexMap{unittest.IdentifierFixture(): 0, unittest.IdentifierFixture(): 1}
}))
t.Run("EpochCommit-v2", func(t *testing.T) {
err := checker.Check(unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {
commit.DKGIndexMap = flow.DKGIndexMap{unittest.IdentifierFixture(): 0, unittest.IdentifierFixture(): 1}
}))
require.NoError(t, err)
})
t.Run("EpochRecover", func(t *testing.T) {
err := checker.Check(unittest.EpochRecoverFixture())
require.NoError(t, err)
})
}

func TestClusterQCVoteData_Equality(t *testing.T) {

pks := unittest.PublicKeysFixture(2, crypto.BLSBLS12381)
Expand Down
290 changes: 290 additions & 0 deletions utils/unittest/entity.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
package unittest

import (
"fmt"
"math/rand"
"reflect"
"testing"
"time"

"github.com/onflow/crypto"
"github.com/stretchr/testify/require"

"github.com/onflow/flow-go/model/flow"
)

// MockEntity implements a bare minimum entity for sake of test.
type MockEntity struct {
Identifier flow.Identifier
Nonce uint64
}

func (m MockEntity) ID() flow.Identifier {
return m.Identifier
}

func (m MockEntity) Checksum() flow.Identifier {
return m.Identifier
}

func EntityListFixture(n uint) []*MockEntity {
list := make([]*MockEntity, 0, n)

for i := uint(0); i < n; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for i := uint(0); i < n; i++ {
for range n {

list = append(list, &MockEntity{
Identifier: IdentifierFixture(),
})
Comment on lines +34 to +36
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
list = append(list, &MockEntity{
Identifier: IdentifierFixture(),
})
list = append(list, MockEntityFixture())

}

return list
}

func MockEntityFixture() *MockEntity {
return &MockEntity{Identifier: IdentifierFixture()}
}

func MockEntityListFixture(count int) []*MockEntity {
Copy link
Member

Choose a reason for hiding this comment

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

This seems identical to EntityListFixture - can we remove one?

entities := make([]*MockEntity, 0, count)
for i := 0; i < count; i++ {
entities = append(entities, MockEntityFixture())
}
return entities
}

// RequireEntityNonMalleable is a sanity check that the entity is not malleable with regards to the ID() function.
// Non-malleability in this sense means that it is computationally hard to build a different entity with the same ID. This implies that in a non-malleable entity, changing any field should change the ID, which is checked by this function. Note that this is sanity check of non-malleability and that passing this test does not guarantee non-malleability.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Non-malleability in this sense means that it is computationally hard to build a different entity with the same ID. This implies that in a non-malleable entity, changing any field should change the ID, which is checked by this function. Note that this is sanity check of non-malleability and that passing this test does not guarantee non-malleability.
// Non-malleability in this sense means that it is computationally hard to build a different entity with the same ID.
// This implies that in a non-malleable entity, changing any field should change the ID, which is checked by this function.
// Note that this is sanity check of non-malleability and that passing this test does not guarantee non-malleability.

Breaking up a super-long line

// Non-malleability is a required property for any entity that implements the [flow.IDEntity] interface. This is especially
// important for entities that contain signatures and are transmitted over the network.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is especially important for entities that contain signatures and are transmitted over the network

Why is it more important when the entity contain signatures more than in other cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was making emphasis on this since attacker can get hold of messages that are transmitted over the network so it's easier for him to exploit this bug comparing to having a malleable data structure that is being stored in memory or written to disk.

Copy link
Contributor

@tarakby tarakby Feb 20, 2025

Choose a reason for hiding this comment

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

ah yes you mean that we rely on ID() for integrity of the transmitted messages, and you're saying that every transmitted message is authenticated so it has a signature field.

it's easier for him to exploit this bug comparing to having a malleable data structure that is being stored in memory or written to disk.

Our threat model for nodes does not cover these "insider" cases anyway. We assume that all components of the nodes are trusted (i.e shielded from attacks) and only external components are not trusted. This is not always true in practice but that's the threat model our code base (incl cryptography) assumes, so we should only focus on the case of transmitted messages between nodes IMO.

// ID is used by the protocol to insure entity integrity when transmitted over the network. ID must therefore be a binding cryptographic commitment to an entity.
// This function consumes the entity and modifies its fields randomly to ensure that the ID changes after each modification.
// Generally speaking each type that implements [flow.IDEntity] method should be tested with this function.
// ATTENTION: We put only one requirement for data types, that is all fields have to be exported so we can modify them.
func RequireEntityNonMalleable(t *testing.T, entity flow.IDEntity, ops ...MalleabilityCheckerOpt) {
err := NewMalleabilityChecker(ops...).Check(entity)
require.NoError(t, err)
}

// MalleabilityChecker is a structure that holds additional information about the context of malleability check.
// It allows to customize the behavior of the check by providing custom types and their generators.
// All underlying checks are implemented as methods of this structure.
// This structure is used to check if the entity is malleable. Strictly speaking if a structure implements [flow.IDEntity] interface
// any change to the data structure has to change the ID of the entity as well.
// This structure performs a recursive check of all fields of the entity and ensures that changing any field will change the ID of the entity.
// The idea behind implementation is that user provides a structure which serves a layout(scheme) for the entity and
// the checker will generate random values for the fields that are not empty or nil. This way it supports structures where some fields
// are omitted in some scenarios but the structure has to be non-malleable.
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the reasoning behind this constraint. (As I understand the documentation, the checker will not modify fields which are set to zero/nil in the input.) However, it also seems like this constraint only applies to map/slice-typed fields.

A big part of the malleability work is to enforce that if a model field changes, the ID must also change. But this constraint introduces significant surface area for our tests to fail to check these cases, if the test accidentally passes in a model fixture with any of its fields nil (those fields won't be checked for malleability!).

It's very easy to accidentally not initialize a struct field in Go, generally speaking. I think there are quite a few instances where our fixtures do not initialize every field. I found a couple of examples:

Most of the existing usages of the Malleability checker are just passing in the fixture for the type being checked. So I'm worried that we can easily be creating tests that are not exhaustively checking malleability, because they do not modify certain fields.

Suggestions

  • Clarify in the documentation exactly what zero value inputs will result in the Checker not checking the field for malleability, if this behaviour is necessary
  • Change the default behaviour: if an entity is passed in with a field which we will not be able to check for any reason (currently: a nil map/slice) then throw an error.

// This checker heavily relies on generation of random values for the fields based on their type. All types are split into three categories:
// 1) structures (generateCustomFlowValue)
// 2) interfaces (generateInterfaceFlowValue)
// 3) primitives, slices, arrays, maps (generateRandomReflectValue)
// Checker knows how to deal with each of the categories and generate random values for them but not for all types.
// If the type is not recognized there are two ways:
// 1) User can provide a custom type generator for the type using WithCustomType option.
// 2) User can extend the checker with new type handling.
// It is recommended to use the second option if type is used in multiple places and general enough. For other cases
// it is better to use the first option. Mind that the first option overrides any type handling that is embedded in the checker.
// This is very useful for cases where the field is context-sensitive, and we cannot generate a completely random value.
type MalleabilityChecker struct {
*testing.T
customTypes map[reflect.Type]func() reflect.Value
}

// MalleabilityCheckerOpt is a functional option for the MalleabilityChecker which allows to modify behavior of the checker.
type MalleabilityCheckerOpt func(*MalleabilityChecker)

// WithCustomType allows to override the default behavior of the checker for the given type, meaning if a field of the given type is encountered
// it will use generator instead of generating a random value.
func WithCustomType[T any](tType any, generator func() T) MalleabilityCheckerOpt {
return func(t *MalleabilityChecker) {
t.customTypes[reflect.TypeOf(tType)] = func() reflect.Value {
return reflect.ValueOf(generator())
}
}
}

// NewMalleabilityChecker creates a new instance of the MalleabilityChecker with the given options.
func NewMalleabilityChecker(ops ...MalleabilityCheckerOpt) *MalleabilityChecker {
checker := &MalleabilityChecker{
customTypes: make(map[reflect.Type]func() reflect.Value),
}
for _, op := range ops {
op(checker)
}
return checker
}

// Check is a method that performs the malleability check on the entity.
// It returns an error if the entity is malleable, otherwise it returns nil.
func (t *MalleabilityChecker) Check(entity flow.IDEntity) error {
v := reflect.ValueOf(entity)
if v.IsValid() {
if v.Kind() == reflect.Ptr {
if v.IsNil() {
return fmt.Errorf("entity is nil, nothing to check")
}
v = v.Elem()
} else {
// If it is not a pointer type, we may not be able to set fields to test malleability, since the entity may not be addressable
return fmt.Errorf("entity is not a pointer type (try taking a reference to it), entity: %v %v", v.Kind(), v.Type())
}
} else {
return fmt.Errorf("tested entity is not valid")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I ran into an issue with reflection when trying to run the malleability check on Header: it failed because the ID() function on the struct had a value receiver instead of a pointer receiver, AND I was passing in the struct directly instead of a pointer/reference to it.
The error message I received was confusing and unintuitive (that one of the structure's fields was unaddressable, when in fact the issue was that the struct as a whole was unaddressable). Therefore I think we should at least improve the error message for this situation, if not fix it entirely.

I think there are three potential ways to address this:

  1. No change here, but enforce that all implementations of ID() must take a pointer receiver (How? Is this otherwise desirable?)
  2. Require that a pointer type be passed here, and add an error message if the entity passed in was not a reference. Tests would need to ensure they are passing a reference to the underlying struct to avoid failing this requirement. For example, with this change the current TestRequireEntityNonMalleable/type_alias would fail, and would need to replace IntList{1, 2, 3} with &IntList{1, 2, 3} to pass.
Suggested change
}
} else {
// If it is not a pointer type, we may not be able to set fields to test malleability, since the entity may not be addressable
require.Failf(t, "entity is not a pointer type (try taking a reference to it)", "entity: %v %v", v.Kind(), v.Type())
}
  1. Automatically create an addressable copy of any non-addressable values: this is a fully general solution that should guarantee that all implementations of IDEntity can be checked.
Suggested change
}
} else {
// Create a copy to guarantee value is settable
tmp := reflect.New(v.Type())
tmp.Elem().Set(v)
entity = tmp.Interface().(flow.IDEntity)
v = tmp.Elem()
}

(adapted from stackoverflow)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting this. I am leaning towards your suggestion 2), I am a bit afraid of adding more shenanigans with reflection as it's already complicated so I would require that we pass a pointer since we most of the time operate on pointers either way.

I have also found another problem that the checker fails on processing time.Time because it tries to recursively check it but it has private fields so I have updated the logic to first try generate a field, event if it's a struct and only if we have failed then we apply a field-by-field recursive check. 1534144 (#7069)

return t.isEntityMalleable(v, entity.ID)
}

// isEntityMalleable is a helper function to recursively check fields of the entity.
// Every time we change a field we check if ID of the entity has changed. If the ID has not changed then entity is malleable.
// This function returns error if the entity is malleable, otherwise it returns nil.
func (t *MalleabilityChecker) isEntityMalleable(v reflect.Value, idFunc func() flow.Identifier) error {
if v.Kind() == reflect.Ptr && v.IsNil() {
return nil // there is nothing to check for this field if it's nil
}

tType := v.Type()
// if we have a custom type function we should use it to generate a random value for the field.
customTypeGenerator, hasCustomTypeOverride := t.customTypes[tType]
if hasCustomTypeOverride {
origID := idFunc()
v.Set(customTypeGenerator())
newID := idFunc()
if origID != newID {
return nil
}
return fmt.Errorf("ID did not change after changing field %s", tType.String())
}

if v.Kind() == reflect.Struct {
// in case we are dealing with struct we have two options:
// 1) if it's a known type where we know how to generate a random value we generate it and replace the whole field with it
// 2) if we don't anticipate the type we check if the field is malleable by checking all fields of the struct recursively
if generatedValue := reflect.ValueOf(generateCustomFlowValue(v)); generatedValue.IsValid() {
origID := idFunc()
v.Set(generatedValue)
newID := idFunc()
if origID != newID {
return nil
}
return fmt.Errorf("ID did not change after changing field %s", tType.String())
} else {
for i := 0; i < v.NumField(); i++ {
field := v.Field(i)
if !field.CanSet() {
return fmt.Errorf("field %s is not settable", tType.Field(i).Name)
}
if err := t.isEntityMalleable(field, idFunc); err != nil {
return fmt.Errorf("field %s is malleable", tType.Field(i).Name)
}
}
return nil
}
} else {
// when dealing with non-composite type we can generate random values for it and check if ID has changed.
origID := idFunc()
expectChange, err := generateRandomReflectValue(v)
if err != nil {
return fmt.Errorf("failed to generate random value for field %s: %w", tType.String(), err)
}
newID := idFunc()
if !expectChange || origID != newID {
return nil
}
return fmt.Errorf("ID did not change after changing field %s", tType.String())
}
}

// generateRandomReflectValue uses reflection to switch on the field type and generate a random value for it.
// Returned values indicate if the field was generated and if there was an error during the generation.
// No errors are expected during normal operations.
func generateRandomReflectValue(field reflect.Value) (generated bool, err error) {
generated = true
switch field.Kind() {
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
field.SetUint(^field.Uint())
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
field.SetInt(^field.Int())
case reflect.String:
field.SetString(fmt.Sprintf("random_%d", rand.Intn(100000)))
case reflect.Float64, reflect.Float32:
field.SetFloat(field.Float() + rand.Float64()*10)
case reflect.Bool:
field.SetBool(!field.Bool())
case reflect.Slice:
if field.Len() > 0 {
index := rand.Intn(field.Len())
generated, err = generateRandomReflectValue(field.Index(index))
} else {
generated = false // empty slice is ignored
}
case reflect.Array:
index := rand.Intn(field.Len())
generated, err = generateRandomReflectValue(field.Index(index))
case reflect.Map:
if mapKeys := field.MapKeys(); len(mapKeys) > 0 {
for _, key := range mapKeys {
oldVal := field.MapIndex(key)
newVal := reflect.New(oldVal.Type()).Elem()
generated, err = generateRandomReflectValue(newVal)
field.SetMapIndex(key, newVal)
break
}
} else {
generated = false // empty map is ignored
}
case reflect.Ptr:
if field.IsNil() {
return false, fmt.Errorf("cannot generate random value for nil pointer")
}
generated, err = generateRandomReflectValue(field.Elem()) // modify underlying value
case reflect.Struct:
generatedValue := reflect.ValueOf(generateCustomFlowValue(field))
if !generatedValue.IsValid() {
return false, fmt.Errorf("cannot generate random value for struct: %s", field.Type().String())
}
field.Set(generatedValue)
case reflect.Interface:
generatedValue := reflect.ValueOf(generateInterfaceFlowValue(field)) // it's always a pointer
if !generatedValue.IsValid() {
return false, fmt.Errorf("cannot generate random value for interface: %s", field.Type().String())
}
field.Set(generatedValue)
default:
return false, fmt.Errorf("cannot generate random value, unsupported type: %s", field.Kind().String())
}
return
}

// generateCustomFlowValue generates a random value for the field of the struct that is not a primitive type.
// This can be extended for types that are broadly used in the code base.
func generateCustomFlowValue(field reflect.Value) any {
switch field.Type() {
case reflect.TypeOf(flow.Identity{}):
return *IdentityFixture()
case reflect.TypeOf(flow.IdentitySkeleton{}):
return IdentityFixture().IdentitySkeleton
case reflect.TypeOf(flow.ClusterQCVoteData{}):
return flow.ClusterQCVoteDataFromQC(QuorumCertificateWithSignerIDsFixture())
case reflect.TypeOf(flow.QuorumCertificate{}):
return *QuorumCertificateFixture()
case reflect.TypeOf(flow.TimeoutCertificate{}):
return flow.TimeoutCertificate{
View: rand.Uint64(),
NewestQCViews: []uint64{rand.Uint64()},
NewestQC: QuorumCertificateFixture(),
SignerIndices: SignerIndicesFixture(2),
SigData: SignatureFixture(),
}
case reflect.TypeOf(time.Time{}):
return time.Now()
}
return nil
}

// generateInterfaceFlowValue generates a random value for the field of the struct that is an interface.
// This can be extended for types that are broadly used in the code base.
func generateInterfaceFlowValue(field reflect.Value) any {
if field.Type().Implements(reflect.TypeOf((*crypto.PublicKey)(nil)).Elem()) {
return KeyFixture(crypto.ECDSAP256).PublicKey()
}
return nil
}
Loading
Loading