949060 - ssl_WriteV buffer overflow

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Chris Newman]

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.

Comment 1

[Chris Newman]

Making bug subject more explicit.

Comment 2

[Daniel Veditz [:dveditz]]

Do we have any existing code (mis)using this API in a dangerous way?

Comment 3

[Chris Newman]

I'm not allowed to provide you with the answer to that question.

Comment 4

[Chris Newman]

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.

Comment 5

[Wan-Teh Chang]

Attachment: Patch

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 6

[Wan-Teh Chang]

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.

Comment 7

[Chris Newman]

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 8

[Chris Newman]

Comment on attachment 8358143 [details] [diff] [review]
Patch

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

Looks good to me.

Comment 9

[Wan-Teh Chang]

Attachment: Patch v2

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.

Comment 10

[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.

Comment 11

[Wan-Teh Chang]

Comment on attachment 8360822 [details] [diff] [review]
Patch v2

Patch checked in: https://hg.mozilla.org/projects/nss/rev/345f4088ef29

Comment 12

[Kai Engert (:KaiE:)]

Should this bug be opened?

Comment 13

[Kai Engert (:KaiE:)]

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" ?

Comment 14

[Wan-Teh Chang]

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.

Comment 15

[Mike Hommey [:glandium]]

Should there be a CVE for this?