[CodeHealth] Use span for SecureZeroData
#23700
Conversation
|
The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "secure" and so security team members have been added as reviewers to take a look. |
cdesouza-chromium
force-pushed
the
use-span-for-secure-zero-data
branch
2 times, most recently
from
3bf7c0e
to
d8d4f62
Compare
|
Adding Chromium reviewers since this is a security-critical change, but it requires deep knowledge of Chromium abstractions in order to review. |
cdesouza-chromium
force-pushed
the
use-span-for-secure-zero-data
branch
2 times, most recently
from
0a2765a
to
0bc9ad6
Compare
| std::fill( | ||
| reinterpret_cast<volatile uint8_t*>(base::to_address(bytes.begin())), | ||
| reinterpret_cast<volatile uint8_t*>(base::to_address(bytes.end())), 0u); |
There was a problem hiding this comment.
Attached guide suggests to use std::ranges::fillbase::ranges::fill
There was a problem hiding this comment.
Why we need to_address?
There was a problem hiding this comment.
I'm using to_address to convert the typed iterators into a pointer, so I can have a pointer that I can cast to volatile. It seems to me to be a more elegant way to explicit get the iterator to yield the underlying pointer.
| SecureZeroData( | ||
| base::as_writable_bytes(UNSAFE_BUFFERS(base::make_span(p, n)))); |
There was a problem hiding this comment.
Did we have a potential bug there? It was allocating n * sizeof(T) bytes, but clearing only first n bytes. Works correctly only when sizeof(T) == 1 which seems the only way it was instantiated with.
There was a problem hiding this comment.
There was indeed a bug here that was flying undetected. There was also some very bad UB in the tests for the SecureZeroData function.
cdesouza-chromium
force-pushed
the
use-span-for-secure-zero-data
branch
from
0bc9ad6
to
edb9497
Compare
| #endif | ||
| base::ranges::fill( | ||
| reinterpret_cast<volatile uint8_t*>(base::to_address(bytes.begin())), | ||
| reinterpret_cast<volatile uint8_t*>(base::to_address(bytes.end())), 0u); |
There was a problem hiding this comment.
is this actually the required form to write this?
can this be written like this?
volatile uint8_t* begin = bytes.data();
volatile uint8_t* end = begin + bytes.size_bytes();
base::ranges::fill(begin, end, 0u);
or this?
base::span<volatile uint8_t> range = bytes;
base::ranges::fill(range, 0u);
edb9497
to
3cbd88e
Compare
`SecureZeroData` is used to clean up memory in a way the data is zeroed out once out of scope. The use of a custom allocator helps to enforce that. However the current way buffers are handled runs into issues when using `Wunsafe-buffers-usage`. This change changes the interface of this function to use `span` to communicate the buffer. Additionally, this change simplifies the way different platforms are handled by having a single handling for all of them. Finally, this change corrects the UB in the tests, where instatiated types like `vector`, and `string` were being menset to naught.
cdesouza-chromium
force-pushed
the
use-span-for-secure-zero-data
branch
from
3cbd88e
to
7bed4ec
Compare
SecureZeroDatais used to clean up memory in a way the data is zeroed out once out of scope. The use of a custom allocator helps to enforce that. However the current way buffers are handled runs into issues when usingWunsafe-buffers-usage.This change changes the interface of this function to use
spanto communicate the buffer. Additionally, this change simplifies the way different platforms are handled by having a single handling for all of them.Finally, this change corrects the UB in the tests, where instatiated types like
vector, andstringwere being menset to naught.For more information: https://chromium.googlesource.com/chromium/src/+/main/docs/unsafe_buffers.md
Resolves brave/brave-browser#38371
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: