From 4de049820866d83b7dd40084adf7db23cd79acb2 Mon Sep 17 00:00:00 2001 From: csoler Date: Fri, 5 Oct 2018 16:54:36 +0200 Subject: [PATCH 1/2] fixed bad signature checking code for config files --- libretroshare/src/pqi/p3cfgmgr.cc | 33 ++++++++++++++++++++----------- libretroshare/src/pqi/pqibin.h | 1 - libretroshare/src/util/rsprint.cc | 27 +++++++++++++++++++++++++ libretroshare/src/util/rsprint.h | 1 + 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/libretroshare/src/pqi/p3cfgmgr.cc b/libretroshare/src/pqi/p3cfgmgr.cc index d41a47c80..687e643bf 100644 --- a/libretroshare/src/pqi/p3cfgmgr.cc +++ b/libretroshare/src/pqi/p3cfgmgr.cc @@ -299,26 +299,37 @@ bool p3Config::loadAttempt(const std::string& cfgFname,const std::string& signFn /* set hash */ setHash(bio->gethash()); + // In order to check the signature that is stored on disk, we compute the hash of the current data (which should match the hash of the data on disc because we just read it), + // and validate the signature from the disk on this data. + std::string signatureRead; RsFileHash strHash(Hash()); - AuthSSL::getAuthSSL()->SignData(strHash.toByteArray(), RsFileHash::SIZE_IN_BYTES, signatureRead); - BinMemInterface *signbio = new BinMemInterface(signatureRead.size(), BIN_FLAGS_READABLE); + BinFileInterface bfi(signFname.c_str(), BIN_FLAGS_READABLE); - if(!signbio->readfromfile(signFname.c_str())) - { - delete signbio; + if(bfi.getFileSize() == 0) + return false ; + + RsTemporaryMemory mem(bfi.getFileSize()) ; + + if(!bfi.readdata(mem,mem.size())) return false; + + // signature is stored as ascii so we need to convert it back to binary + + RsTemporaryMemory mem2(bfi.getFileSize()/2) ; + + if(!RsUtil::HexToBin(std::string((char*)(unsigned char*)mem,mem.size()),mem2,mem2.size())) + { + std::cerr << "Input string is not a Hex string!!"<< std::endl; + return false ; } - std::string signatureStored((char *) signbio->memptr(), signbio->memsize()); + bool signature_checks = AuthSSL::getAuthSSL()->VerifyOwnSignBin(strHash.toByteArray(), RsFileHash::SIZE_IN_BYTES,mem2,mem2.size()); - delete signbio; + std::cerr << "(II) checked signature of config file " << cfgFname << ": " << (signature_checks?"OK":"Wrong!") << std::endl; - if(signatureRead != signatureStored) - return false; - - return true; + return signature_checks; } bool p3Config::saveConfiguration() diff --git a/libretroshare/src/pqi/pqibin.h b/libretroshare/src/pqi/pqibin.h index b8cbb1c58..349a049ba 100644 --- a/libretroshare/src/pqi/pqibin.h +++ b/libretroshare/src/pqi/pqibin.h @@ -69,7 +69,6 @@ virtual bool bandwidthLimited() { return false; } virtual RsFileHash gethash(); virtual uint64_t bytecount(); -protected: virtual uint64_t getFileSize(); private: diff --git a/libretroshare/src/util/rsprint.cc b/libretroshare/src/util/rsprint.cc index dca3d925c..88ee27ca5 100644 --- a/libretroshare/src/util/rsprint.cc +++ b/libretroshare/src/util/rsprint.cc @@ -104,6 +104,33 @@ std::string RsUtil::HashId(const std::string &id, bool reverse) return hash; } +static int toHalfByte(char u,bool& ok) +{ + if(u >= 'a' && u <= 'f') return u-'a' + 0xa; + if(u >= 'A' && u <= 'F') return u-'A' + 0xa; + if(u >= '0' && u <= '9') return u-'0' + 0x0; + + ok = false ; + + return 0; +} + +bool RsUtil::HexToBin(const std::string& input,unsigned char *data, const uint32_t len) +{ + if(input.size() & 1) + return false ; + + if(len != input.size()/2) + return false ; + + bool ok = true ; + + for(uint32_t i=0;(i0 and len>max_len, only the first "max_len" bytes are writen to the string and "..." is happened. std::string BinToHex(const unsigned char *arr, const uint32_t len, uint32_t max_len=0); +bool HexToBin(const std::string& input,unsigned char *data, const uint32_t len); std::string NumberToString(uint64_t n, bool hex=false); std::string HashId(const std::string &id, bool reverse = false); std::vector BinToSha256(const std::vector &in); From dbd52c0c9c79d96d82d46c2fb804ec4cccfa3465 Mon Sep 17 00:00:00 2001 From: csoler Date: Fri, 5 Oct 2018 17:01:54 +0200 Subject: [PATCH 2/2] fixed bug in previous commit --- libretroshare/src/pqi/p3cfgmgr.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libretroshare/src/pqi/p3cfgmgr.cc b/libretroshare/src/pqi/p3cfgmgr.cc index 687e643bf..f7f7f6ebf 100644 --- a/libretroshare/src/pqi/p3cfgmgr.cc +++ b/libretroshare/src/pqi/p3cfgmgr.cc @@ -300,7 +300,8 @@ bool p3Config::loadAttempt(const std::string& cfgFname,const std::string& signFn setHash(bio->gethash()); // In order to check the signature that is stored on disk, we compute the hash of the current data (which should match the hash of the data on disc because we just read it), - // and validate the signature from the disk on this data. + // and validate the signature from the disk on this data. The config file data is therefore hashed twice. Not a security issue, but + // this is a bit inelegant. std::string signatureRead; RsFileHash strHash(Hash());