Closed Bug 1411744 Opened 3 years ago Closed 3 years ago

TBE-01-019: Integer Overflow in Attachment Code

Categories

(MailNews Core :: Attachments, defect)

defect
Not set
critical

Tracking

(thunderbird_esr5257+ fixed, thunderbird57 fixed, thunderbird58 fixed)

RESOLVED FIXED
Thunderbird 58.0
Tracking Status
thunderbird_esr52 57+ fixed
thunderbird57 --- fixed
thunderbird58 --- fixed

People

(Reporter: BenB, Assigned: jorgk-bmo)

Details

(Keywords: crash, csectype-intoverflow, sec-moderate, Whiteboard: TB 57 beta => TB 52.5 ESR)

Attachments

(1 file, 1 obsolete file)

Outside of the forward in-line mode the body parsing code can trigger a similar integer overflow similar to TBE-01-018. This time, however, the overflow happens when an attachment’s file size is used inside a PR_MALLOC call. The code in question can be found below.

Affected File:
/mailnews/mime/src/mimedrft.cpp

Affected Code:
uint32_t bodyLen = 0;
[...]
int64_t fileSize;
nsCOMPtr<nsIFile> tempFileCopy;
mdd->messageBody->m_tmpFile->Clone(getter_AddRefs(tempFileCopy));
mdd->messageBody->m_tmpFile = do_QueryInterface(tempFileCopy);
tempFileCopy = nullptr;
mdd->messageBody->m_tmpFile->GetFileSize(&fileSize);
bodyLen = fileSize;
body = (char *)PR_MALLOC (bodyLen + 1);
if (body)
{
  memset (body, 0, bodyLen+1);
  uint32_t bytesRead;
  nsCOMPtr <nsIInputStream> inputStream;
  nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), mdd->messageBody->m_tmpFile);
  if (NS_FAILED(rv))
    return;
  inputStream->Read(body, bodyLen, &bytesRead);

At first glance it has to be noticed that the previously queried file size is stored in a 64-bit integer called fileSize. This value, however, gets truncated to a 32-bit integer which can assume the value of 0xffffffff. Consequently, the next call to PR_MALLOC with bodyLen + 1 will wrap around 0, thus leading to a zero-sized allocation. Afterwards, the call to inputStream->Read will read up to 0xffffffff bytes of data into the zero-sized heap area, again leading to a heap overflow. It is recommended to fix the integer truncation and make sure that passing a value of 0 to PR_MALLOC becomes unattainable.

For the original report as PDF, see bug 1411701.
Attached patch 1411744-overflow.patch (v1) (obsolete) — Splinter Review
Not a very likely bug since only a file with a size of exactly 0xFFFFFFFF bytes would cause the bug of calling malloc(0). malloc(0) might return NULL, so the |if (body)| block would be executed.

Since the stream interface can't handle more than uint32_t, we might as go through the same code path as if malloc() had failed and skip body processing.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8922321 - Flags: review?(acelists)
Attachment #8922321 - Attachment is obsolete: true
Attachment #8922321 - Flags: review?(acelists)
Attachment #8922364 - Flags: review?(acelists)
Comment on attachment 8922364 [details] [diff] [review]
1411744-overflow.patch (v1b)

Review of attachment 8922364 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, for now we only support messages and other object sizes up to 4GB (UINT32_MAX) throughout the code.
On the other hand, the stream reads bodyLen, not bodyLen+1. So why limit bodyLen to UINT32_MAX-1 ?

Would it work as (char *)PR_MALLOC(uint64_t(bodyLen) + 1) ? Is the (char*) needed?
I limit to UINT32_MAX-1 so I can allocate UINT32_MAX. The stream won't read more than UINT32_MAX, so what's the point arguing over one byte that adds more complication?

Looks like the PR_MALLOC is ultimately defined to be malloc, so the cast is necessary, since malloc returns a void* and 'body' is a char*.

malloc typically takes a size_t argument which is a 32bit uint, and malloc can only allocate 4GB on 32bit systems which we still support (in fact, officially exclusively on Windows). Also see:
https://stackoverflow.com/questions/37988273/allocate-more-than-4gb-memory-in-c

So your confusing trickery to squeeze one byte out of the machine for the totally unlikely case of such a big message ever occurring is not worth considering, IMHO.
Comment on attachment 8922364 [details] [diff] [review]
1411744-overflow.patch (v1b)

Review of attachment 8922364 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mime/src/mimedrft.cpp
@@ +1493,5 @@
>          tempFileCopy = nullptr;
>          mdd->messageBody->m_tmpFile->GetFileSize(&fileSize);
> +        uint32_t bodyLen = 0;
> +
> +        // The stream interface can only read up to 4GB (32bit uint).

OK, just please expand this comment to say we will just fail/skip bigger sized bodies. Otherwise there could be expectation we just read in chunks with smaller malloced buffer.
Attachment #8922364 - Flags: review?(acelists) → review+
https://hg.mozilla.org/comm-central/rev/c468a23c5c045056e9db8d18182d2d342e324b68
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8922364 [details] [diff] [review]
1411744-overflow.patch (v1b)

Highly unlikely to ever cause a problem, but I can uplift it if desired.
Attachment #8922364 - Flags: approval-comm-esr52?
Attachment #8922364 - Flags: approval-comm-beta+
Group: mail-core-security → core-security-release
Summary: Integer Overflow in Attachment Code → TBE-01-019: Integer Overflow in Attachment Code
Target Milestone: --- → Thunderbird 58.0
Whiteboard: TB 57 beta => TB 52.5 ESR
Attachment #8922364 - Flags: approval-comm-esr52? → approval-comm-esr52+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.