Fix sensitive data leaking in Authentication

[jagerman]

The current Authentication constructor has multiple points where a copy can get made: in the arguments themselves, in the intermediate concatenations, and in the potential need for the concatenation to copy itself during a memory reallocation.

An additional copy of the auth data could end up unwiped in the implicit move constructor/assignment (in particular when small string optimization applies to the value).

Any such copies end up potentially leaving the sensitive data behind in memory, undermining the changes in #776 that were trying to securely erase such sensitive data.

This commit avoids any such copies by:

• changing Authentication to take std::string_views (instead of std::string) for username and password so that no copy of input will be done
• properly reserving auth_string_ to its required size before building it
• Adding an explicit move constructor that resizes the moved-from auth string to capacity to ensure it gets erased when SSO applies.
• Adding an explicit move assignment operator that wipes the current value before replacing it, and properly resizes the moved-from string to capacity to ensure it gets wiped when SSO applies.

[COM8]

Good idea, thanks @jagerman!

I found two use after move cases that you want to look at other than that LGTM.

[COM8]

This is probably not what you want since after moving old.auth_string_ is not guaranteed thet old.auth_string_.capacity() returns any value that makes sense.

Better:

Suggested change

	Authentication::Authentication(Authentication&& old) noexcept : auth_string_{std::move(old.auth_string_)}, auth_mode_{old.auth_mode_} {
	old.auth_string_.resize(old.auth_string_.capacity());
	}
	Authentication::Authentication(Authentication&& old) noexcept auth_mode_{old.auth_mode_} {
	auth_string_ = old.auth_string_;
	util::secureStringClear(old.auth_string_);
	}

[jagerman]

This is probably not what you want since after moving old.auth_string_ is not guaranteed thet old.auth_string_.capacity() returns any value that makes sense.

I think it still makes sense: the moved-from string is still required to have a valid state. It'll almost certainly return the capacity of an SSO string (since it either was an SSO string to begin with, or else transferred away its pointer and would have returned to being a 0-length SSO string).

In the case that it was SSO to start with, the old value will still be there, inside the SSO storage of the string itself. If it wasn't SSO to start with, this is indeed a useless operation -- but unfortunately there's no interface to ask std::string if it was applying SSO or not.

However, if you'd rather not invoke the string's move constructor like this then I think we should just delete the move constructor and assignment from Authentication entirely as, with a copy of the string like in the suggestion, it isn't actually doing anything that a copy constructor wouldn't do anyway. (The only effect would be a slight reordering of calling secureStringClear here instead of waiting for it to happen in the destruction of the copied-from object).

[jagerman]

Another option that might be preferable: change auth_string_ to a std::vector<char>, which does have a guarantee that the memory only exists in one place and is always transferred by a move.

That would actually simplify this quite a bit: we could use default move constructors that would do the right thing.

[jagerman]

I've pushed the std::vector version, currently as a separate commit for discussion, but I'll squash those if it looks good.

[COM8]

I see your point there and the std::vector approach somehow resolves it. But I really do not like this approach since it does not feel like C++ it feels like C .

On the String move. There are no guarantees std::move gives you when moving an object that any of it's components might still be intact, even if we consider different implementations of std::string. So a potential use after move is always a bad idea in my eyes.

Deleting the move ctr is an option but I see my solution for the problem as a good and secure version where we do not have to make any assumptions.

Thanks for taking the time! I really appreciate it!

[jagerman]

Deleting the move ctr is an option but I see my solution for the problem as a good and secure version where we do not have to make any assumptions.

I don't really like this move constructor approach because it signals efficient move semantics but then doesn't actually provide them (because the implement always copies); and so better I think to just delete the move ctor/assignment if you want to keep this as a string.

[COM8]

Suggested change

	auth_string_ = std::move(old.auth_string_);
	old.auth_string_.resize(old.auth_string_.capacity());
	auth_string_ = old.auth_string_;
	util::secureStringClear(old.auth_string_);

Again use after moved.

[COM8]

Yes, let us delete the move semantics ctr and switch back from the std::vector based solution.

[jagerman]

Updated to work with std::strings and without any (problematic) std::string moves.

[COM8]

@jagerman Thanks!