-
Notifications
You must be signed in to change notification settings - Fork 283
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
Remove ineffectual parameter wiping #5868
Conversation
Removes ineffectual parameter wiping introduced by 6e24ca4. Signed-off-by: Colin James <colin.barr@cloud.com>
This commit is a follow up formatting commit to make the changeset easier to review. It should be squashed! Signed-off-by: Colin James <colin.barr@cloud.com>
|
"This all makes Bigarrays ideal for storing secrets. We can be sure, that they are not moved by the GC, we can overwrite them to prevent the information leakage once they are freed." from a related StackOverflow question: https://stackoverflow.com/questions/47707142/overwriting-data-in-memory And from Edwin, some time ago:
|
@psafont informed me about the following feature which drastically reduces the visual size of the diff (the "hide whitespace" option):
Even if we did achieve it, there's likely many more buffers on the way down that contain fragments of sensitive information. The problem just gets pushed up the way if we start using |
I would agree overwriting memory is solving the problem at the wrong end. Once something is a string in OCaml you need to be very careful to not create copies and so it becomes difficult to overwrite it. And even before it reaches OCaml, it can be in memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we fixed this long ago, I remember mentioning this at the time.
But perhaps that was just in person, not on the PR.
Thanks for removing these, it gives the wrong impression to someone reading the code.
We have many copies of the password:
- in the XML/JSON buffers
- in the password strings (the bytes was just a copy, strings are immutable now)
Although some of these are short lived, the garbage collector will eventually return it to the OS, or replace it with some other allocation.
Even before having immutable strings we still had a copy in the JSON/XML buffers, so the wiping was ineffective anyway.
Strings are immutable in OCaml, so we can't overwrite them (if we want to keep them overwritable then we'd need to keep them as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of churn due to the indentation change. Other than that, I'm happy the misleading code got removed
For future reference I found the internal ticket referencing this: CP-28410, which was refered to in the linked PR |
There is code in
xapi_session.ml
that appears to attempt to wipe memory by explicitly zeroing it. However, strings have been immutable in OCaml for a long time now and so the code creates a copy of the strings' contents asbytes
(usingBytes.of_string
) and then zeroes that. I suggest we remove this code as it has never been effective at doing what it set out to do.As per a commit message, you can see that the first commit is effectively undoing the changes introduced by 6e24ca4.
I have done this in 2 commits: (1) removes the code. (2) applies formatting. This should hopefully make the changes easier to review. These should be squashed if PR is accepted and merged.