Closed Bug 949060 Opened 6 years ago Closed 6 years ago
_Write V buffer overflow
For Posix, "struct iovec" includes a size_t length. For NSPR, PRIOVec includes an int length. When a caller of PR_Writev converts a Posix "struct iovec" to PRIOVec without checking sizes against MAX_INT32, the result for write operations above 2GB will be a negative iov_len. I'd also say that NSPR doesn't exactly document what negative iov_len means even though the NSPR API permits it to be used. When ssl_WriteV (sslsock.c) gets a negative iov_len, it proceeds to write data from the socket onto the "buf" stack variable and overflows the bounds of that variable. It's possible this could be leveraged into a security attack by various software so I'm marking this a security bug. ssl_WriteV should be changed to eliminate this buffer overflow error.
Making bug subject more explicit.
Summary: ssl_WriteV mishandles negative iov_len → ssl_WriteV buffer overflow
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.15.5
Do we have any existing code (mis)using this API in a dangerous way?
I'm not allowed to provide you with the answer to that question.
Actually, there is an answer I can provide: PR_Writev() does call ssl_WriteV() in a dangerous way (without validating arguments that can result in a buffer overflow). So that makes the answer to your question yes.
Chris: does this patch fix the bug for you? Would you prefer that I add a for loop at the beginning of ssl_WriteV to check the iov_len field of all the PRIOVec structures? In this patch I check the iov_len field as I send each PRIOVec structure. NSPR's pt_Writev and SocketWritev functions need the same change: http://mxr.mozilla.org/nspr/ident?i=pt_Writev http://mxr.mozilla.org/nspr/ident?i=SocketWritev Note: this patch is sufficient only on 32-bit platforms. On 64-bit platforms, a size_t of 0x100000010 will be truncated to 0x10, which is a positive value and therefore won't be detected by this patch. So an application still needs to take care when converting a struct iovec to a PRIOVec.
Comment on attachment 8358143 [details] [diff] [review] Patch Review of attachment 8358143 [details] [diff] [review]: ----------------------------------------------------------------- Hmm... another idea is to check iov_len in PR_Writev: http://mxr.mozilla.org/nspr/ident?i=PR_Writev Then it will apply to all implementations of the writev method. The downside is that it won't cover direct calls to fd->methods->writev.
Yes, attachment #8358143 [details] [diff] [review] resolves the issue and I prefer that to a fix only in NSPR. Also fixing the problem in NSPR would be nice to have as a matter of defense in depth.
Comment on attachment 8358143 [details] [diff] [review] Patch Review of attachment 8358143 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
The first patch is incomplete. It assumes we only access iov_len via myIov.iov_len. But I found that we also do iov->iov_len and iov.iov_len. So I now use a for loop to inspect the iov_len of every element in the 'iov' array.
Comment on attachment 8360822 [details] [diff] [review] Patch v2 This looks right to me. I like that it returns PR_INVALID_ARGUMENT_ERROR so callers are informed when they provide bogus data.
Attachment #8360822 - Flags: review?(chris.newman) → review+
Comment on attachment 8360822 [details] [diff] [review] Patch v2 Patch checked in: https://hg.mozilla.org/projects/nss/rev/345f4088ef29
Attachment #8360822 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [does not affect Gecko, may affect other NSS-based products]
Should this bug be opened?
What is an appropriate description for this bug in the NSS 3.15.5 release notes? "fixed a potential buffer overflow in the sslWriteV function" ?
Kai: I updated your description of this bug in the NSS 3.15.5 release notes. I also mentioned the caveat at the end of comment 5.
Should there be a CVE for this?
You need to log in before you can comment on or make changes to this bug.