Skip to content

Support reliable zeroing of sensitive buffers#758

Closed
devnexen wants to merge 8 commits intosquid-cache:masterfrom
devnexen:clear_mem
Closed

Support reliable zeroing of sensitive buffers#758
devnexen wants to merge 8 commits intosquid-cache:masterfrom
devnexen:clear_mem

Conversation

@devnexen
Copy link
Contributor

@devnexen devnexen commented Jan 17, 2021 •
edited by rousskov
Loading

TODO: Use the new API for more sensible buffers, possibly adding a
wrapper class for sensitive content to automate cleanup.

@squid-prbot
Copy link
Collaborator

squid-prbot commented Jan 17, 2021

Can one of the admins verify this patch?

yadij
yadij requested changes Jan 17, 2021
View reviewed changes
@devnexen devnexen force-pushed the clear_mem branch 2 times, most recently from dff6945 to fa3086b Compare January 17, 2021 12:56
rousskov
rousskov requested changes Jan 17, 2021
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please edit the PR description to explain what this PR is trying to accomplish. I am very worried that I am reviewing this PR incorrectly due to the lack of explicit PR goals/intent statements. (That description will automatically become the future official commit message when this PR is merged as documented in MergeProcedure)

Some of my change requests may become inapplicable or may need adjustment once the PR goals are clarified. You may ignore my inlined change requests until we settle the PR intent issue.

yadij
yadij requested changes Jan 17, 2021
View reviewed changes
@devnexen devnexen force-pushed the clear_mem branch 2 times, most recently from d08eea4 to b2c6a61 Compare January 18, 2021 04:09
yadij
yadij reviewed Jan 18, 2021
View reviewed changes
@devnexen devnexen force-pushed the clear_mem branch 2 times, most recently from d966e29 to 4c13b71 Compare January 18, 2021 18:46
yadij
yadij requested changes Jan 28, 2021
View reviewed changes
@yadij yadij changed the title Proposal of ClearMem introduction for few usages. Support memset_s() for clearing sensitive buffers Jan 28, 2021
@devnexen
Copy link
Contributor Author

devnexen commented Jan 28, 2021

ah my bad I forgot this one.

@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jan 28, 2021
@yadij
Copy link
Contributor

yadij commented Jan 28, 2021

@rousskov, please re-review. LGTM, so when you are happy it can go in.

@yadij yadij requested a review from rousskov January 28, 2021 05:30
rousskov
rousskov requested changes Jan 28, 2021
View reviewed changes
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jan 28, 2021
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 30, 2021
@devnexen
Copy link
Contributor Author

devnexen commented Feb 11, 2021

Is there any remaining concern as there is waiting for author label ? :)

@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels May 19, 2021
rousskov
rousskov requested changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure this PR was waiting for me, but I am trying to push it forward.

@devnexen
Copy link
Contributor Author

devnexen commented Jul 21, 2021

I had tried but failed with linkage, so many dependencies.

rousskov
rousskov previously requested changes Jul 21, 2021
View reviewed changes
@devnexen devnexen force-pushed the clear_mem branch 2 times, most recently from ee51393 to e1cce38 Compare July 21, 2021 21:13
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 15, 2021
@rousskov
fixup: Document the new function
e4db81f 
I also made the documentation a bit more concise.
@rousskov rousskov dismissed their stale review December 14, 2021 20:18

Thank you for addressing all my concerns.

@rousskov rousskov removed the S-waiting-for-author author action is expected (and usually required) label Dec 14, 2021
@rousskov rousskov changed the title Support memset_s() for clearing sensitive buffers Support reliable zeroing of sensitive buffers Dec 14, 2021
@rousskov
Copy link
Contributor

rousskov commented Dec 14, 2021

I have added the new function documentation in commit e4db81f and adjusted PR title/description (i.e. the future commit message) to match the current PR code and acknowledge that we are far from done here. Please adjust further as needed.

I also tested the new code and can confirm that, unlike a bare call to std::memset(), the call to the new function is not optimized away in a basic use case.

@yadij, the ball is on your side AFAICT.

@rousskov
Copy link
Contributor

rousskov commented Dec 14, 2021

OK to test

yadij
yadij requested changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor typo.

@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jan 17, 2022
@devnexen
Copy link
Contributor Author

devnexen commented Jan 17, 2022

wow exactly one year later :-)

@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Feb 3, 2022
squid-anubis pushed a commit that referenced this pull request Feb 3, 2022
@devnexen @squid-anubis
Support reliable zeroing of sensitive buffers (#758)
2d6a38a 
TODO: Use the new API for more sensible buffers, possibly adding a
wrapper class for sensitive content to automate cleanup.
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadij yadij yadij approved these changes

@rousskov rousskov rousskov left review comments

Assignees

No one assigned

Labels

M-merged https://github.com/measurement-factory/anubis#pull-request-labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@devnexen @squid-prbot @yadij @rousskov @squid-anubis @eduard-bagdasaryan