Closed Bug 949060 Opened 6 years ago Closed 6 years ago

ssl_WriteV buffer overflow

Categories

(NSS :: Libraries, defect, P2, critical)

3.15.3.1
defect

Tracking

(firefox27 unaffected, firefox28 unaffected, firefox29 unaffected, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected)

RESOLVED FIXED
3.15.5
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected

People

(Reporter: chris.newman, Assigned: wtc)

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [does not affect Gecko, may affect other NSS-based products])

Attachments

(1 file, 1 obsolete file)

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
Group: crypto-core-security
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8358143 - Flags: review?(chris.newman)
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.
Attached patch Patch v2Splinter Review
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[1].iov_len.

So I now use a for loop to inspect the iov_len of every element
in the 'iov' array.
Attachment #8358143 - Attachment is obsolete: true
Attachment #8358143 - Flags: review?(chris.newman)
Attachment #8360822 - Flags: review?(chris.newman)
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]
Group: crypto-core-security
Target Milestone: 3.15.5 → 3.16
Target Milestone: 3.16 → 3.15.5
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?
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.