Improve visibility of IMemory security risks
This change renames the IMemory raw pointer accessors to
unsecure*() to make it apparent to coders and code reviewers
that the returned buffer may potentially be shared with
untrusted processes, who may, after the fact, attempt to
read and/or modify the contents. This may lead to hard to
find security bugs and hopefully the rename makes it harder
to forget.
The change also attempts to fix all the callsites to make
everything build correctly, but in the processes, wherever the
callsite code was not obviously secure, I added a TODO requesting
the owners to either document why it's secure or to change the
code. Apologies in advance to the owners if there are some false
positives here - I don't have enough context to reason about all
the different callsites.
Test: Completely syntactic change. Made sure code still builds.
Change-Id: If8474d0c6a2f0a23cb0514eac1a86c71e334fcc9
diff --git a/libs/binder/IMemory.cpp b/libs/binder/IMemory.cpp
index caf2318..a7662e9 100644
--- a/libs/binder/IMemory.cpp
+++ b/libs/binder/IMemory.cpp
@@ -149,7 +149,7 @@
return static_cast<char*>(base) + offset;
}
-void* IMemory::pointer() const {
+void* IMemory::unsecurePointer() const {
ssize_t offset;
sp<IMemoryHeap> heap = getMemory(&offset);
void* const base = heap!=nullptr ? heap->base() : MAP_FAILED;
@@ -158,6 +158,8 @@
return static_cast<char*>(base) + offset;
}
+void* IMemory::pointer() const { return unsecurePointer(); }
+
size_t IMemory::size() const {
size_t size;
getMemory(nullptr, &size);
diff --git a/libs/binder/include/binder/IMemory.h b/libs/binder/include/binder/IMemory.h
index 071946f..8791741 100644
--- a/libs/binder/include/binder/IMemory.h
+++ b/libs/binder/include/binder/IMemory.h
@@ -77,10 +77,33 @@
virtual sp<IMemoryHeap> getMemory(ssize_t* offset=nullptr, size_t* size=nullptr) const = 0;
// helpers
- void* fastPointer(const sp<IBinder>& heap, ssize_t offset) const;
- void* pointer() const;
+
+ // Accessing the underlying pointer must be done with caution, as there are
+ // some inherent security risks associated with it. When receiving an
+ // IMemory from an untrusted process, there is currently no way to guarantee
+ // that this process would't change the content after the fact. This may
+ // lead to TOC/TOU class of security bugs. In most cases, when performance
+ // is not an issue, the recommended practice is to immediately copy the
+ // buffer upon reception, then work with the copy, e.g.:
+ //
+ // std::string private_copy(mem.size(), '\0');
+ // memcpy(private_copy.data(), mem.unsecurePointer(), mem.size());
+ //
+ // In cases where performance is an issue, this matter must be addressed on
+ // an ad-hoc basis.
+ void* unsecurePointer() const;
+
size_t size() const;
ssize_t offset() const;
+
+private:
+ // These are now deprecated and are left here for backward-compatibility
+ // with prebuilts that may reference these symbol at runtime.
+ // Instead, new code should use unsecurePointer()/unsecureFastPointer(),
+ // which do the same thing, but make it more obvious that there are some
+ // security-related pitfalls associated with them.
+ void* pointer() const;
+ void* fastPointer(const sp<IBinder>& heap, ssize_t offset) const;
};
class BnMemory : public BnInterface<IMemory>