1843752 - (CVE-2024-1546) Crash when reading a malformed NNTP article with an overlong body line

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

Try to read Message-ID: <nnd$54637960$0da2d9c3@3f2ba0a65bb508bb>
| Subject: This is a test [1/1] "test.jpg" yEnc (2/2)
| Newsgroups: misc.test
| Date: Mon, 10 Jul 2023 06:44:56 +0200

Result with TB117.0a1:
https://crash-stats.mozilla.org/report/index/b5064a44-4821-46e4-bc3c-320ab0230716

TB102.13.0 crashes as well.

Comment 1

[Wayne Mery (:wsmwk)]

I was not able to reproduce this with daily on Mac.

Comment 2

[Wayne Mery (:wsmwk)]

TCW can you reproduce?

Comment 3

[Arthur K. (he/him)]

(In reply to Wayne Mery (:wsmwk) from comment #2)

TCW can you reproduce?

I don't have NNTP anymore to test with. Sorry.

Comment 4

[Wayne Mery (:wsmwk)]

max, can you reproduce?

I don't have NNTP anymore to test with. Sorry.

I messed up. Records now corrected.

Comment 6

[max]

(In reply to Alfred Peters from comment #0)

Try to read Message-ID: <nnd$54637960$0da2d9c3@3f2ba0a65bb508bb>
| Subject: This is a test [1/1] "test.jpg" yEnc (2/2)
| Newsgroups: misc.test
| Date: Mon, 10 Jul 2023 06:44:56 +0200

At which NNTP server "misc.test" is hosted?

I have unlikely faced messages with long lines in the groups I am reading at the gmane mail to news gateway.

I am not running my own NNTP server for testing purposes.

Comment 7

[Wayne Mery (:wsmwk)]

I am using news.blueworldhosting.com. But I should think most news servers will carry misc.test

Comment 8

[max]

(In reply to Wayne Mery (:wsmwk) from comment #7)

I am using news.blueworldhosting.com. But I should think most news servers will carry misc.test

Thanks. I thought the Thunderbird team hosts a NNTP server to ensure reproducible results, to avoid attempts to sanitize data during transfer that might be performed on some servers.

On Linux I do not see any issue with the specified message. Thunderbird-102.13

Comment 9

[Hartmut Welpmann [:welpy-cw]]

I can't reproduce this either. Viewing that message works for me on latest Daily (Debian Linux) as well as on 116.0b7 (Windows 10).

Comment 10

[Alfred Peters [:infofrommozilla]]

Attachment: Data structure nsIStringInputStream on crash

I can confirm that the crash does not occur when accessing a server over the WAN.
However, it is reproducible with my local NNTP server (on the same computer; Windows 10). It seems to deliver the data faster than TB can process it.

I was finally able to take a look at it in the debugger. It tells me that it is an access violation during reading.

The exception occurs here with memcpy():
https://searchfox.org/comm-central/rev/451a7f57d5f3c6e62df232542555c595fa34e599/mozilla/xpcom/io/nsStreamUtils.cpp#781

nsresult NS_CopySegmentToBuffer(nsIInputStream* aInStr, void* aClosure,
                                const char* aBuffer, uint32_t aOffset,
                                uint32_t aCount, uint32_t* aCountWritten) {
  char* toBuf = static_cast<char*>(aClosure);
  memcpy(&toBuf[aOffset], aBuffer, aCount);
  *aCountWritten = aCount;
  return NS_OK;
}

aCount is 264461, which is the length of the line in the body (inclusiv CRLF).

Source is the mBuffer of the nsIStringInputStream. As you can see in the picture, sise_ is specified as 264461. However, the memory area under data_ is only 64k bytes in size! It is therefore quite obvious that memcpy() fails.

Comment 11

[Alfred Peters [:infofrommozilla]]

This is where the 64k for the buffer size comes from:
https://searchfox.org/comm-central/rev/451a7f57d5f3c6e62df232542555c595fa34e599/mozilla/ipc/glue/DataPipe.h#158

If I increase this, there is no longer an exception. But of course this is not the solution to the problem.

The 64k is the size of the segments from which the NS_Copy Segment ToBuffer() function takes its name.

But it looks like the segmentation algorithm could still be improved.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This looks security-sensitive, and I'm not sure but it looks like the impact wouldn't be limited to Thunderbird. We seem to be creating a Span with the wrong length but I can't easily see where; from looking around in searchfox (and based on the nsBorrowedSource from comment #10) it seems to come from a call to NS_NewByteInputStream with NS_ASSIGNMENT_DEPEND, but there are a lot of calls to that. Nika, do you have an idea of what could be going on here?

Comment 13

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

The searchfox link is broken in comment #11, here's a m-c link https://searchfox.org/mozilla-central/rev/52f45d12530f9f8da61c6c7237003b5d1499b6d1/ipc/glue/DataPipe.h#158

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧ from comment #12)

This looks security-sensitive, and I'm not sure but it looks like the impact wouldn't be limited to Thunderbird. We seem to be creating a Span with the wrong length but I can't easily see where; from looking around in searchfox (and based on the nsBorrowedSource from comment #10) it seems to come from a call to NS_NewByteInputStream with NS_ASSIGNMENT_DEPEND, but there are a lot of calls to that. Nika, do you have an idea of what could be going on here?

A nsBorrowedSource within a string input stream, like its name and the flag used to construct it (NS_ASSIGNMENT_DEPEND) suggests, does not own the source of its data. It holds a raw pointer to a buffer, which the caller needs to make sure is held alive for the full life of the string stream.

Looking at the stack in comment 0, it looks like the issue is happening after DocumentLoadListener has delayed a call to OnDataAvailable. This happens as a compensation strategy for necko channels which cannot be suspend()-ed. When we do this, we read the data into a nsCString when storing (https://searchfox.org/mozilla-central/rev/52f45d12530f9f8da61c6c7237003b5d1499b6d1/netwerk/ipc/DocumentLoadListener.cpp#2692-2699), then convert it back into a nsStringInputStream when calling OnDataAvailable (https://searchfox.org/mozilla-central/rev/52f45d12530f9f8da61c6c7237003b5d1499b6d1/netwerk/ipc/NeckoCommon.cpp#34-38).

Scanning the code, the biggest risk I see is around how we create that second string stream from a Span<const char>, trusting that aParams.count is the same length as the stringStream. This isn't guaranteed to be true by the checks, as NS_ReadInputStreamToString returns success if the provided input stream doesn't provide aCount bytes (the resulting string is truncated). I expect this is the cause of the crash in comment 0. It should be quite easy to fix, as we already have a nsCString available in that function which has the true data's length.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1843752 - Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=#necko-reviewers!

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1754004

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Can we assign a severity to this?

Comment 18

[Nika Layzell [:nika] (ni? for response)]

My first intuition suggests this is probably sec-high, as it is effectively an out-of-bounds memory read in an edge case. This may not impact Firefox though, as the OOB read cannot happen unless a necko channel implementation is misbehaving, and calls OnDataAvailable with an aCount which is larger than the actual amount of data available in the stream.

Based on comment 10 and comment 11, and the bug being triggered due to NNTP streams, it seems that there is a bug in Thunderbird's NntpChannel implementation which can cause this situation. It looks like we are receiving a large (>64k) block of data from the network, and trying to write it into a nsPipe (https://searchfox.org/comm-central/rev/f8240a0a0668a3c1092dbbdd4dfe936378d0d9a6/mailnews/news/src/NntpChannel.jsm#346). Because this pipe was initialized with pipe.init(true, true, 0, 0) (https://searchfox.org/comm-central/rev/f8240a0a0668a3c1092dbbdd4dfe936378d0d9a6/mailnews/news/src/NntpChannel.jsm#316), it is a bounded pipe with the default segment size and maximum segment count, which I believe is a 64k buffer. Because of that, the write call will only write 64k bytes and return, and the onDataAvailable callback will be invoked with an aCount larger than the buffer being passed in, triggering this issue.

My patch on this bug only fixes the underlying out-of-bounds read and does not fix this issue with NntpChannel, which will still be broken after the changes (though it'll drop data rather than causing an out-of-bounds read). There are various ways that NntpChannel could be changed to avoid the bug, including creating a string input stream each time onDataAvailable is called, checking the return value from outputStream.write and looping back to write the rest if it doesn't fit in one go, or making the pipe unbounded.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9372067 [details]
Bug 1843752 - Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=#necko-reviewers!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Constructing an exploit would require identifying specific channel types which have a bug causing them to call onDataAvailable with the wrong aCount parameter. I imagine this would be quite time consuming and/or difficult to do.

The patch also makes a small ownership change which slightly obfuscates the actual functional change.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 1556489
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The most recent changes to this code were when adding early hints in bug 1771867, which I believe is landed in all supported branches, so I expect that the patches will just apply to all supported branches.
• How likely is this patch to cause regressions; how much testing does it need?: Simple refactoring with low risk.
• Is Android affected?: Yes

Comment 20

[Wayne Mery (:wsmwk)]

If bp-dc54926d-cec6-4a73-a969-eed770240109 is another example, then we have another signature memcpy | NS_CopySegmentToBuffer More

(In reply to Alfred Peters [:infofrommozilla] from comment #0)

Try to read Message-ID: <nnd$54637960$0da2d9c3@3f2ba0a65bb508bb>
| Subject: This is a test [1/1] "test.jpg" yEnc (2/2)
| Newsgroups: misc.test
| Date: Mon, 10 Jul 2023 06:44:56 +0200

Result with TB117.0a1:
https://crash-stats.mozilla.org/report/index/b5064a44-4821-46e4-bc3c-320ab0230716

TB102.13.0 crashes as well.

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  VCRUNTIME140.dll  memcpy_repmovs  d:\a01\_work\12\s\src\vctools\crt\vcruntime\src\string\amd64\memcpy.asm:40
1  xul.dll  NS_CopySegmentToBuffer  xpcom/io/nsStreamUtils.cpp:781
1  xul.dll  nsStringInputStream::ReadSegments  xpcom/io/nsStringStream.cpp:305
1  xul.dll  nsStringInputStream::Read  xpcom/io/nsStringStream.cpp:276
2  xul.dll  nsStreamConverter::OnDataAvailable  mailnews/mime/src/nsStreamConverter.cpp:757
3  xul.dll  mozilla::net::ForwardStreamListenerFunctions::<lambda_30>::operator const  netwerk/ipc/NeckoCommon.cpp:40
3  xul.dll  mozilla::detail::VariantImplementation<unsigned char, 1, mozilla::net::OnDataAvailableParams, mozilla::net::OnStopRequestParams, mozilla::net::OnAfterLastPartParams>::matchN  mfbt/Variant.h:309
3  xul.dll  mozilla::detail::VariantImplementation<unsigned char, 0, mozilla::net::OnStartRequestParams, mozilla::net::OnDataAvailableParams, mozilla::net::OnStopRequestParams, mozilla::net::OnAfterLastPartParams>::matchN  mfbt/Variant.h:318
3  xul.dll  mozilla::Variant<mozilla::net::OnStartRequestParams, mozilla::net::OnDataAvailableParams, mozilla::net::OnStopRequestParams, mozilla::net::OnAfterLastPartParams>::matchN  mfbt/Variant.h:902
3  xul.dll  mozilla::Variant<mozilla::net::OnStartRequestParams, mozilla::net::OnDataAvailableParams, mozilla::net::OnStopRequestParams, mozilla::net::OnAfterLastPartParams>::match  mfbt/Variant.h:857
3	xul.dll	mozilla::net::ForwardStreamListenerFunctions(nsTArray<mozilla::Variant<mozilla::net::OnStartRequestParams, mozilla::net::OnDataAvailableParams, mozilla::net::OnStopRequestParams, mozilla::net::OnAfterLastPartParams> >&, nsIStreamListener*)	netwerk/ipc/NeckoCommon.cpp:21
4	xul.dll	mozilla::net::DocumentLoadListener::ResumeSuspendedChannel(nsIStreamListener*)	netwerk/ipc/DocumentLoadListener.cpp:1441
5	xul.dll	mozilla::net::DocumentLoadListener::FinishReplacementChannelSetup(nsresult)	netwerk/ipc/DocumentLoadListener.cpp:1355
6	xul.dll	mozilla::net::DocumentLoadListener::RedirectToRealChannelFinished(nsresult)	netwerk/ipc/DocumentLoadListener.cpp:1296
7	xul.dll	mozilla::net::DocumentLoadListener::TriggerRedirectToRealChannel::<lambda_22>::operator()(nsresult const&)	netwerk/ipc/DocumentLoadListener.cpp:2255

Comment 21

[Donal Meehan [:dmeehan]]

Too late for 122.0, switching tracking to Fx123

Comment 22

[Alfred Peters [:infofrommozilla]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #18)

My patch on this bug only fixes the underlying out-of-bounds read

Confirmed. TB no longer crashes with the patch.

and does not fix this issue with NntpChannel, which will still be broken after the changes (though it'll drop data rather than causing an out-of-bounds read).

That's exactly how it is. TB only displays the first 64k characters of the line.

Comment 23

[max]

May shredded NNTP offline cache (Bug #1857450) be caused by this issue?

I am unsure if it is appropriate to add such comment there since that bug is a public one while this one is protected.

Comment 24

[Tom Ritter [:tjr]]

Comment on attachment 9372067 [details]
Bug 1843752 - Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=#necko-reviewers!

Approved to land and uplift

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1843752 - Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=#necko-reviewers!

Original Revision: https://phabricator.services.mozilla.com/D198189

Comment 26

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Risk associated with taking this patch: Low
• User impact if declined: Potential out-of-bounds read if a necko channel misbehaves
• Explanation of risk level: The code is fairly straightforward and shouldn't pose risks
• Steps to reproduce for manual QE testing: N/A
• Fix verified in Nightly: no
• Is Android affected?: yes
• Code covered by automated testing: no
• Needs manual QE test: no

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86103acfc050
Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=necko-reviewers,kershaw

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/86103acfc050

Comment 29

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/700859e125b5

Comment 30

[Dianna Smith [:diannaS]]

:nika when you get a chance can you submit an uplift request for esr? (it applies cleanly)

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9372067 [details]
Bug 1843752 - Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=#necko-reviewers!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: Potential out-of-bounds read if a necko channel misbehaves
• Fix Landed on Version: 124
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The code is fairly straightforward and shouldn't pose risks

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9372067 [details]
Bug 1843752 - Explicitly transfer ownership of queued-up OnDataAvailableParams data buffers, r=#necko-reviewers!

Approved for 115.8esr.

Comment 33

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/85a897036b0d

Comment 34

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 35

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons