Closed Bug 1102022 Opened 5 years ago Closed 5 years ago

Possible buffer overflow in nsSOCKSSocketInfo::WriteV4ConnectRequest()

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

Details

(Keywords: sec-moderate, Whiteboard: currently not exploitable in practice given jemalloc)

Attachments

(1 file)

While discussing the safety of the assumptions made in nsSOCKSSocketInfo::WriteUint8() and similar functions on IRC, I was looking at nsSOCKSSocketInfo::WriteV4ConnectRequest(), and it looks like it's actually possible to overflow it:

Here's the relevant code path:

    mDataLength = 0;
    WriteUint8(0x04); // version -- 4
    WriteUint8(0x01); // command -- connect
    WriteNetPort(addr);
    if (proxy_resolve) {
       WriteUint32(htonl(0x00000001)); // Fake IP
       WriteUint8(0x00); // Send an emtpy username
       if (mDestinationHost.Length() > MAX_HOSTNAME_LEN) {
          ...
       }
       WriteString(mDestinationHost); // Hostname
       WriteUint8(0x00);
    }


WriteUint8 writes 1 byte.
WriteNetPort writes 2.
WriteUint32 writes 4
So by the time we're at WriteString, we've written 9.

From the test, mDestinationHost can have up to MAX_HOSTNAME_LEN bytes, which is 255.

255 + 9 is 264. Plus the last WriteUint8, that's 265.

The assumption is that mDataLength, which each of these Write* functions increments by the number of bytes it writes stays under BUFFER_SIZE, which is... 262.

Looking at nsSOCKSSocketInfo::WriteV5ConnectRequest(), the corresponding code path writes at most... 262 bytes, so I guess that's where the BUFFER_SIZE comes from (although both code paths landed at the same time).

Now, let's take a look at the risk:
- nsSOCKSSocketInfo::WriteV4ConnectRequest() is only called when a user has set a socks v4 proxy as proxy.
- proxy_resolve is only set if the network.proxy.socks_remote_dns pref is set, and it's not the default.
- mDestinationHost is, afaik, the hostname from an url, so that makes 2 overflown bytes controlled by an attacker.
- mData, that the Write* functions write to is a buffer created with new uint8_t[BUFFER_SIZE], which means it's heap allocated. In practice, with jemalloc, this means the allocated buffer is actually 272 bytes long because of the different size classes. We're using jemalloc everywhere, except, it has to be noted, on OSX when running as 32-bits, but I just checked and malloc returns a 272 bytes long buffer too.

So all in all, while there is a possible buffer overflow in that function it a) requires a very specific setup that very few users use and b) doesn't actually writes data over some other buffer.

For those reasons, I didn't make this bug security sensitive.

The fix would be to bump the BUFFER_SIZE to 265, but I feel like there should be some static assert.
Thanks Mike - do you want to provide the patch or should someone for necko paste it up?
I agree with your reasoning that this bug doesn't need to be security sensitive because the risk is small, but we should still flag this as a potential security risk because if not fixed one or more of those assumptions may no longer hold at some time in the future: 
 * It's not the default, but some users could pick it
 * we may change allocators in the future, or the compiler and/or allocator
   we use may behave differently in a future version or on a new platform.
Keywords: sec-moderate
Whiteboard: currently not exploitable in practice given jemalloc
(In reply to Patrick McManus [:mcmanus] from comment #1)
> Thanks Mike - do you want to provide the patch or should someone for necko
> paste it up?

I have an idea to static assert this thing, I'll do that.
This also adds static checks that buffer overflows do not sneak in again in
the future.

Interestingly, this also makes (at least9 GCC generate more efficient code.
For example, before, writing to the buffer in WriteV5AuthRequest would look
like this:
  mov    0x38(%rbx),%eax
  mov    0x28(%rbx),%rcx
  movb   $0x5,(%rcx,%rax,1)
  mov    0x38(%rbx),%eax
  inc    %eax
  mov    %eax,0x38(%rbx)
  mov    0x28(%rbx),%rcx
  movb   $0x1,(%rcx,%rax,1)
  mov    0x38(%rbx),%eax
  inc    %eax
  mov    %eax,0x38(%rbx)
  mov    0x28(%rbx),%rcx
  movb   $0x0,(%rcx,%rax,1)
  incl   0x38(%rbx)

Now it looks like this:
  mov    0x28(%rbx),%rax
  movb   $0x5,(%rax)
  movb   $0x1,0x1(%rax)
  movb   $0x0,0x2(%rax)
  movl   $0x3,0x38(%rbx)

There's probably some bikeshedding to do on names.

The same trick could be used for the Read* functions.
Attachment #8527040 - Flags: review?(mcmanus)
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Comment on attachment 8527040 [details] [diff] [review]
Increase the SOCKS I/O buffer size to avoid buffer overflows

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

sweet. thanks
Attachment #8527040 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/9bc03c937fae
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.