PG-2512: Secure-erase sensitive data#39
Conversation
Since the library deals with (private) keys and other possible secure data, we should make sure that everything we do uses a secure erase allocator. This is an API-breaking change since we have to modify the interface to use proper allocators.
jeltz
left a comment
There was a problem hiding this comment.
Not a full review, jsut took a quick look. One of the trickiest parts with reviewing this is making sure we have covered everything which may affect the public interfaces.
| ::memset_s(p, n, 0, n); | ||
| #elif defined(__GLIBC__) && \ | ||
| (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25)) | ||
| // glibc >= 2.25 and the BSDs provide explicit_bzero. |
There was a problem hiding this comment.
Comment is a bit funny as is is gated behind glibc only.
| } | ||
| // Compiler barrier so the writes are not reordered/coalesced away. | ||
| __asm__ __volatile__("" : : "r"(p) : "memory"); | ||
| #endif |
There was a problem hiding this comment.
Do we really need the fallback? glibc 2.25 was released in 2017. Does our C++ even compile on such ancient platforms?
| * dead-store elimination target: the optimizer sees the memory is never read | ||
| * again and removes the write, leaving secrets in place. secure_clear routes | ||
| * through a platform "explicit" zeroing primitive when available and falls | ||
| * back to a volatile write loop otherwise, so the store always happens. |
There was a problem hiding this comment.
I feel this comment is a bit unnecessary and can be shortened.
|
From my point of view, this PR looks good, and I'd rather approve it, But, it changes the interface a little bit, so I'm asking to postpone this PR for some time. Let's make all changes that do not break the interface, then our QA and I will test these changes with Perona Server (8.4, 9.7 and 10.x) , and then we can apply this PR. And it it good idea to request a review from Github Copilot here, it has reasonable comments. |
|
@lukin-oleksiy okay, I'll create other PRs then. I was waiting with those not to cause a conflict with this, but I can rebase this one later. |
Since the library deals with (private) keys and other possible secure data, we should make sure that everything we do uses a secure erase allocator.
This is an API-breaking change since we have to modify the interface to use proper allocators.