1735152 - nsSocketTransport::Open{Input/Output}Stream can set output pointer without AddRef-ing it.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

In SocketTransport2 we have a somewhat bad pattern:

NS_IMETHODIMP
nsSocketTransport::OpenInputStream(uint32_t flags, uint32_t segsize,
                                   uint32_t segcount, nsIInputStream** result) {
  if (!(flags & OPEN_UNBUFFERED) || (flags & OPEN_BLOCKING)) {
   // [...]
    nsCOMPtr<nsIAsyncOutputStream> pipeOut;
    rv = NS_NewPipe2(getter_AddRefs(pipeIn), getter_AddRefs(pipeOut),
                     !openBlocking, true, segsize, segcount);
    if (NS_FAILED(rv)) return rv;

    // async copy from socket to pipe
    rv = NS_AsyncCopy(&mInput, pipeOut, mSocketTransportService,
                      NS_ASYNCCOPY_VIA_WRITESEGMENTS, segsize);
    if (NS_FAILED(rv)) return rv;

    *result = pipeIn;
  } else {
    *result = &mInput;
  }

  // flag input stream as open
  mInputClosed = false;

  rv = PostEvent(MSG_ENSURE_CONNECT);
  if (NS_FAILED(rv)) return rv; 
  // BUG IS HERE!!!
  NS_ADDREF(*result);
  return NS_OK;
}

The problem is that if PostEvent fails to dispatch the event for some reason, we exit the method and result is assigned but not addreffed.
If the callsite passes in a member variable like here, that member nsCOMPtr gets assigned a value, but not addreffed.
When the object gets released, we also release the COMPtr, but since the refcount was not incremented this will lead to a double-free, either immediately or sometimes later.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1735152 - Avoid using NS_ADDREF in nsSocketTransport r=#necko

Comment 2

[Valentin Gosu [:valentin] (he/him)]

It's possible this is what's causing bug 1687269.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Note: postEvent is unlikely to fail in regular usage. It would only be a problem at shutdown and in low-memory situations.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9245258 [details]
Bug 1735152 - Avoid using NS_ADDREF in nsSocketTransport r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unclear. This requires posting an event to the thread to fail, which is possible during shutdown but unlikely otherwise (might be triggered by low memory though).
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 176919
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: patch should apply to all branches
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions.

Comment 5

[Daniel Veditz [:dveditz]]

If not all supported branches, which bug introduced the flaw?: Bug 176919

That's a long time ago. Any idea what changed in Fx90 that might have made this crash more likely? (see bug 1687269, bug 1731943) If nothing immediately comes to mind don't worry about it, could be a lot of things.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #5)

If not all supported branches, which bug introduced the flaw?: Bug 176919

That's a long time ago. Any idea what changed in Fx90 that might have made this crash more likely? (see bug 1687269, bug 1731943) If nothing immediately comes to mind don't worry about it, could be a lot of things.

No idea. It could be related to the conditions in which Dispatch to the thread fails, but I'm not able to find a change that I can point to.

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9245258 [details]
Bug 1735152 - Avoid using NS_ADDREF in nsSocketTransport r=#necko

Approved to land/request uplift

Comment 8

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

Avoid using NS_ADDREF in nsSocketTransport r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/d76783498a30c23ffe3e201d42dc32aef8479738
https://hg.mozilla.org/mozilla-central/rev/d76783498a30

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9245258 [details]
Bug 1735152 - Avoid using NS_ADDREF in nsSocketTransport r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Potential UAF
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): No functionality change other than making sure return arg is always AddRef'd
• String changes made/needed:

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 UAF
• Fix Landed on Version: 95
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): No functionality change other than making sure return arg is always AddRef'd
• String or UUID changes made by this patch:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9245258 [details]
Bug 1735152 - Avoid using NS_ADDREF in nsSocketTransport r=#necko

Approved for 94.0b7 and 91.3esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b6fc6de1d80

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/9e341f07e58d

Comment 13

[Release mgmt bot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.