Closed
Bug 1102022
Opened 10 years ago
Closed 9 years ago
Possible buffer overflow in nsSOCKSSocketInfo::WriteV4ConnectRequest()
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: glandium, Assigned: glandium)
Details
(Keywords: sec-moderate, Whiteboard: currently not exploitable in practice given jemalloc)
Attachments
(1 file)
14.59 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
Thanks Mike - do you want to provide the patch or should someone for necko paste it up?
Comment 2•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: currently not exploitable in practice given jemalloc
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc03c937fae
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9bc03c937fae
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•