[SEC] Missing Check in aggregate*() Which Cannot Return INVALID

[eschorn1]

labels: nbc-audit-2020-1, status:reported
labels: difficulty:high, severity:medium, type:bug

Description

The two aggregate() functions shown below as implemented in bls_signature_scheme.nim exhibit diverging behavior when presented with an empty signature set. The former will simply return while the latter will encounter a failing assertion.

proc aggregate*(agg: var AggregateSignature, sigs: openarray[Signature]) =
  ## Aggregates an array of signatures `sigs` into a signature `sig`
  for s in sigs:
    agg.point.add(s.point)

...

proc aggregate*(sigs: openarray[Signature]): Signature =
  ## Aggregates array of signatures ``sigs``
  ## and return aggregated signature.
  ##
  ## Array ``sigs`` must not be empty!
  # TODO: what is the correct empty signature to return?
  #       for now we assume that empty aggregation is handled at the client level
  doAssert(len(sigs) > 0)
  result = sigs[0]
  for i in 1 ..< sigs.len:
    result.point.add(sigs[i].point)

Separately, the aggregate*() functions are not able to signal INVALID to calling code, which is a minor divergence from the BLS Signature specification. While INVALID is primarily associated with point deserialization which is performed elsewhere and so not needed here, it also pertains to the len(sigs) == 0 condition, which the latter function handles via the assertion. Further, should Infinity signatures be handled differently in future, this code will struggle to adapt.

This issue is also present in blscurve/blst/bls_sig_min_pubkey_size_pop.nim on lines 256-279.

Exploit Scenario

The first aggregate*() function will silently accept an empty set of signatures and return. This violates the specification and may impact downstream logic which may not be hardened sufficiently to prevent unanticipated or undesirable behavior.

Mitigation Recommendation

Ensure both functions handle an empty set of signatures in the same way (ideally by returning INVALID, but an assertion is acceptable). Consider adapting the functions to return a boolean indication of success along with its result, perhaps via a nim result structure.

References

1. nim-blscurve/blscurve/miracl/bls_signature_scheme.nim

   Line 144 in da9ae49

   proc aggregate*(sigs: openarray[Signature]): Signature =

2. https://tools.ietf.org/html/draft-irtf-cfrg-bls-signature-02

3. https://github.com/arnetheduck/nim-result

[mratsim]

Fixed in #100 by introduction of composable/streaming init/aggregate/finish pattern and an "aggregateAll" function that returns a bool.

https://github.com/status-im/nim-blscurve/pull/100/files#diff-f579f7950ec0f4df1d6cdbef3eb89ab45b2206131072e38c8a0ff589f89876f9R155-R186