31674 - Fixes for ssl_EmulateAcceptRead

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

The source tarball is nss20000210.tar.gz, downloaded
from http://www.mozilla.org/projects/security/pki/src/.

The source file in security/lib/ssl/emulate.c.  The
function is ssl_EmulateAcceptRead.

There are three problems with that function:
1. The comments about the size of the 'buf' argument
   are wrong.
2. The assumption that 4-byte alignment of PRNetAddr
   should work is incorrect in NSPR 4.0.  In NSPR 4.0,
   PRNetAddr has a new 'ipv6' union member, which
   contains PRUint64 elements and hence may be 8-byte
   aligned on some platforms.
3. The PR_NETADDR_SIZE macro is no longer exported in
   NSPR 4.0.  In ssl_EmulateAcceptRead, it should be
   replaced by sizeof(PRNetAddr).

A patch for security/lib/ssl/emulate.c will be attached.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch for security/lib/ssl/emulate.c.

Comment 2

[Wan-Teh Chang]

Regarding the PRNetAddr alignment code you can
see the corresponding code in NSPR:
1. mozilla/nsprpub/pr/src/io/prsocket.c, _PR_EmulateAcceptRead().
2. mozilla/nsprpub/pr/src/pthreads/ptio.c, pt_AcceptRead().

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 

I agree that the code in NSS is wrong, but I don't 
agree that the suggested fix is correct.

The code in PR_EmulateAcceptRead in pr/src/io/prsocket.c
assumes that the alignment boundary for a PRNetAddr is
equal to the length of a pointer to void (a void*). 
I believe this is not correct for all platforms, esp. IRIX,
where pointers can be 32 or 64 bits (depending on ABI)
but there are no 64-bit values in a in6_addr with either ABI. 

So, I think we need a better rule to resolve this.

I suggest that we simply declare that the PRNetAddr
pointer returned by PR_EmulateAcceptRead will be
a multiple of 8.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

What we need is an alignmentof() operator.
Unforunately, on some platforms 
sizeof(void*) != alignmentof(void*)

In IRIX's N32 ABI, for example, long longs need to 
be 8-byte aligned, but sizeof(void*) == 4.

Comment 5

[Wan-Teh Chang]

How about if we just align PRNetAddr on 8-byte
boundary?  It is PRUint64, not void*, that
PRNetAddr contains.  So trying to align PRNetAddr
on the size of void* is wrong.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

How about this compromise?

#if defined(_PR_INET6)
#define AMASK 7 /* mask for alignment of PRNetAddr */
#else
#define AMASK 3 /* mask for alignment of PRNetAddr */
#endif
    if (rv >= 0) {
        ptrdiff_t pNetAddr = (((ptrdiff_t)buf) + amount + AMASK) & ~AMASK;

        *nd = newsockfd;
        *raddr = (PRNetAddr *)pNetAddr;
        memcpy((void *)pNetAddr, &remote, sizeof(PRNetAddr));
        return rv;
    }

Comment 7

[Wan-Teh Chang]

_PR_INET6 is not an exported macro.  You can't depend
on it.  I guess you can test for PR_AF_INET6, but I
would just use an AMASK of 7, to make thing simple.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

_PR_INET6 is not an exported macro? 
I can't depend on it?

prio.h depends on _PR_INET6.  Therefore, any source 
that includes prio.h depends on _PR_INET6.  
Is that not so?

Comment 9

[Wan-Teh Chang]

In NSPR 3.x, _PR_INET6 is a macro that is exposed
but strictly for internal use by public NSPR headers.
The leading underscore suggests that this macro
is for internal use only.

In NSPR 4.0, _PR_INET6 is not exposed at all.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Long ago I checked in the change suggested by wtc, to always use
8-byte alignment for PRNetAddr in ssl_EmulateAcceptRead().
However, I am concerned that it may no longer be sufficient,
on platforms that do not support IPV6, for the application to 
allocate the buffer of size 
        amount + (2 * sizeof(PRNetAddr))
where amount is the amount of data to be read.