Bug 880279 - plexus-cipher: improper use of SecureRandom in PBECipher
Summary: plexus-cipher: improper use of SecureRandom in PBECipher
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: plexus-cipher
Version: 18
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Mikolaj Izdebski
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 880257 880673
TreeView+ depends on / blocked
 
Reported: 2012-11-26 15:50 UTC by Florian Weimer
Modified: 2012-12-06 07:27 UTC (History)
3 users (show)

Fixed In Version: 1.5-11
Clone Of:
: 880673 (view as bug list)
Environment:
Last Closed: 2012-12-06 06:59:53 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Proposed patch (2.89 KB, patch)
2012-11-27 14:44 UTC, Mikolaj Izdebski
no flags Details | Diff

Description Florian Weimer 2012-11-26 15:50:27 UTC
Description of problem:

src/main/java/org/sonatype/plexus/components/cipher/PBECipher.java contains this code (slightly abbreviated):

    protected SecureRandom _secureRandom;


    public PBECipher() throws PlexusCipherException
    {
        try
        {
            _digester = MessageDigest.getInstance( DIGEST_ALG );
            if( System.getProperty( "os.name", "blah" ).toLowerCase().indexOf( "linux" ) != -1 )
                _onLinux = true;
            if( _onLinux )
                System.setProperty( "securerandom.source", "file:/dev/./urandom");
            else
                _secureRandom = new SecureRandom();
        }
        catch ( NoSuchAlgorithmException e )
        {
            throw new PlexusCipherException(e);
        }
    }

So the constructor sets a system property (bad), and does not initialize _secureRandom (worse).  Because of this, getSalt() falls back to Random (seeded by the current time) instead of SecureRandom:

    private byte[] getSalt( final int sz )
    throws NoSuchAlgorithmException, NoSuchProviderException
    {
        byte [] res = null;
        
        if( _secureRandom != null )
        {
            _secureRandom.setSeed( System.currentTimeMillis() );
            res = _secureRandom.generateSeed( sz );
        }
        else
        {
            res = new byte[ sz ];
            Random r = new Random( System.currentTimeMillis() );
            r.nextBytes( res );
        }

        return res;
    }

The if branch is not executed, but neither SecureRandom#setSeed(long) nor SecureRandom#generateSeed(int) are appropriate methods to call in this context.  You should allocate a byte array and call SecureRandom#nextBytes(byte[]).

getSalt() is called from here:

    public String encrypt64( final String clearText, final String password )
    throws PlexusCipherException
    {
        try
        {
            byte[] clearBytes = clearText.getBytes();
    
            byte[] salt = getSalt( SALT_SIZE );
            
            // spin it :)
            if( _secureRandom != null )
            {
                new SecureRandom().nextBytes( salt );
            }

Constructing a short-lived SecureRandom object is not a good idea because it is a very costly operation.

To fix this, no system properties should be examined or set, and a static SecureRandom instance should be used to generate the salts, using SecureRandom#nextBytes(byte[]).  This should simplify the code considerably.  The default SecureRandom implementation used to block on older/proprietary JDKs (and that's probably reason for this convoluted code), but that workaround should no longer be needed.

These bugs just decreases the randomness of the salt/IV, so they may not actually result in an exploitable security vulnerability.  But that depends on how this class is used.

Note that I haven't verified if this class actually implements a strong encryption scheme.

Version-Release number of selected component (if applicable):

plexus-cipher-1.5.8-fc18

Comment 1 Mikolaj Izdebski 2012-11-27 14:44:19 UTC
Created attachment 652787 [details]
Proposed patch

I have replaced multiple generator objects with a single static SecureRandom which relies on Java to properly seed it. I removed OS detection as the new code should work properly on all OSes supported by Java. There should be no problems because of API/ABI changes. SecureRandom#nextBytes(byte[]) is thread safe.

Does this patch satisfy you?

Comment 2 Florian Weimer 2012-11-27 14:49:21 UTC
(In reply to comment #1)
> Created attachment 652787 [details]
> Proposed patch
> 
> I have replaced multiple generator objects with a single static SecureRandom
> which relies on Java to properly seed it. I removed OS detection as the new
> code should work properly on all OSes supported by Java. There should be no
> problems because of API/ABI changes. SecureRandom#nextBytes(byte[]) is
> thread safe.
> 
> Does this patch satisfy you?

Looks good, thanks.

Comment 5 Mikolaj Izdebski 2012-11-27 15:10:12 UTC
Fixed in plexus-cipher-1.5-11

Comment 6 Fedora Update System 2012-11-27 15:54:28 UTC
plexus-cipher-1.5-11.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/plexus-cipher-1.5-11.fc16

Comment 7 Fedora Update System 2012-11-27 15:55:05 UTC
plexus-cipher-1.5-11.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/plexus-cipher-1.5-11.fc17

Comment 8 Fedora Update System 2012-11-27 15:55:34 UTC
plexus-cipher-1.5-11.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/plexus-cipher-1.5-11.fc18

Comment 9 Fedora Update System 2012-11-28 02:08:36 UTC
Package plexus-cipher-1.5-11.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing plexus-cipher-1.5-11.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-19162/plexus-cipher-1.5-11.fc18
then log in and leave karma (feedback).

Comment 10 Fedora Update System 2012-12-06 06:59:56 UTC
plexus-cipher-1.5-11.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 11 Fedora Update System 2012-12-06 07:05:31 UTC
plexus-cipher-1.5-11.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 12 Fedora Update System 2012-12-06 07:21:53 UTC
plexus-cipher-1.5-11.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.


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