Skip to content

Commit

Permalink
ASB - Security Patch integration November 2023
Browse files Browse the repository at this point in the history
Integrating Security Patches

Test done: STS TCs Passed

Tracked-On: OAM-112870
Signed-off-by: Alam, SahibeX <[email protected]>
  • Loading branch information
AlamIntel committed Oct 27, 2023
1 parent ed61179 commit ec718b8
Show file tree
Hide file tree
Showing 14 changed files with 2,659 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ index 47bb92c142..2d0ac256a4 100644
# It must match one of the Android Security Patch Level strings of the Public Security Bulletins.
# If there is no $PLATFORM_SECURITY_PATCH set, keep it empty.
- PLATFORM_SECURITY_PATCH := 2022-02-05
+ PLATFORM_SECURITY_PATCH := 2023-10-01
+ PLATFORM_SECURITY_PATCH := 2023-11-01
endif
.KATI_READONLY := PLATFORM_SECURITY_PATCH

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
From c9558b05adfe11520e8257ebb061ebeffae4f1bb Mon Sep 17 00:00:00 2001
From: Shruti Bihani <[email protected]>
Date: Mon, 10 Jul 2023 08:53:42 +0000
Subject: [PATCH] Fix for heap buffer overflow issue flagged by fuzzer test.

OOB write occurs when a value is assigned to a buffer index which is greater than the buffer size. Adding a check on buffer bounds fixes the issue.

Similar checks have been added wherever applicable on other such methods of the class.

Bug: 243463593
Test: Build mtp_packet_fuzzer and run on the target device
(cherry picked from commit a669e34bb8e6f0f7b5d7a35144bd342271a24712)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:38a83caefc4b5fd5aa1071bbabf0c71f49e6ac80)
Merged-In: Icd0f2307803a1a35e655bc08d9d4cca5e2b58a9b
Change-Id: Icd0f2307803a1a35e655bc08d9d4cca5e2b58a9b
---
media/mtp/MtpPacket.cpp | 40 +++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/media/mtp/MtpPacket.cpp b/media/mtp/MtpPacket.cpp
index f069a83b5f..5faaac2026 100644
--- a/media/mtp/MtpPacket.cpp
+++ b/media/mtp/MtpPacket.cpp
@@ -92,24 +92,46 @@ void MtpPacket::copyFrom(const MtpPacket& src) {
}

uint16_t MtpPacket::getUInt16(int offset) const {
- return ((uint16_t)mBuffer[offset + 1] << 8) | (uint16_t)mBuffer[offset];
+ if ((unsigned long)(offset+2) <= mBufferSize) {
+ return ((uint16_t)mBuffer[offset + 1] << 8) | (uint16_t)mBuffer[offset];
+ }
+ else {
+ ALOGE("offset for buffer read is greater than buffer size!");
+ abort();
+ }
}

uint32_t MtpPacket::getUInt32(int offset) const {
- return ((uint32_t)mBuffer[offset + 3] << 24) | ((uint32_t)mBuffer[offset + 2] << 16) |
- ((uint32_t)mBuffer[offset + 1] << 8) | (uint32_t)mBuffer[offset];
+ if ((unsigned long)(offset+4) <= mBufferSize) {
+ return ((uint32_t)mBuffer[offset + 3] << 24) | ((uint32_t)mBuffer[offset + 2] << 16) |
+ ((uint32_t)mBuffer[offset + 1] << 8) | (uint32_t)mBuffer[offset];
+ }
+ else {
+ ALOGE("offset for buffer read is greater than buffer size!");
+ abort();
+ }
}

void MtpPacket::putUInt16(int offset, uint16_t value) {
- mBuffer[offset++] = (uint8_t)(value & 0xFF);
- mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF);
+ if ((unsigned long)(offset+2) <= mBufferSize) {
+ mBuffer[offset++] = (uint8_t)(value & 0xFF);
+ mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF);
+ }
+ else {
+ ALOGE("offset for buffer write is greater than buffer size!");
+ }
}

void MtpPacket::putUInt32(int offset, uint32_t value) {
- mBuffer[offset++] = (uint8_t)(value & 0xFF);
- mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF);
- mBuffer[offset++] = (uint8_t)((value >> 16) & 0xFF);
- mBuffer[offset++] = (uint8_t)((value >> 24) & 0xFF);
+ if ((unsigned long)(offset+4) <= mBufferSize) {
+ mBuffer[offset++] = (uint8_t)(value & 0xFF);
+ mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF);
+ mBuffer[offset++] = (uint8_t)((value >> 16) & 0xFF);
+ mBuffer[offset++] = (uint8_t)((value >> 24) & 0xFF);
+ }
+ else {
+ ALOGE("offset for buffer write is greater than buffer size!");
+ }
}

uint16_t MtpPacket::getContainerCode() const {
--
2.42.0.515.g380fc7ccd1-goog

Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
From 38888a8b8bbb0008714539ea141c5341d8e35ff5 Mon Sep 17 00:00:00 2001
From: Shruti Bihani <[email protected]>
Date: Thu, 13 Jul 2023 09:19:08 +0000
Subject: [PATCH] Fix heap-use-after-free issue flagged by fuzzer test.

A data member of class MtpFfsHandle is being accessed after the class object has been freed in the fuzzer. The method accessing the data member is running in a separate thread that gets detached from its parent. Using a conditional variable with an atomic int predicate in the close() function to ensure the detached thread's execution has completed before freeing the object fixes the issue without blocking the processing mid-way.

Bug: 243381410
Test: Build mtp_handle_fuzzer and run on the target device
(cherry picked from commit 50bf46a3f62136386548a9187a749936bda3ee8f)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:e376b3dd401339cf736b1f76948b2f2338a647c9)
Merged-In: I41dde165a5eba151c958b81417d9e1065af1b411
Change-Id: I41dde165a5eba151c958b81417d9e1065af1b411
---
media/mtp/MtpFfsHandle.cpp | 14 ++++++++++++++
media/mtp/MtpFfsHandle.h | 4 ++++
2 files changed, 18 insertions(+)

diff --git a/media/mtp/MtpFfsHandle.cpp b/media/mtp/MtpFfsHandle.cpp
index 2ffd7759e0..ef8c9aa789 100644
--- a/media/mtp/MtpFfsHandle.cpp
+++ b/media/mtp/MtpFfsHandle.cpp
@@ -297,6 +297,10 @@ int MtpFfsHandle::start(bool ptp) {
}

void MtpFfsHandle::close() {
+ auto timeout = std::chrono::seconds(2);
+ std::unique_lock lk(m);
+ cv.wait_for(lk, timeout ,[this]{return child_threads==0;});
+
io_destroy(mCtx);
closeEndpoints();
closeConfig();
@@ -669,6 +673,11 @@ int MtpFfsHandle::sendEvent(mtp_event me) {
char *temp = new char[me.length];
memcpy(temp, me.data, me.length);
me.data = temp;
+
+ std::unique_lock lk(m);
+ child_threads++;
+ lk.unlock();
+
std::thread t([this, me]() { return this->doSendEvent(me); });
t.detach();
return 0;
@@ -680,6 +689,11 @@ void MtpFfsHandle::doSendEvent(mtp_event me) {
if (static_cast<unsigned>(ret) != length)
PLOG(ERROR) << "Mtp error sending event thread!";
delete[] reinterpret_cast<char*>(me.data);
+
+ std::unique_lock lk(m);
+ child_threads--;
+ lk.unlock();
+ cv.notify_one();
}

} // namespace android
diff --git a/media/mtp/MtpFfsHandle.h b/media/mtp/MtpFfsHandle.h
index e552e03bec..51cdef0953 100644
--- a/media/mtp/MtpFfsHandle.h
+++ b/media/mtp/MtpFfsHandle.h
@@ -60,6 +60,10 @@ protected:
bool mCanceled;
bool mBatchCancel;

+ std::mutex m;
+ std::condition_variable cv;
+ std::atomic<int> child_threads{0};
+
android::base::unique_fd mControl;
// "in" from the host's perspective => sink for mtp server
android::base::unique_fd mBulkIn;
--
2.42.0.515.g380fc7ccd1-goog

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
From 911ecf2f0a94045acd4b93185fb911631a15a322 Mon Sep 17 00:00:00 2001
From: Venkatarama Avadhani <[email protected]>
Date: Fri, 11 Aug 2023 15:19:24 +0000
Subject: [PATCH] Initialise VPS buffer to NULL in constructor

Missing initialisation of this pointer could lead to an incorrect free
if the ARTWriter object is cleared immeddiately after the constructor
call.

Bug: 287298721
Test: rtp_writer_fuzzer
(cherry picked from https://partner-android-review.googlesource.com/q/commit:2710696b001f2e95586151c1ee337a4e3c4da48a)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:900195c1d3589c7cbf9e116f61bebaefc0519101)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0efe2b4d6b739650039c2cab176ef11d5f5ac49c)
Merged-In: I08eacd7a0201bc9a41b821e20cae916d8870147a
Change-Id: I08eacd7a0201bc9a41b821e20cae916d8870147a
---
media/libstagefright/rtsp/ARTPWriter.cpp | 1 +
1 file changed, 1 insertion(+)

diff --git a/media/libstagefright/rtsp/ARTPWriter.cpp b/media/libstagefright/rtsp/ARTPWriter.cpp
index 11c7aeb9fc..1e08606407 100644
--- a/media/libstagefright/rtsp/ARTPWriter.cpp
+++ b/media/libstagefright/rtsp/ARTPWriter.cpp
@@ -105,6 +105,7 @@ ARTPWriter::ARTPWriter(int fd)

mRTCPAddr = mRTPAddr;
mRTCPAddr.sin_port = htons(ntohs(mRTPAddr.sin_port) | 1);
+ mVPSBuf = NULL;
mSPSBuf = NULL;
mPPSBuf = NULL;

--
2.42.0.515.g380fc7ccd1-goog

Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
From 702cbce9997e4d70c3cb1670bf61e004c909eb23 Mon Sep 17 00:00:00 2001
From: Thomas Stuart <[email protected]>
Date: Mon, 21 Nov 2022 17:38:21 -0800
Subject: [PATCH] enforce stricter rules when registering phoneAccounts

- include disable accounts when looking up accounts for a package to
check if the limit is reached (10)
- put a new limit of 10 supported schemes
- put a new limit of 256 characters per scheme
- put a new limit of 256 characters per address
- ensure the Icon can write to memory w/o throwing an exception

bug: 259064622
bug: 256819769
Test: cts + unit
Change-Id: Ia7d8d00d9de0fb6694ded6a80c40bd55d7fdf7a7
Merged-In: Ia7d8d00d9de0fb6694ded6a80c40bd55d7fdf7a7
(cherry picked from commit on googleplex-android-review.googlesource.com host: 6a02885f90fa64d88bac31efbcdbc2bfe0a9328f)
Merged-In: Ia7d8d00d9de0fb6694ded6a80c40bd55d7fdf7a7
---
.../java/android/telecom/PhoneAccount.java | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/telecomm/java/android/telecom/PhoneAccount.java b/telecomm/java/android/telecom/PhoneAccount.java
index e332d3ff2b4d..808d032f5d66 100644
--- a/telecomm/java/android/telecom/PhoneAccount.java
+++ b/telecomm/java/android/telecom/PhoneAccount.java
@@ -517,6 +517,11 @@ public final class PhoneAccount implements Parcelable {

/**
* Sets the address. See {@link PhoneAccount#getAddress}.
+ * <p>
+ * Note: The entire URI value is limited to 256 characters. This check is
+ * enforced when registering the PhoneAccount via
+ * {@link TelecomManager#registerPhoneAccount(PhoneAccount)} and will cause an
+ * {@link IllegalArgumentException} to be thrown if URI is over 256.
*
* @param value The address of the phone account.
* @return The builder.
@@ -550,6 +555,10 @@ public final class PhoneAccount implements Parcelable {

/**
* Sets the icon. See {@link PhoneAccount#getIcon}.
+ * <p>
+ * Note: An {@link IllegalArgumentException} if the Icon cannot be written to memory.
+ * This check is enforced when registering the PhoneAccount via
+ * {@link TelecomManager#registerPhoneAccount(PhoneAccount)}
*
* @param icon The icon to set.
*/
@@ -583,6 +592,10 @@ public final class PhoneAccount implements Parcelable {
/**
* Specifies an additional URI scheme supported by the {@link PhoneAccount}.
*
+ * <p>
+ * Each URI scheme is limited to 256 characters. Adding a scheme over 256 characters will
+ * cause an {@link IllegalArgumentException} to be thrown when the account is registered.
+ *
* @param uriScheme The URI scheme.
* @return The builder.
*/
@@ -596,6 +609,12 @@ public final class PhoneAccount implements Parcelable {
/**
* Specifies the URI schemes supported by the {@link PhoneAccount}.
*
+ * <p>
+ * A max of 10 URI schemes can be added per account. Additionally, each URI scheme is
+ * limited to 256 characters. Adding more than 10 URI schemes or 256 characters on any
+ * scheme will cause an {@link IllegalArgumentException} to be thrown when the account
+ * is registered.
+ *
* @param uriSchemes The URI schemes.
* @return The builder.
*/
--
2.42.0.515.g380fc7ccd1-goog

Loading

0 comments on commit ec718b8

Please sign in to comment.