Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 354299

Summary: org.eclipse.equinox.internal.security.storage.Base64 is not threadsafe
Product: [Eclipse Project] Equinox Reporter: Matt Lavin <matt_lavin>
Component: SecurityAssignee: Security Inbox <equinox.security-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: critical    
Priority: P3 CC: john.arthorne, ob1.eclipse, remy.suen, tjwatson
Version: 3.7   
Target Milestone: Juno M2   
Hardware: PC   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 355400    
Attachments:
Description Flags
proposed fix none

Description Matt Lavin CLA 2011-08-09 16:31:00 EDT 
Build Identifier: 

If more than one thread calls Base64.decode at the same time, the wrong value can be returned to the user.  It's possible that this is problem might be the cause for some defects related to our product:

https://jazz.net/jazz/resource/itemName/com.ibm.team.workitem.WorkItem/171495
https://jazz.net/jazz/resource/itemName/com.ibm.team.workitem.WorkItem/167948


Reproducible: Sometimes

Steps to Reproduce:
The problem can be reproduced by looking at the code.  Imagine two threads calling Base64.decode() at the same time.

Both threads execute until the call to init(), but because init() is synchronized only one will enter.  Now, one thread is executing in init() and the other is waiting to enter the synchronized method.

After the first thread finishes with init() it proceeds through the rest of the method calling decode() at various places.  If the second thread starts executing init() while decode() is executing in the first thread, then two threads will be accessing a shared field without synchronization.  That can lead to corruption in the decoding of the string because the field used in the decode method might not be initialized while it's used.
Comment 1 Thomas Watson CLA 2011-08-10 09:30:20 EDT 
Seems decodeTable should be final and initialized in a static initializer.
Comment 2 Thomas Watson CLA 2011-08-10 10:41:42 EDT 
Oleg, do you know the history of this version of the org.eclipse.equinox.internal.security.storage.Base64 class.  We have many copies of a class called Base64 in Eclipse, but this version looks to be a completely different implementation.

The other implementations are:

org.eclipse.osgi.internal.signedcontent.Base64
org.eclipse.core.internal.preferences.Base64
org.eclipse.ui.internal.preferences.Base64

These versions do not have this issue.  I am wondering if we should just copy one of these versions of Base64 or try to fix this version.
Comment 3 Oleg Besedin CLA 2011-08-10 11:04:34 EDT 
(In reply to comment #2)
> The other implementations are:
> 
> org.eclipse.osgi.internal.signedcontent.Base64
> org.eclipse.core.internal.preferences.Base64
> org.eclipse.ui.internal.preferences.Base64
> 

None of those are APIs. At the time we discussed making Base64 processing an API, but that was buried in usual questions on where it is going to reside, how we make other classes to switch to it, etc.

Due to this I only briefly looked into other classes. I am not sure if the processing they provide is identical. Unless the changes to secure storage's Base64 are done in a way the preserves it "input -> output" behavior, it will break the ability to decrypt currently stored data.
Comment 4 Thomas Watson CLA 2011-08-11 13:25:18 EDT 
Created attachment 201337 [details]
proposed fix

(In reply to comment #3)
> None of those are APIs. At the time we discussed making Base64 processing an
> API, but that was buried in usual questions on where it is going to reside, how
> we make other classes to switch to it, etc.
> 
> Due to this I only briefly looked into other classes. I am not sure if the
> processing they provide is identical. Unless the changes to secure storage's
> Base64 are done in a way the preserves it "input -> output" behavior, it will
> break the ability to decrypt currently stored data.

Do to your concerns the safest fix is to fix this version.  Here is my proposed fix.
Comment 5 Oleg Besedin CLA 2011-08-11 14:46:07 EDT 
(In reply to comment #4)
> Created attachment 201337 [details]
> proposed fix

+1, looks good.
Comment 7 Thomas Watson CLA 2011-08-11 18:09:26 EDT 
I forgot to update the version.  That is done with commit:

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=9dfa830ee7dcc2978dc6a34bcd801a1577cc4eab