Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced the secureStringClear mechanism with a SecureString class #1170

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mikael-s-persson
Copy link

Replaced the secureStringClear mechanism with a SecureString class

The existing mechanism isn't fully secure. The idea was to fill the old memory of sensitive strings (e.g., auth) with zeros before deallocating the memory. However, the implementation, via the secureStringClear, only really did this on the final destruction of certain objects containing those sensitive strings. That is not sufficient because assignment operations or temporary strings (e.g., urlEncode) would not get the same treatment (e.g. not clearing the old memory deallocated during assignment).

As far as I know, a more reliable (and convenient) way of doing this is through a custom allocator that zeroes out any deallocated memory, and then, define a SecureString as simply a std::string that uses that secure allocator. So, that's what I implemented in this PR.

@COM8 COM8 added this to the CPR 1.12.0 milestone Feb 3, 2025
@COM8
Copy link
Member

COM8 commented Feb 3, 2025

Oh wow @mikael-s-persson! Thank you very much for your great work!

I agree this makes total sense to do this with a custom allocator. That way we can by far better control clearing the sensitive data.

I will try to give you some feedback and make a review over the next weekend.

@mikael-s-persson
Copy link
Author

My pleasure, just a simple fix in passing.

Please note that this PR causes some breaking changes in API, like string_view instead of const string& and things like that, but I think those are unlikely to cause much breakage for users and are very simple to fix up in case of breaking when upgrading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants