Bug 654657

Summary: Incorrect socket accept error message due to bad pointer arithmetic
Product: [Retired] Dogtag Certificate System Reporter: John Dennis <jdennis>
Component: JSSAssignee: RHCS Maintainers <rhcs-maint>
Status: CLOSED EOL QA Contact: Ben Levenson <benl>
Severity: medium Docs Contact:
Priority: high    
Version: unspecifiedCC: cfu, dpal, jgalipea, mharmsen, rrelyea
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 660436 (view as bug list) Environment:
Last Closed: 2020-03-27 20:01:17 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 660436    
Attachments:
Description Flags
Minimal patch to correct bad string pointer
cfu: review+
Clean up most compiler warnings none

Description John Dennis 2010-11-18 14:42:30 UTC 
JSS produces incorrect error messages for socket accept due to bad pointer arithmetic.

There are 3 places with code like this:

JSSL_throwSSLSocketException(env, "Accept operation timed out with error code " + err);

The variable err is an integer with a value typically around -5000. The above code isn't correct C code (just barely valid syntax) although the compiler will accept it while issuing a warning about bad subscripting.

SSLServerSocket.c: In function 'Java_org_mozilla_jss_ssl_SSLServerSocket_socketAccept':
SSLServerSocket.c:127: warning: array subscript is below array bounds
SSLServerSocket.c:133: warning: array subscript is below array bounds
SSLServerSocket.c:136: warning: array subscript is below array bounds

I think the original author forget they weren't writing Java code. As far as I can tell what occurs is the pointer to the error string in the static data segment is offeset by the value of the err code to produce a totally bogus pointer. I think we've been lucky it hasn't segfaulted.

Here is an example of what happens at run time:

java.net.SocketException: SSL handshake error java.net.SocketTimeoutException: login to token: (-5990) I/O operation timed out.

The error string in the call to JSSL_throwSSLSocketException() is replaced by another string entirely. In this instance causing a developer to investigate token login problems when that wasn't the issue at all.

This is a great example of why we shouldn't ignore compiler warnings.

I'm going to attach 2 patches. The first one is a minimal patch that just corrects the above incorrect behaviour. I also went through and fixed most of the warnings, I don't think the other warnings were as serious. The second patch just leaves a couple of warnings concerning unreferenced values that do need to be present. I didn't have the time to figure out how to silence those.

Applying the first patch is an absolute must.

Applying the second patch is probably a good idea, should probably be reviewed to assure the initial values I gave uninitialized variables are correct and to figure out the right way to address the few remaining warnings.
Comment 1 John Dennis 2010-11-18 15:00:00 UTC 
Created attachment 461316 [details]
Minimal patch to correct bad string pointer

Note, there is no need to specify the err code, JSSL_throwSSLSocketException() will add that information.
Comment 2 John Dennis 2010-11-18 15:01:39 UTC 
Created attachment 461317 [details]
Clean up most compiler warnings

Includes the error string pointer fix, but also addresses most of the other compiler warnings.
Comment 4 Christina Fu 2010-11-22 22:14:43 UTC 
Comment on attachment 461316 [details]
Minimal patch to correct bad string pointer

approval for this attachment only.

Please consult mharmsen on details of how to do JSS patches.
Comment 7 John Dennis 2010-12-15 17:27:50 UTC 
patch was checked into Fedora for rawhide, F14 & F13 on 12/6 and packages were built and pushed to updates-testing.

However, I noticed the RHEL-6 is does not have the last 4 patches which are in Fedora, will follow up with Matt and Christina to see why not and if there is any reason they should be applied to the RHEL-6 version as well.
Comment 8 John Dennis 2010-12-17 15:34:20 UTC 
jss-4_2_6-10 has now been committed to rawhide, F14, F13 and RHEL-6. All packages have been built for rawhide, F14, F13 and RHEL-6.

Final changes included fixing the attribution/blame in the changelog entry and relocating the jar to /usr/lib/jss and/or /usr/lib64/jss as per Fedora packaging guidelines. Reviews performed by mharmsen and cfu.
Comment 9 Christina Fu 2011-07-26 00:23:20 UTC 
John,  Kashyap was asking how to test this.  If not too much trouble, could you give him a test case that could allow him to observe what would cause JSSL_throwSSLSocketException() now to spit out the correct message then that would be good.  Thanks!
Comment 10 John Dennis 2011-07-26 14:10:37 UTC 
It's been so long since I discovered this I forget how I discovered it or what provoked it. We fixed a JSS socket timeout parameter problem at about the same time which is what I think provoked this error, perhaps you could revert that patch. Or perhaps you could set up a firewall to prevent communication on the specified port which would cause the timeout to occur.

Or perhaps you could have faith in static source code analysis and accept the previous code was indeed wrong and was properly corrected :-) I realize it's better to test, sometimes the cost/benefit is not proportional, especially considering you only hit this code in aberrant circumstances, which is probably why the bogus code was in production for 8 years (if I recall correctly).