394202 - ssl_GetPrivate can corrupt non-SSL private structures

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

There are some functions that must be called with an SSL PRFD, not with
a PRFD of another layer in the same stack as an SSL PRFD.  Those functions
call ssl_GetPrivate to verify that they have been called with an SSL PRFD,
and to return the address of the private sslSocket structure for that PRFD.
It also stores the address of the PRFD in the sslSocket.

ssl_GetPrivate only checks that the PRFD is an SSL PRFD in DEBUG builds,
not in optimized builds.  So, in optimized builds, if called with the 
address of an non-SSL PRFD, it will corrupt that layer's private structure
by storing the PRFD address in it somewhere.  If the caller of the function
that calls ssl_GetPrivate has made an error, passing in the wrong PRFD,
ssl_GetPrivate will compound the error by corrupting the private area of
that PRFD.

Comment 1

[Julien Pierre]

Comment on attachment 278814 [details] [diff] [review]
patch v1

You might also want to check for a non-NULL fd and set an invalid argument error, since you have those other checks. That would match the assertion completely.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278814 [details] [diff] [review]
patch v1

Wan-Teh, please SR

Comment 3

[Wan-Teh Chang]

Comment on attachment 278814 [details] [diff] [review]
patch v1

r=wtc.  Do you want to check if fd == NULL first?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Questions for Wan-Teh and Julien:

All the callers of ssl_GetPrivate are in the same source file.  
All but one of them are methods in the PRIOMethods table for SSL.

Q1) Wan-Teh, can an NSPR IO method ever get called with a NULL first 
argument (the PRFileDesc *)?   Isn't it the case that if the PRFileDesc* 
was NULL, that NSPR could not have found the IOMethods table in which to 
find the address of the IOMethod it called?  Can we not infer from the 
successful call to the IOMethod that the PRFileDesc* was non-NULL and valid?

The one exception, the one non-IOMethod caller to ssl_GetPrivate is 
ssl_SetTimeout.  ssl_SetTimeout has two callers, both in sslsecur.c, 
neither of which is a PRIOMethod.  

It appears to me that the only reason that ssl_SetTimeout is located in 
sslsock.c rather than in sslsecur.c (with its callers) is that 
ssl_GetPrivate is a static function in sslsock.c.  

In another thread, for which there is no bug filed yet (AFAIK),
Julien and I now think that ssl_SetTimeout should call ssl_FindSocket
rather than ssl_GetPrivate.  (The block comment for ssl_GetPrivate seems
to confirm this.)  

So, I propose to change ssl_SetTimeout to call ssl_FindSocket instead of 
ssl_GetPrivate, and move ssl_SetTimeout from sslsock.c to sslsecur.c,
where it can be static.  This can be done because ssl_FindSocket is not
static, unlike ssl_GetPrivate.

Q2) Julien and Wan-Teh, what do you think of that plan?
(I will shortly file the other bug about ssl_SetTimeout.)

Comment 5

[Wan-Teh Chang]

Once you fix bug 394271, the block comment for ssl_GetPrivate
will be true again, so you don't need to fix this bug.

Regarding Q1: I looked at the assertions in ssl_GetPrivate
and mechanically expected a matching if statement.  I didn't
know how this function should be used.  So please ignore my
question about checking for fd == NULL.

Comment 6

[Julien Pierre]

Wan-Teh,

Re: comment 5, it still wouldn't hurt to check in this patch in case some other code accidentally calls ssl_GetPrivate again from non-IOLayer functions.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I intend to check this in.  It's just not my first priority.  

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

On trunk
Checking in sslsock.c; new revision: 1.53; previous revision: 1.52

Comment 9

[Julien Pierre]

Don't forget the branch checkin also.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Wanted to see it on trunk first.

ssl/sslsock.c; new revision: 1.44.2.8; previous revision: 1.44.2.7