Skip to content

Commit

Permalink
Fix store malfeasant ATX (#5929)
Browse files Browse the repository at this point in the history 
* Fix ATX handling of double initial ATXs
  • Loading branch information
@fasmat
fasmat authored May 13, 2024
1 parent 9aff88d commit 0d2e5c3
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 33 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

See [RELEASE](./RELEASE.md) for workflow instructions.

## Release v1.5.3

### Improvements

* [#5929](https://github.com/spacemeshos/go-spacemesh/pull/5929) Fix "no nonce" error when persisting malicious
(initial) ATXs.

## Release v1.5.2-hotfix1

This release includes our first CVE fix. A vulnerability was found in the way a node handles incoming ATXs. We urge all
Expand Down
51 changes: 32 additions & 19 deletions activation/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,32 +483,45 @@ func (h *Handler) checkWrongPrevAtx(
return nil, fmt.Errorf("%s referenced incorrect previous ATX", atx.SmesherID.ShortString())
}

// check if atx.PrevATXID is actually the last published ATX by the same node
prev, err := atxs.Get(tx, prevID)
if err != nil {
return nil, fmt.Errorf("get prev atx: %w", err)
}

// if atx references a previous ATX that is not the last ATX by the same node, there must be at least one
// atx published between prevATX and the current epoch
var atx2 *types.VerifiedActivationTx
pubEpoch := h.clock.CurrentLayer().GetEpoch()
for pubEpoch > prev.PublishEpoch {
id, err := atxs.PrevIDByNodeID(tx, atx.SmesherID, pubEpoch)
if atx.PrevATXID == types.EmptyATXID {
// if the ATX references an empty previous ATX, we can just take the initial ATX and create a proof
// that the node referenced the wrong previous ATX
id, err := atxs.GetFirstIDByNodeID(tx, atx.SmesherID)
if err != nil {
return nil, fmt.Errorf("get prev atx id by node id: %w", err)
return nil, fmt.Errorf("get initial atx id: %w", err)
}

atx2, err = atxs.Get(tx, id)
if err != nil {
return nil, fmt.Errorf("get initial atx: %w", err)
}
} else {
prev, err := atxs.Get(tx, atx.PrevATXID)
if err != nil {
return nil, fmt.Errorf("get prev atx: %w", err)
}

if atx.ID() != atx2.ID() && atx.PrevATXID == atx2.PrevATXID {
// found an ATX that points to the same previous ATX
break
// if atx references a previous ATX that is not the last ATX by the same node, there must be at least one
// atx published between prevATX and the current epoch
pubEpoch := h.clock.CurrentLayer().GetEpoch()
for pubEpoch > prev.PublishEpoch {
id, err := atxs.PrevIDByNodeID(tx, atx.SmesherID, pubEpoch)
if err != nil {
return nil, fmt.Errorf("get prev atx id by node id: %w", err)
}

atx2, err = atxs.Get(tx, id)
if err != nil {
return nil, fmt.Errorf("get prev atx: %w", err)
}

if atx.ID() != atx2.ID() && atx.PrevATXID == atx2.PrevATXID {
// found an ATX that points to the same previous ATX
break
}
pubEpoch = atx2.PublishEpoch
}
pubEpoch = atx2.PublishEpoch
}

if atx2 == nil || atx2.PrevATXID != atx.PrevATXID {
Expand All @@ -533,7 +546,7 @@ func (h *Handler) checkWrongPrevAtx(
return nil, fmt.Errorf("add malfeasance proof: %w", err)
}

h.log.WithContext(ctx).With().Warning("smesher referenced the wrong previous in published ATX",
h.log.WithContext(ctx).With().Warning("smesher referenced the wrong previous ATX in published ATX",
log.Stringer("smesher", atx.SmesherID),
log.ShortStringer("expected", prevID),
log.ShortStringer("actual", atx.PrevATXID),
Expand Down Expand Up @@ -713,7 +726,7 @@ func (h *Handler) processATX(
poetRef, atxIDs := collectAtxDeps(h.goldenATXID, &atx)
h.registerHashes(peer, poetRef, atxIDs)
if err := h.fetchReferences(ctx, poetRef, atxIDs); err != nil {
return nil, fmt.Errorf("fetching references for atx %x: %w", atx.ID(), err)
return nil, fmt.Errorf("fetching references for atx %v: %w", atx.ID(), err)
}

vAtx, proof, err := h.SyntacticallyValidateDeps(ctx, &atx)
Expand Down Expand Up @@ -768,7 +781,7 @@ func (h *Handler) fetchReferences(ctx context.Context, poetRef types.Hash32, atx
}

if err := h.fetcher.GetAtxs(ctx, atxIDs, system.WithoutLimiting()); err != nil {
return fmt.Errorf("missing atxs %x: %w", atxIDs, err)
return fmt.Errorf("missing atxs %v: %w", atxIDs, err)
}

h.log.WithContext(ctx).With().Debug("done fetching references", log.Int("fetched", len(atxIDs)))
Expand Down
128 changes: 121 additions & 7 deletions activation/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,7 @@ func TestHandler_ProcessAtx_SamePrevATX(t *testing.T) {
sig,
0,
types.EmptyATXID,
types.EmptyATXID,
goldenATXID,
nil,
types.EpochID(2),
0,
Expand Down Expand Up @@ -1167,25 +1167,135 @@ func TestHandler_ProcessAtx_SamePrevATX(t *testing.T) {
require.NoError(t, err)
require.Nil(t, proof)

// second non-initial ATX references prevATX as prevATX
// valid first non-initial ATX
atx2 := newActivationTx(
t,
sig,
1,
atx1.ID(),
atx1.ID(),
nil,
types.EpochID(4),
0,
100,
coinbase,
100,
&types.NIPost{PostMetadata: &types.PostMetadata{}},
)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
proof, err = atxHdlr.processVerifiedATX(context.Background(), atx2)
require.NoError(t, err)
require.Nil(t, proof)

// second non-initial ATX references prevATX as prevATX
atx3 := newActivationTx(
t,
sig,
2,
prevATX.ID(),
atx1.ID(),
nil,
types.EpochID(5),
0,
100,
coinbase,
100,
&types.NIPost{PostMetadata: &types.PostMetadata{}},
)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
atxHdlr.mtortoise.EXPECT().OnMalfeasance(gomock.Any())
atxHdlr.mclock.EXPECT().CurrentLayer().Return(types.EpochID(5).FirstLayer())
proof, err = atxHdlr.processVerifiedATX(context.Background(), atx3)
require.NoError(t, err)
proof.SetReceived(time.Time{})
nodeID, err := malfeasance.Validate(
context.Background(),
atxHdlr.log,
atxHdlr.cdb,
atxHdlr.edVerifier,
nil,
&mwire.MalfeasanceGossip{
MalfeasanceProof: *proof,
},
)
require.NoError(t, err)
require.Equal(t, sig.NodeID(), nodeID)
}

func TestHandler_ProcessAtx_SamePrevATX_NewInitial(t *testing.T) {
// Arrange
goldenATXID := types.ATXID{2, 3, 4}
atxHdlr := newTestHandler(t, goldenATXID)

sig, err := signing.NewEdSigner()
require.NoError(t, err)

coinbase := types.GenerateAddress([]byte("aaaa"))

// Act & Assert
prevATX := newActivationTx(
t,
sig,
0,
types.EmptyATXID,
goldenATXID,
nil,
types.EpochID(2),
0,
100,
coinbase,
100,
&types.NIPost{PostMetadata: &types.PostMetadata{}},
withVrfNonce(7),
)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
proof, err := atxHdlr.processVerifiedATX(context.Background(), prevATX)
require.NoError(t, err)
require.Nil(t, proof)

// valid first non-initial ATX
atx1 := newActivationTx(
t,
sig,
1,
prevATX.ID(),
prevATX.ID(),
nil,
types.EpochID(3),
0,
100,
coinbase,
100,
&types.NIPost{PostMetadata: &types.PostMetadata{}},
)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
proof, err = atxHdlr.processVerifiedATX(context.Background(), atx1)
require.NoError(t, err)
require.Nil(t, proof)

// second non-initial ATX references empty as prevATX
atx2 := newActivationTx(
t,
sig,
2,
types.EmptyATXID,
atx1.ID(),
nil,
types.EpochID(4),
0,
100,
coinbase,
100,
&types.NIPost{PostMetadata: &types.PostMetadata{}},
withVrfNonce(7),
)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
atxHdlr.mtortoise.EXPECT().OnMalfeasance(gomock.Any())
atxHdlr.mclock.EXPECT().CurrentLayer().Return(types.EpochID(4).FirstLayer())
proof, err = atxHdlr.processVerifiedATX(context.Background(), atx2)
require.NoError(t, err)
proof.SetReceived(time.Time{})
Expand Down Expand Up @@ -1450,8 +1560,8 @@ func BenchmarkNewActivationDb(b *testing.B) {

goldenATXID := types.ATXID{2, 3, 4}
atxHdlr := newTestHandler(b, goldenATXID)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any()).AnyTimes()
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()

const (
numOfMiners = 300
Expand Down Expand Up @@ -1486,6 +1596,8 @@ func BenchmarkNewActivationDb(b *testing.B) {
}
npst := newNIPostWithPoet(b, poetBytes)
atx = newAtx(challenge, npst.NIPost, npst.NumUnits, coinbase)
atx.VRFNonce = new(types.VRFPostIndex)
*atx.VRFNonce = types.VRFPostIndex(7)
SignAndFinalizeAtx(sigs[i], atx)
vAtx, err := atx.Verify(0, 1)
r.NoError(err)
Expand Down Expand Up @@ -1957,8 +2069,8 @@ func TestHandler_HandleSyncedAtx(t *testing.T) {
func BenchmarkGetAtxHeaderWithConcurrentProcessAtx(b *testing.B) {
goldenATXID := types.ATXID{2, 3, 4}
atxHdlr := newTestHandler(b, goldenATXID)
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any())
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any())
atxHdlr.mbeacon.EXPECT().OnAtx(gomock.Any()).AnyTimes()
atxHdlr.mtortoise.EXPECT().OnAtx(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()

var (
stop uint64
Expand All @@ -1980,6 +2092,8 @@ func BenchmarkGetAtxHeaderWithConcurrentProcessAtx(b *testing.B) {
sig, err := signing.NewEdSigner()
require.NoError(b, err)
atx := newAtx(challenge, nil, 1, types.Address{})
atx.VRFNonce = new(types.VRFPostIndex)
*atx.VRFNonce = types.VRFPostIndex(7)
require.NoError(b, SignAndFinalizeAtx(sig, atx))
vAtx, err := atx.Verify(0, 1)
if !assert.NoError(b, err) {
Expand Down
2 changes: 1 addition & 1 deletion malfeasance/wire/malfeasance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func (mp *MalfeasanceProof) MarshalLogObject(encoder log.ObjectEncoder) error {
encoder.AddString("type", "invalid prev atx")
p, ok := mp.Proof.Data.(*InvalidPrevATXProof)
if ok {
encoder.AddString("atx1_id", p.Atx2.ID().String())
encoder.AddString("atx1_id", p.Atx1.ID().String())
encoder.AddString("atx2_id", p.Atx2.ID().String())
encoder.AddString("smesher", p.Atx1.SmesherID.String())
encoder.AddString("prev_atx", p.Atx1.PrevATXID.String())
Expand Down
8 changes: 2 additions & 6 deletions sql/atxs/atxs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ func Add(db sql.Executor, atx *types.VerifiedActivationTx) error {

// AddGettingNonce adds an ATX for a given ATX ID and returns the nonce for the newly added ATX.
func AddGettingNonce(db sql.Executor, atx *types.VerifiedActivationTx) (*types.VRFPostIndex, error) {
if atx.ActivationTx.VRFNonce == nil && atx.PrevATXID != types.EmptyATXID {
if atx.VRFNonce == nil && atx.PrevATXID != types.EmptyATXID {
nonce, err := NonceByID(db, atx.PrevATXID)
if err != nil && !errors.Is(err, sql.ErrNotFound) {
return nil, fmt.Errorf("error getting nonce: %w", err)
Expand All @@ -387,11 +387,7 @@ func AddGettingNonce(db sql.Executor, atx *types.VerifiedActivationTx) (*types.V
}
}

if err := add(db, atx, atx.VRFNonce); err != nil {
return nil, err
}

return atx.VRFNonce, nil
return atx.VRFNonce, add(db, atx, atx.VRFNonce)
}

// AddMaybeNoNonce adds an ATX for a given ATX ID. It doesn't try
Expand Down

0 comments on commit 0d2e5c3

Please sign in to comment.