1411744 - TBE-01-019: Integer Overflow in Attachment Code

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Ben Bucksch (:BenB)]

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.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1411744-overflow.patch (v1)

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.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1411744-overflow.patch (v1b)

Comment 3

[:aceman]

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?

Comment 4

[Jorg K (CEST = GMT+2)]

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 5

[:aceman]

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.

Comment 6

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c468a23c5c045056e9db8d18182d2d342e324b68

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Jorg K (CEST = GMT+2)]

Beta (TB 57):
https://hg.mozilla.org/releases/comm-beta/rev/b5a0ec39fbcb2a015dfa24e4551c117de5cac723

Comment 9

[Jorg K (CEST = GMT+2)]

TB 52.5 ESR (should be tracking 57+):
https://hg.mozilla.org/releases/comm-esr52/rev/90f4fd509699

Comment 10

[Jorg K (CEST = GMT+2)]

Made it compile:
https://hg.mozilla.org/releases/comm-esr52/rev/d4e6767007b3b0c92c57ea57f3532ad55f211b9f