Review of libpep by Hans
Het ziet er goed uit. Ik heb wel wat vragen/opmerkingen:
-
Ik heb
const char*
moeten veranderen inauto
inbase.cpp
regel 233 om te kunnen compileren met MSVC -
Waarom andere C++ standaard voor Android? Betekent dat niet dat je dan sowieso alleen maar C++17 features kunt gebruiken? Of, als je #ifdef gebruikt, dat je in ieder geval een C++17 implementatie moet leveren. Kun je dan niet net zo goed alleen C++17 gebruiken?
Het compileert voor mij sowieso niet met C++17:
src\core.cpp(16): error C2039: 'invalid_argument': is not a member of 'std' include\base.h(117): error C2668: 'radboud::pep::_SHA512Update': ambiguous call to overloaded function
Die eerste is makkelijk op te lossen door
#include <stdexcept>
toe te voegen. -
In
lib-common.h
: werktclang diagnostic push
ook in gcc? Dit wordt gebruikt in een blok#if defined(__clang__) || defined(__GNUC__)
-
Ik zou de voorkeur geven aan
Scalar::is_zero
, ipvScalar::zero
. Duidelijker dat het een boolean is, en niet je scalar op 0 zet oid.Scalar::valid
vind ik minder ambigu, maar zou ik dan ookis_valid
van maken. Idem voorGroupElement
. -
Ik vind
Scalar::base
een onduidelijke naam voor wat het doet. (kan aan mij liggen, misschien is het gebruikelijke terminologie)mult_base
oid zou ik duidelijker vinden -
Scalar::invert
controleert niet of de input 0 is, of de return value vancrypto_core_ristretto255_scalar_invert
die -1 returnt als de input 0 is. Moet dat niet gecontroleerd worden? -
Goed dat de error in
Scalar::base
een hint geeft over wat er mis kan zijn. Op basis van de libsodium-documentatie zou ik zelfs zeggen dat je 'probably' weg kunt laten. -
In
GroupElement::operator+
enGroupElement::operator-
wordt de return-value van de gebruikte libsodium-functie niet gecheckt. Libsodium zegt bij beide functies:The function returns 0 on success, or -1 if p and/or q are not valid encoded elements.
Ga je er vanuit dat ze nooit invalid kunnen zijn (door de check in
FromHex
)? Dit zou je expliciet kunnen maken metEXPECT
. -
Scalar::operator==
enGroupElement::operator==
zijn niet constant-time. Alle gebruikte libsodium-functies zijn dat dacht ik wel. Is dat iets wat we hier ook willen? -
In
RandomBytes
gebruik je een functie die niet in de libsodium documentatie genoemd wordt. Hij lijkt er vooral vanwege NaCl-compatibiliteit in te zitten. Waarom gebruik je nietrandombytes_buf
? -
RerandomizeY
voert een voor mij onbekende actie uit. De functie wordt ook nergens gebruikt, en staat ook niet incore.h
. Dus wat is hier precies de bedoeling van?