From 15b729b35cdf8c21770cdab6303b4bb7cad8c3c8 Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Fri, 12 Oct 2018 22:28:44 +0200 Subject: [PATCH] Fix crash in p3Peers::loadCertificateFromString This function is part of the public API so it must be safe to call with any input, before this commit if would crash if feeded with a broken or empty certificate radix string. --- libretroshare/src/pgp/rscertificate.cc | 36 +++++--- libretroshare/src/pgp/rscertificate.h | 120 +++++++++++++++---------- libretroshare/src/rsserver/p3peers.cc | 22 +++-- 3 files changed, 113 insertions(+), 65 deletions(-) diff --git a/libretroshare/src/pgp/rscertificate.cc b/libretroshare/src/pgp/rscertificate.cc index 4ead10c3e..a580f8fe8 100644 --- a/libretroshare/src/pgp/rscertificate.cc +++ b/libretroshare/src/pgp/rscertificate.cc @@ -30,6 +30,7 @@ #include #include "rscertificate.h" #include "util/rsstring.h" +#include "util/stacktrace.h" //#define DEBUG_RSCERTIFICATE @@ -88,6 +89,17 @@ void RsCertificate::addPacket(uint8_t ptag, const unsigned char *mem, size_t siz offset += size ; } +const RsCertificate&RsCertificate::operator=(const RsCertificate&) +{ + memset(ipv4_external_ip_and_port,0,6); + memset(ipv4_internal_ip_and_port,0,6); + binary_pgp_key = nullptr; + binary_pgp_key_size = 0; + only_pgp = false; + hidden_node = false; + return *this; +} + std::string RsCertificate::toStdString() const { //std::string res ; @@ -154,17 +166,21 @@ std::string RsCertificate::toStdString() const return out2 ; } -RsCertificate::RsCertificate(const std::string& str) - : - location_name(""), - pgp_version("Version: OpenPGP:SDK v0.9"), - dns_name(""),only_pgp(true) +RsCertificate::RsCertificate(const std::string& str) : + location_name(""), pgp_version("Version: OpenPGP:SDK v0.9"), + dns_name(""), only_pgp(true) { - uint32_t err_code ; - binary_pgp_key = NULL ; + uint32_t err_code; + binary_pgp_key = nullptr; - if(!initFromString(str,err_code)) - throw err_code ; + if(!initializeFromString(str, err_code)) + { + std::cerr << __PRETTY_FUNCTION__ << " is deprecated because it can " + << "miserably fail like this! str: " << str + << " err_code: " << err_code << std::endl; + print_stacktrace(); + throw err_code; + } } RsCertificate::RsCertificate(const RsPeerDetails& Detail, const unsigned char *binary_pgp_block,size_t binary_pgp_block_size) @@ -256,7 +272,7 @@ void RsCertificate::scan_ip(const std::string& ip_string, unsigned short port,un ip_and_port[5] = port & 0xff ; } -bool RsCertificate::initFromString(const std::string& instr,uint32_t& err_code) +bool RsCertificate::initializeFromString(const std::string& instr,uint32_t& err_code) { try { diff --git a/libretroshare/src/pgp/rscertificate.h b/libretroshare/src/pgp/rscertificate.h index 177440530..986f5922c 100644 --- a/libretroshare/src/pgp/rscertificate.h +++ b/libretroshare/src/pgp/rscertificate.h @@ -51,72 +51,94 @@ struct RsPeerDetails; class RsCertificate { - public: - typedef enum { RS_CERTIFICATE_OLD_FORMAT, RS_CERTIFICATE_RADIX } Format ; +public: + typedef enum { RS_CERTIFICATE_OLD_FORMAT, RS_CERTIFICATE_RADIX } Format; - // Constructs from text. - // - new format: The input string should only contain radix chars and spaces/LF/tabs. - // - explicit RsCertificate(const std::string& input_string) ; + /** + * @brief Costruct an empty certificate, use toghether with + * if(initializeFromString) for safe certificate radix string parsing + */ + RsCertificate() : + ipv4_external_ip_and_port{0,0,0,0,0,0}, + ipv4_internal_ip_and_port{0,0,0,0,0,0}, + binary_pgp_key(nullptr), binary_pgp_key_size(0), + pgp_version("Version: OpenPGP:SDK v0.9"), only_pgp(true), + hidden_node(false) {} - // Constructs from binary gpg key, and RsPeerDetails. - // - RsCertificate(const RsPeerDetails& details,const unsigned char *gpg_mem_block,size_t gpg_mem_block_size) ; + /** + * @brief Initialize from certificate string + * @param[in] str radix format string + * @param[out] errCode storage for eventual error code + * @return false on failure, true otherwise + */ + bool initializeFromString(const std::string& str, uint32_t& errCode); - // Constructs + /// Constructs from binary gpg key, and RsPeerDetails. + RsCertificate( const RsPeerDetails& details, + const unsigned char *gpg_mem_block, + size_t gpg_mem_block_size ); - virtual ~RsCertificate(); + virtual ~RsCertificate(); - // Outut to text - std::string toStdString() const ; + /// Convert to certificate radix string + std::string toStdString() const; - std::string ext_ip_string() const ; - std::string loc_ip_string() const ; - std::string location_name_string() const { return location_name; } - std::string dns_string() const { return dns_name ; } - RsPeerId sslid() const { return location_id ; } - std::string hidden_node_string() const; + std::string ext_ip_string() const; + std::string loc_ip_string() const; + std::string location_name_string() const { return location_name; } + std::string dns_string() const { return dns_name ; } + RsPeerId sslid() const { return location_id ; } + std::string hidden_node_string() const; - std::string armouredPGPKey() const ; + std::string armouredPGPKey() const; - unsigned short ext_port_us() const ; - unsigned short loc_port_us() const ; + unsigned short ext_port_us() const; + unsigned short loc_port_us() const; - const unsigned char *pgp_key() const { return binary_pgp_key ; } - size_t pgp_key_size() const { return binary_pgp_key_size ; } + const unsigned char *pgp_key() const { return binary_pgp_key ; } + size_t pgp_key_size() const { return binary_pgp_key_size ; } - static bool cleanCertificate(const std::string& input, std::string& output, RsCertificate::Format& format, int& error_code, bool check_content) ; - const std::set& locators() const { return mLocators; } + static bool cleanCertificate( + const std::string& input, std::string& output, + RsCertificate::Format& format, int& error_code, bool check_content); - private: - static bool cleanCertificate(const std::string& input,std::string& output,int&) ; // new radix format - static void scan_ip(const std::string& ip_string, unsigned short port,unsigned char *destination_memory) ; + const std::set& locators() const { return mLocators; } - bool initFromString(const std::string& str,uint32_t& err_code) ; + /** + * @deprecated using this costructor may raise exception that cause + * crash if not handled, use empty constructor + if(initFromString) for a + * safer behaviour. + */ + RS_DEPRECATED explicit RsCertificate(const std::string& input_string); - static void addPacket(uint8_t ptag, const unsigned char *mem, size_t size, unsigned char *& buf, size_t& offset, size_t& buf_size) ; +private: + // new radix format + static bool cleanCertificate( const std::string& input, + std::string& output, int&); - RsCertificate(const RsCertificate&) {} // non copy-able - const RsCertificate& operator=(const RsCertificate&) - { memset(ipv4_external_ip_and_port,0,6); memset(ipv4_internal_ip_and_port,0,6); - binary_pgp_key = NULL; binary_pgp_key_size = 0; - only_pgp = false; hidden_node = false; - return *this ;} // non copy-able + static void scan_ip( const std::string& ip_string, unsigned short port, + unsigned char *destination_memory ); - unsigned char ipv4_external_ip_and_port[6] ; - unsigned char ipv4_internal_ip_and_port[6] ; + static void addPacket(uint8_t ptag, const unsigned char *mem, size_t size, + unsigned char*& buf, size_t& offset, size_t& buf_size); - unsigned char *binary_pgp_key ; - size_t binary_pgp_key_size ; + RsCertificate(const RsCertificate&) {} /// non copy-able + const RsCertificate& operator=(const RsCertificate&); /// non copy-able - std::string location_name ; - RsPeerId location_id ; - std::string pgp_version ; - std::string dns_name ; - std::string hidden_node_address; - std::set mLocators; + unsigned char ipv4_external_ip_and_port[6]; + unsigned char ipv4_internal_ip_and_port[6]; - bool only_pgp ; // does the cert contain only pgp info? - bool hidden_node; // IP or hidden Node Address. + unsigned char *binary_pgp_key; + size_t binary_pgp_key_size; + + std::string location_name; + RsPeerId location_id; + std::string pgp_version; + std::string dns_name; + std::string hidden_node_address; + std::set mLocators; + + bool only_pgp ; /// does the cert contain only pgp info? + bool hidden_node; /// IP or hidden Node Address. }; diff --git a/libretroshare/src/rsserver/p3peers.cc b/libretroshare/src/rsserver/p3peers.cc index 3bed343af..8c9c29c8a 100644 --- a/libretroshare/src/rsserver/p3peers.cc +++ b/libretroshare/src/rsserver/p3peers.cc @@ -1213,17 +1213,27 @@ std::string p3Peers::GetRetroshareInvite( //=========================================================================== -bool p3Peers::loadCertificateFromString(const std::string& cert, RsPeerId& ssl_id, RsPgpId& gpg_id, std::string& error_string) +bool p3Peers::loadCertificateFromString( + const std::string& cert, RsPeerId& ssl_id, + RsPgpId& gpg_id, std::string& error_string ) { - RsCertificate crt(cert) ; - RsPgpId gpgid ; + RsCertificate crt; + uint32_t errNum = 0; + if(!crt.initializeFromString(cert,errNum)) + { + error_string = "RsCertificate failed with errno: " + + std::to_string(errNum) + " parsing: " + cert; + return false; + } - bool res = AuthGPG::getAuthGPG()->LoadCertificateFromString(crt.armouredPGPKey(),gpgid,error_string) ; + RsPgpId gpgid; + bool res = AuthGPG::getAuthGPG()-> + LoadCertificateFromString(crt.armouredPGPKey(), gpgid,error_string); gpg_id = gpgid; - ssl_id = crt.sslid() ; + ssl_id = crt.sslid(); - return res ; + return res; } bool p3Peers::loadDetailsFromStringCert( const std::string &certstr,