From 4154fbb8a351dcb63c6fa645f0ca9bbdae67aeb8 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 4 Jun 2016 19:19:15 +0200 Subject: [PATCH] Add conversions between wxSecretValue and wxString This is less secure, but more convenient, than using raw pointers and in most cases the password will already be stored in a wxString anyhow. --- include/wx/secretstore.h | 18 +++++++++++++ interface/wx/secretstore.h | 41 ++++++++++++++++++++++++++--- samples/secretstore/secretstore.cpp | 26 +++++++++--------- src/common/secretstore.cpp | 20 ++++++++++++++ 4 files changed, 89 insertions(+), 16 deletions(-) diff --git a/include/wx/secretstore.h b/include/wx/secretstore.h index 26e49bd342..a91410aeb1 100644 --- a/include/wx/secretstore.h +++ b/include/wx/secretstore.h @@ -14,6 +14,8 @@ #if wxUSE_SECRETSTORE +#include "wx/string.h" + class wxSecretStoreImpl; class wxSecretValueImpl; @@ -35,6 +37,13 @@ public: { } + // Creates a secret value from string. + explicit wxSecretValue(const wxString& secret) + { + const wxScopedCharBuffer buf(secret.utf8_str()); + m_impl = NewImpl(buf.length(), buf.data()); + } + wxSecretValue(const wxSecretValue& other); wxSecretValue& operator=(const wxSecretValue& other); @@ -58,10 +67,19 @@ public: // Don't assume it is NUL-terminated, use GetSize() instead. const void *GetData() const; + // Get the secret data as a string. + // + // Notice that you may want to overwrite the string contents after using it + // by calling WipeString(). + wxString GetAsString(const wxMBConv& conv = wxConvWhateverWorks) const; + // Erase the given area of memory overwriting its presumably sensitive // content. static void Wipe(size_t size, void *data); + // Overwrite the contents of the given wxString. + static void WipeString(wxString& str); + private: // This method is implemented in platform-specific code and must return a // new heap-allocated object initialized with the given data. diff --git a/interface/wx/secretstore.h b/interface/wx/secretstore.h index 1fb9fd3c35..4daa66ff58 100644 --- a/interface/wx/secretstore.h +++ b/interface/wx/secretstore.h @@ -31,13 +31,18 @@ public: The @a data argument may contain NUL bytes and doesn't need to be NUL-terminated. - - Under MSW the secret size is effectively limited to 511 bytes and - while constructing longer values will still succeed, saving it will - fail with an error. */ wxSecretValue(size_t size, const void *data); + /** + Creates a secret value from the given string. + + The @a secret argument may contain NUL bytes. + + The secret value will stored serialized in UTF-8 encoding. + */ + explicit wxSecretValue(const wxSecretValue& secret); + /** Creates a copy of an existing secret. */ @@ -88,14 +93,42 @@ public: Get read-only access to the secret data. Don't assume it is NUL-terminated, use GetSize() instead. + + @see GetAsString() */ const void *GetData() const; + /** + Get the secret data as a string. + + This is a more convenient but less secure alternative to using + GetSize() and GetData(), as this function creates another copy of a + secret which won't be wiped when this object is destroyed and you will + need to call WipeString() to overwrite the content of the returned + string, as well all its copies, if any, manually to avoid the secret + being left in memory. + + This function uses the specified @a conv object to convert binary + secret data to string form. As the secret data may have been created + by external programs not using wxWidgets API, it may be not a valid + UTF-8-encoded string, so by default ::wxConvWhateverWorks, which tries + to interpret it in any way not avoiding loss of data, is used. However + if the secrets are only saved by the program itself and are known to be + always encoded in UTF-8, it may be better to pass ::wxConvUTF8 as the + converter to use. + */ + wxString GetAsString(const wxMBConv& conv = wxConvWhateverWorks) const; + /** Erase the given area of memory overwriting its presumably sensitive content. */ static void Wipe(size_t size, void *data); + + /** + Overwrite the contents of the given string. + */ + static void WipeString(wxString& str); }; /** diff --git a/samples/secretstore/secretstore.cpp b/samples/secretstore/secretstore.cpp index bc2ed2eddd..b18e04d783 100644 --- a/samples/secretstore/secretstore.cpp +++ b/samples/secretstore/secretstore.cpp @@ -40,8 +40,13 @@ bool Save(wxSecretStore& store, const wxString& service, const wxString& user) return false; } - const size_t size = wxStrlen(password) - 1; - password[size] = 0; // Strip trailing new line. + size_t size = wxStrlen(password); + if ( size ) + { + // Strip trailing new line. + --size; + password[size] = 0; + } wxSecretValue secret(size, password); @@ -74,18 +79,15 @@ bool Load(wxSecretStore& store, const wxString& service, const wxString& user) return false; } + // Create a temporary variable just to make it possible to wipe it after + // using it. + wxString str(secret.GetAsString()); + const size_t size = secret.GetSize(); - wxPrintf("Password for %s/%s is %zu bytes long: \"", - service, user, size); + wxPrintf("Password for %s/%s is %zu bytes long: \"%s\"\n", + service, user, size, str); - // We can't easily print a non-NUL-terminated string and copying it into a - // wxString or std::string would leave the password in memory, so do it the - // hard way. - const char* p = static_cast(secret.GetData()); - for ( size_t n = 0; n < size; n++ ) - wxPutc(*p++, stdout); - - wxPrintf("\"\n"); + wxSecretValue::WipeString(str); return true; } diff --git a/src/common/secretstore.cpp b/src/common/secretstore.cpp index f6836d0ef1..d17db8d3a4 100644 --- a/src/common/secretstore.cpp +++ b/src/common/secretstore.cpp @@ -104,6 +104,19 @@ const void *wxSecretValue::GetData() const return m_impl ? m_impl->GetData() : NULL; } +wxString wxSecretValue::GetAsString(const wxMBConv& conv) const +{ + if ( !m_impl ) + return wxString(); + + return wxString + ( + static_cast(m_impl->GetData()), + conv, + m_impl->GetSize() + ); +} + #ifndef __WINDOWS__ /* static */ @@ -117,6 +130,13 @@ void wxSecretValue::Wipe(size_t size, void *data) #endif // __WINDOWS__ +/* static */ +void wxSecretValue::WipeString(wxString& str) +{ + for ( wxString::iterator it = str.begin(); it != str.end(); ++it ) + *it = '*'; +} + // ============================================================================ // wxSecretStore implementation // ============================================================================