search

status-im / nim-blscurve

Nim implementation of BLS signature scheme (Boneh-Lynn-Shacham) over Barreto-Lynn-Scott (BLS) curve BLS12-381
Apache License 2.0
26 stars 11 forks source link

[SEC] Extraneous Exports and Dead Code #78

Closed eschorn1 closed 2 years ago

eschorn1 commented 4 years ago

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

Description

The common.nim source file contains a significant amount of unnecessary exports as well as unused code.

The code was installed locally and the following functions removed from export. The executable was built and the self-tests successfully run.

The above ten functions are low-level and should not be usable from outside the library. Note that this list is not meant to be exhaustive.

Additionally, a variety of low-level functions can be removed completely including both sub() and two of the four setx() functions. Again, these specific examples are not exhaustive.

Exploit Scenario

The presence of extraneous exports and dead code could indicate simple coding oversights or an incomplete code refactoring. The availability of this functionality may allow unexpected or incorrect usage outside of the intended API. If untested dead code were to become active then it may become a source of unanticipated bugs.

Mitigation Recommendation

Remove unnecessary exports and unnecessary code. Test coverage may be an informative (initial) method of identification for the latter.

References

  1. https://cwe.mitre.org/data/definitions/561.html

  2. https://github.com/status-im/nim-blscurve/blob/da9ae49dab4cbb95b2cdaa68ecc221ee15c67a9a/blscurve/miracl/common.nim

mratsim commented 2 years ago

Fixed in #100, Miracl common.nim is not reexported anymore and stays internal.

Functions like zero (used in HKDF_mod_r), bitsCount, shiftr and copy (used for hex serialization) are still used internally.