Skip to content

Commit f5000f3

Browse files
Hayim.Shaul@ibm.comAkramBitar
authored andcommitted
fix(fabtoken): guard TransferHTLCValidate against missing input signatures
TransferHTLCValidate indexed ctx.Signatures with the ctx.InputTokens loop index without a bounds check. For an HTLC-owned input at index i, a nil or short ctx.Signatures made the validator panic with index-out-of-range instead of returning a validation error. The equivalent zkatdlog validator already guards this case. Add the same bounds check, plus a nil guard for entries of ctx.InputTokens, whose owner was dereferenced unconditionally in the same loop. Neither state is reachable through the default pipeline, where TransferSignatureValidate populates one signature per input before this step runs, so this is a defense-in-depth fix for reordered or custom validation pipelines. Also document, in the validator extension guide, that a validation function must not assume other pipeline steps have already run. Fixes #2032 Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
1 parent 42aefe6 commit f5000f3

3 files changed

Lines changed: 105 additions & 1 deletion

File tree

docs/drivers/extending_validator.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ func MyCustomTransferValidation(ctx validator.Context, tr *transfer.Action) erro
6969
}
7070
```
7171

72+
A validation function must not assume that any other step of the pipeline has already run:
73+
fields that earlier steps populate (`validator.Context.InputTokens`, `Context.Signatures`, ...)
74+
may be empty or shorter than expected, so bound-check them and return an error instead of
75+
indexing blindly.
76+
7277
### 2. Create a custom Validator Driver
7378

7479
Implement the `driver.ValidatorDriver` interface by wrapping the standard one.

token/core/fabtoken/v1/validator/validator_test.go

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -997,6 +997,93 @@ func TestTransferHTLCValidate(t *testing.T) {
997997
require.Error(t, err)
998998
assert.Contains(t, err.Error(), "expiration date has already passed")
999999
})
1000+
1001+
t.Run("MissingSignature_NoPanic", func(t *testing.T) {
1002+
sender, _ := identity.WrapWithType(x509.IdentityType, []byte("sender"))
1003+
htlcOwner := newExpiredHTLCOwner(t, sender)
1004+
1005+
ta := &actions.TransferAction{
1006+
Outputs: []*actions.Output{
1007+
{Owner: sender, Type: "ABC", Quantity: "100"},
1008+
},
1009+
}
1010+
c := &validator.Context{
1011+
TransferAction: ta,
1012+
InputTokens: []*actions.Output{{Owner: htlcOwner, Type: "ABC", Quantity: "100"}},
1013+
Signatures: nil,
1014+
MetadataCounter: make(map[string]int),
1015+
}
1016+
err := validator.TransferHTLCValidate(ctx, c)
1017+
require.Error(t, err)
1018+
assert.Contains(t, err.Error(), "missing signature for input at index [0]")
1019+
})
1020+
1021+
t.Run("ShortSignatures_NoPanic", func(t *testing.T) {
1022+
sender, _ := identity.WrapWithType(x509.IdentityType, []byte("sender"))
1023+
htlcOwner := newExpiredHTLCOwner(t, sender)
1024+
1025+
ta := &actions.TransferAction{
1026+
Outputs: []*actions.Output{
1027+
{Owner: sender, Type: "ABC", Quantity: "100"},
1028+
},
1029+
}
1030+
// two htlc-owned inputs but only one signature: the second input must
1031+
// return an error instead of indexing past the end of ctx.Signatures
1032+
c := &validator.Context{
1033+
TransferAction: ta,
1034+
InputTokens: []*actions.Output{
1035+
{Owner: htlcOwner, Type: "ABC", Quantity: "100"},
1036+
{Owner: htlcOwner, Type: "ABC", Quantity: "100"},
1037+
},
1038+
Signatures: [][]byte{[]byte("sig")},
1039+
MetadataCounter: make(map[string]int),
1040+
}
1041+
err := validator.TransferHTLCValidate(ctx, c)
1042+
require.Error(t, err)
1043+
assert.Contains(t, err.Error(), "missing signature for input at index [1]")
1044+
})
1045+
1046+
t.Run("NilInputToken_NoPanic", func(t *testing.T) {
1047+
owner1, _ := identity.WrapWithType(x509.IdentityType, []byte("owner1"))
1048+
ta := &actions.TransferAction{
1049+
Outputs: []*actions.Output{{Owner: owner1, Type: "ABC", Quantity: "100"}},
1050+
}
1051+
c := &validator.Context{
1052+
TransferAction: ta,
1053+
InputTokens: []*actions.Output{nil},
1054+
MetadataCounter: make(map[string]int),
1055+
}
1056+
err := validator.TransferHTLCValidate(ctx, c)
1057+
require.Error(t, err)
1058+
assert.Contains(t, err.Error(), "nil input token at index [0]")
1059+
})
1060+
}
1061+
1062+
// newExpiredHTLCOwner returns an htlc-script identity whose deadline has already
1063+
// passed, so that a transfer back to the sender is validated as a reclaim.
1064+
func newExpiredHTLCOwner(t *testing.T, sender driver.Identity) driver.Identity {
1065+
t.Helper()
1066+
1067+
recipient, err := identity.WrapWithType(x509.IdentityType, []byte("recipient"))
1068+
require.NoError(t, err)
1069+
hash := crypto.SHA256.New()
1070+
hash.Write([]byte("preimage"))
1071+
script := &htlc.Script{
1072+
Sender: sender,
1073+
Recipient: recipient,
1074+
Deadline: time.Now().Add(-1 * time.Hour), // expired
1075+
HashInfo: htlc.HashInfo{
1076+
Hash: hash.Sum(nil),
1077+
HashFunc: crypto.SHA256,
1078+
HashEncoding: encoding.Base64,
1079+
},
1080+
}
1081+
scriptBytes, err := json.Marshal(script)
1082+
require.NoError(t, err)
1083+
htlcOwner, err := identity.WrapWithType(htlc.ScriptType, scriptBytes)
1084+
require.NoError(t, err)
1085+
1086+
return htlcOwner
10001087
}
10011088

10021089
// BenchmarkValidatorTransfer benchmarks the verification of a transfer token request.

token/core/fabtoken/v1/validator/validator_transfer.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,11 +150,18 @@ func TransferBalanceValidate(c context.Context, ctx *Context) error {
150150
return nil
151151
}
152152

153-
// TransferHTLCValidate checks the validity of the HTLC scripts, if any
153+
// TransferHTLCValidate checks the validity of the HTLC scripts, if any.
154+
// A nil input token or a signature missing at the index of an HTLC-owned input
155+
// yields a validation error rather than a panic, regardless of the order in which
156+
// the validation steps of the pipeline are executed.
154157
func TransferHTLCValidate(c context.Context, ctx *Context) error {
155158
now := time.Now()
156159

157160
for i, in := range ctx.InputTokens {
161+
// guard: a nil token in the input slice must return an error, not panic
162+
if in == nil {
163+
return errors.Errorf("nil input token at index [%d]", i)
164+
}
158165
owner, err := identity.UnmarshalTypedIdentity(in.GetOwner())
159166
if err != nil {
160167
return errors.Wrap(err, "failed to unmarshal owner of input token")
@@ -186,6 +193,11 @@ func TransferHTLCValidate(c context.Context, ctx *Context) error {
186193
}
187194

188195
// check metadata
196+
// guard against a missing signature at index i (e.g., when this validator
197+
// runs without TransferSignatureValidate having populated ctx.Signatures)
198+
if i >= len(ctx.Signatures) {
199+
return errors.Errorf("missing signature for input at index [%d]", i)
200+
}
189201
sigma := ctx.Signatures[i]
190202
metadataKey, err := htlc2.MetadataClaimKeyCheck(ctx.TransferAction, script, op, sigma)
191203
if err != nil {

0 commit comments

Comments
 (0)