Bug 880279

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