As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 410587 - SSL_GetChannelInfo returns SECSuccess on invalid arguments
: SSL_GetChannelInfo returns SECSuccess on invalid arguments
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4
: All All
: -- minor (vote)
: 3.12
Assigned To: Wan-Teh Chang
Depends on:
  Show dependency treegraph
Reported: 2008-01-02 18:25 PST by Wan-Teh Chang
Modified: 2008-01-03 11:13 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Proposed patch (863 bytes, patch)
2008-01-02 18:25 PST, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review

Description User image Wan-Teh Chang 2008-01-02 18:25:44 PST
Created attachment 295177 [details] [diff] [review]
Proposed patch

The SSL_GetChannelInfo function (added in NSS 3.4) returns SECSuccess
if the input argument 'info' or 'len' is invalid.

It should set the error code and return SECFailure.

I rated the severity of this bug at minor because if 'info' is NULL,
we crash, and 'len < sizeof inf.length' can only be true when we
add new fields to SSLChannelInfo, and we can fix this bug before
we add new fields.
Comment 1 User image Nelson Bolyard (seldom reads bugmail) 2008-01-03 10:44:32 PST
Comment on attachment 295177 [details] [diff] [review]
Proposed patch

I'm surprised at this bug, because I wrote this code and I am usually 
meticulous about returning proper error codes. 

Regarding the comment:

> 'len < sizeof inf.length' can only be true when we add new fields to 
> SSLChannelInfo, and we can fix this bug before we add new fields.

That's not correct.  Adding new fields to the struct will not affect
the truth of that test.  Notice that that test does *not* ask 
"is len big enough to hold the entire "inf" structure?".  It only asks 
"is len big enough to hold the length field at the beginning of the inf 

This API outputs as much of the inf structure as the caller requests. 
That way, when we grow the structure, we don't create problems for 
callers that still request an older smaller subset.  

The output includes the length of the total inf structure, so that the
caller can call it twice, once to get the desired output length, and 
then again to get the entire structure, after allocating a buffer 
large enough to hold it.  

So, no change is required as we add new fields at the end.  But we 
can fix this failure to report error.
Comment 2 User image Wan-Teh Chang 2008-01-03 11:13:39 PST
You're right.  Would be nice to document how the function should
be used.  The caller can't call the function to get the desired
length as you described because we store

    inf.length = PR_MIN(sizeof inf, len);

as opposed to

    inf.length = sizeof inf;

Also, it's hard for the caller to handle the case where
len > sizeof inf, so we may want to treat that as an invalid
input argument.  (The caller would need to check the offset
of a field is < info->length before using it.)

I checked in the patch on the NSS trunk for NSS 3.12.
Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.17; previous revision: 1.16

Note You need to log in before you can comment on or make changes to this bug.