As the new password reset flow will be appearing in 2.1 and not 2.0.5 as originally planned, I have updated our password generation to use SecureRandom.

I have also updated xs:ID generation to use SecureRandom.

For xs:ID generation, it's quite simple:

SecureRandom r = SecureRandom.getInstance("SHA1PRNG");
r.setSeed(SecureRandom().generateSeed(20));

(You might have to catch NoSuchAlgorithmException and choose a suitable fallback.)

Let's quote:

A user could potentially reset their own password a few times in order to get some samples of the obfuscated Random output. It's probably computationally feasible to take the obfuscated output and work out the Random seed.

Thank you for noticing the attack I've described. It takes less than a day to run on an Atom 270.

The Crowd authentication layer should be responsible for preventing brute force attacks on login, thus there is no reason for this random password to be cryptographically secure

There is no reason for passwords to be insecure. If the intention is to disable login until the user clicks "reset password", the traditional way is to store an invalid password hash (in /etc/password this is "x" or "*"). Alternatively, 160 bits of /dev/urandom is probably "good enough".

If the crowd.properties file is not secure then you have bigger problems than guessing a user's password as you have total access to Crowd at that stage

The password is guessable from System.nanoTime() at which the XML backup was imported; on OSX this is actually gettimeofday() which has microsecond resolution. It seems unfeasible to prevent brute-force attacks on applications, so if I know the minute in which the XML backup was restored, I can hit the server with 60M requests and recover the Crowd password.

There is no reason for a "master password" to only have 48 bits of entropy.

... sets the token seed (really a salt) for use when producing Crowd SSO token values. The value is only accessible to Crowd administrators ...

You haven't said how secure it needs to be, so I'll assume that it would allow anyone to log in as anyone else.

The question is what else is hashed in the Crowd SSO token. My guess (I haven't looked at the code) is that it's used as MAC key, i.e. the user knows "user-visible data" and H(token seed, "user-visible data"). This lets the user brute-force the token seed. 48 bits is not secure.

As above, it is also guessable from System.nanoTime() at install time.

saml.Util: there is no need for cryptographically secure IDs. This requirement is specified in the SAML specs as referenced in the javadoc.

I didn't say it needed to be cryptographically secure. Let's quote SAML 2.0 Core §1.3.4: "In the case that a random or pseudorandom technique is employed, the probability of two randomly chosen identifiers being identical MUST be less than or equal to 2^-128 and SHOULD be less than or equal to 2^-160. This requirement MAY be met by encoding a randomly chosen value between 128 and 160 bits in length. The encoding must conform to the rules defining the xs:ID datatype. A pseudorandom generator MUST be seeded with unique material in order to ensure the desired uniqueness properties between different systems."

java.util.Random only has 48 bits of state. It cannot generate more than 2⁴⁸ outputs. You can expect a collision after about 2²⁴ (16.7 million is tiny!) outputs. It is seeded with System.nanoTime(), which is not guaranteed to be "unique" (Java 1.5 only tries to guarantee uniqueness within the same VM, i.e. if you write Random a = new Random(), b = new Random();, a and b should generate different values); it's probably feasible that different instances will be seeded with the same input.

There are no usages of RandomGenerator in Crowd. Without looking at actual usages in other products, it's hard to tell whether having a little less entropy breaks the implied security of the usage. If you do find something of concern then please raise a bug with the relevant product.

A "little less entropy" is not the problem; RandomGenerator has "password-generating" functions which generate guessable passwords. I'd remove it and see what builds broke; obviously you are in a better position to do this than I am.

We do treat security seriously at Atlassian and also with Crowd, and if a security vulnerability is found, (and they have been found), we will analyse that attack vector and protect against it accordingly.

If you do have a specific attack vector against Crowd, please let us know, e.g. you have found a way to programatically guess the next token used by Crowd.

You have raised an issue concerning the use of java.util.Random with password generation, and we agree that this is an issue. Shihab has spent considerable time over the last day looking at each specific usage of java.util.Random, and importantly where it used with regards to auto-generated 'random' passwords.

We agree that its use for password generation for user credentials is incorrect and in the upcoming release of Crowd (2.0.5) we are removing this ability (see: CWD-1875).

However, just removing all usages of java.util.Random from every part of every Atlassian products code base is both impractical and unnecessary. Each usage must be looked at on its own merits and analysed accordingly, and as noted Shihab has provided this analysis in detail. If a piece of code needs to be cryptographically more secure we will use SecureRandom.

Cheers,

Justin

It doesn't matter if the password is "exposed" or not; the password is weak and should not be used. It's like choosing MD5 over SHA-1 — you might not see any any attacks (and, indeed, your use of MD5 may not be insecure), but that's not a good reason to choose an insecure function. How can you hope to write secure software if you cherry-pick the things that are "security critical" and ignore the rest?

For example, the token seed is long-lived but has less than 48 bits of entropy. An attack's not going to take that long.

This isn't a "conglomerate issue"; it's a persistent design flaw. If you don't care about your source of entropy, you shouldn't be writing any kind of crypto. I've lost any faith I had in the security of Atlassian products.

I am resolving this issue because it's a conglomerate issue that covers a lot of things. In particular, we will not fix:

• Admin actions that generate a password: we will not be changing this as the password is never exposed.
• Generating a password in crowd.properties: the password is not exposed beyond the filesystem.
• Usages of Random in tests, etc.

However, we will fix:

• Password resets to use SecureRandom: CWD-1875
• Salting so that it is per user: CWD-1923

java.util.Random is no doubt insecure.

Let's go through the usages in our code and see the ramifications:

• PasswordHelper:
  • DirectoryManager.resetPassword: this is called when a user requests their password to be reset. It is exposed in the UI and also via the SOAP API. Either code path is accessible, in particular, the Crowd UI code path does not require any authentication. So someone can click the "forgotPassword" link and get an email sent to their email address notifying them of their new (java.util.Random) password. A user could potentially reset their own password a few times in order to get some samples of the obfuscated Random output. It's probably computationally feasible to take the obfuscated output and work out the Random seed. Once the seed is obtained, it's relatively trivial to reset someone else's password and know what their password will be without needing to intercept their email. This particular attack can be mitigated by using a SecureRandom. In fact, our new password reset flow, which is scheduled to be released in the upcoming Crowd 2.0.5 release, will email out a SecureRandom token which can then be exchanged to prove authentication to allow the user to manually change their password. See CWD-1875 for more info.
  • csv.UserMapper, jdbc.UserMapper, ImportJive: used when importing users from a CSV file or from a database, where it is not possible to ascertain the password so a random password is used. This random password is never exposed. We may as well use a fixed password - the only reason we use a random password is to prevent users from manually guessing the password. The Crowd authentication layer should be responsible for preventing brute force attacks on login, thus there is no reason for this random password to be cryptographically secure as it does not ever leak out, not even to the admin.
  • CrowdApplicationPasswordManagerGeneric: used to reset the Crowd application's password on successful import of crowd 1.1.x and 1.0.x xml backups where the crowd.properties aren't stored in the database. This random value leaks out to crowd.properties, which is only viewable by users that have access to the filesystem. If the crowd.properties file is not secure then you have bigger problems than guessing a user's password as you have total access to Crowd at that stage (you could change a user's password to whatever you want via the SOAP API, masquerading as Crowd), ie. Random vs SecureRandom in this case makes no difference.
  • UpdateGeneral: sets the token seed (really a salt) for use when producing Crowd SSO token values. The value is only accessible to Crowd administrators so using a Random vs SecureRandom makes no difference - if you have admin access, you can change anyone's password.
  • DefaultAdministrator: generates a default crowd.properties password on setup. The implications are identical to CrowdApplicationPasswordManagerGeneric.
  • Options: sets the token seed on setup. The implications are identical to UpdateGeneral.

Further:

• There are no usages of RandomGenerator in Crowd. Without looking at actual usages in other products, it's hard to tell whether having a little less entropy breaks the implied security of the usage. If you do find something of concern then please raise a bug with the relevant product.
• DirectoryPluginLoaderUtils: this does not need to be secure, the random characters are just for creating a unique filename on the filesystem, and from the code it looks like a test directory. I am not sure why it doesn't use the random function of another library - perhaps to reduce dependencies on the module itself.
• SimpleXsrfTokenGenerator: this is not used in Crowd yet but is used in other Atlassian apps. It is computationally feasible to calculate the Random seed, so you can always work out the next XSRF token value. However, this attack is only really feasible on an idle server since serving a different session would move the state of the Random to the next number - making it difficult to target a particular user. However, this seems like a bug that's worthwhile raising in the appropriate products such as Confluence and JIRA. Confluence, for example, is putting in place a web "sudo" mechanism to require all admin actions to require explicit username/password authentication.

The usages of Math.random() you have noted are in tests. Perhaps CrowdWebResourceIntegration could change to using a local Random instance. The benefit would be the disappearance of thread contention as noted in the javadocs. If you look at the surrounding code, you will see that you only use the Math.random() if application caching has been globally disabled - in which case performance is not of concern.

Also:

• TokenGeneratorImpl: the random number is never exposed. It's used internally to avoid session fixation attacks on the Crowd cookie and is hashed in the Crowd cookie value.
• saml.Util: there is no need for cryptographically secure IDs. This requirement is specified in the SAML specs as referenced in the javadoc.
• seraph.util.EncryptionUtils: fixed salts allow a database or XML dump of the password hashes to be reversed via rainbow tables in a computationally feasible manner. Crowd uses per instance salts, which are a step better than fixed salts. I have created another issue for per user salting in Crowd: CWD-1923

We appreciate your research into the matter, but I think in general, we shouldn't just move everything to SecureRandom in place of Random. Random does have it's weaknesses, but it's strength is that it doesn't need to wait for entropy to initialise whereas SecureRandom does and further, each invocation to get the next random number on a SecureRandom is synchronised.

If there is a specific reason or need for something to be cryptographically secure, then it must use SecureRandom.

So I looked at the code.

What's Random() seeded with?

• J2SE 1.4: Random() is roughly Random(System.currentTimeMillis()). There have been around 1.2e9 milliseconds since the Star Wars Day release. This is bruteforceable in about 20 seconds if you can get the first password reset after launch.
• J2SE 1.5/5.0: Random() is roughly Random(System.nanoTime()+ ++someCounter), which provides more uniqueness (oddly they don't use an AtomicLong counter). This means you can do better than 2⁴⁸/91 if you know the value of System.nanoTime() within about 51 minutes (i.e. ±25 minutes). On Linux, it appears to be the system uptime, so you just ask Netcraft/nmap/whatever. On OSX, it's just gettimeofday() so you only need to know the start time within 35 days (due to microsecond granularity). On either system, you can just wait for a reboot (e.g. shortly after a kernel patch).

Of course, none of those attacks are worth the trouble. My boring Java implementation of the full 2⁴⁸/91 bruteforce takes about 19.7 hours on one core of a Core2 6600 (2.40GHz), or about 23 nanoseconds per guess. The C version takes around 16 nanoseconds per guess with the right optimizations. It's trivially parallelizable and vaguely vectorizable.

It's even fast on a low-end system: it's 50 ns per guess on one thread on an Atom 330 (1.6 GHz, 2x2HT); this rises to 66 ns per guess per thread on four threads (I would've expected the HT overheads to be worse) for a combined throughput of 61M/s, which makes it approximately equivalent to one thread on a Core2 6600. I'm using a D945GCLF2; the successor D510MO is only £65 including shipping.

My point is that it falls below the "I'm bored on a weekend" threshold. I spent most of the time investigating Random and RandomStringUtils, or playing with GCC optimization options to squeeze out the last 10%; I didn't expect the bruteforce to be fast enough so it wasn't what I focused on initially.

I'll post the code after you've had some time to fix this

This bug is about the use of java.util.Random (a textbook error), not the "reset password" workflow.

PasswordHelper is used in several places, all of which appear to be security-critical:

atlassian-crowd/components/crowd-core/src/main/java/com/atlassian/crowd/integration/authentication/PasswordHelper.java:    public String generateRandomPassword()
atlassian-crowd/components/crowd-core/src/main/java/com/atlassian/crowd/manager/application/CrowdApplicationPasswordManagerGeneric.java:            final String password = passwordHelper.generateRandomPassword();
atlassian-crowd/components/crowd-core/src/main/java/com/atlassian/crowd/manager/directory/DirectoryManagerGeneric.java:            String password = passwordHelper.generateRandomPassword();
atlassian-crowd/components/crowd-importer/src/main/java/com/atlassian/crowd/importer/importers/DirectoryImporter.java:                    credential = new PasswordCredential(passwordHelper.generateRandomPassword(), false);
atlassian-crowd/components/crowd-importer/src/main/java/com/atlassian/crowd/importer/mappers/csv/UserMapper.java:                credential = new PasswordCredential(passwordHelper.generateRandomPassword(), false);
atlassian-crowd/components/crowd-importer/src/main/java/com/atlassian/crowd/importer/mappers/jdbc/UserMapper.java:            credential = new PasswordCredential(passwordHelper.generateRandomPassword(), false);
atlassian-crowd/components/crowd-web-app/src/main/java/com/atlassian/crowd/console/action/dataimport/ImportJive.java:                PasswordCredential credential = new PasswordCredential(passwordHelper.generateRandomPassword(), false);
atlassian-crowd/components/crowd-web-app/src/main/java/com/atlassian/crowd/console/action/options/UpdateGeneral.java:            setSeed(passwordHelper.generateRandomPassword());
atlassian-crowd/components/crowd-web-app/src/main/java/com/atlassian/crowd/console/action/setup/DefaultAdministrator.java:        final String crowdApplicationPassword = passwordHelper.generateRandomPassword();
atlassian-crowd/components/crowd-web-app/src/main/java/com/atlassian/crowd/console/action/setup/Options.java:            String seed = passwordHelper.generateRandomPassword();

There are similar functions in "core" code:

• atlassian-core/src/main/java/com/atlassian/core/util/RandomGenerator.java consists of entirely insecure functions which use Math.random(). Even if it was fixed to use SecureRandom, randomCharacter() is biased - it outputs at most 5.83 bits of entropy instead of the optimal 5.95. It's unclear why this exists in addition to PasswordHelper.
• atlassian-plugins-core/src/test/java/com/atlassian/plugin/loaders/classloading/DirectoryPluginLoaderUtils.java defines its own randomAlpha() and randomString(). It's debatable whether it needs to be secure, but why can't it use atlassian-core or apache.commons.lang?
• atlassian-xwork-core/src/main/java/com/atlassian/xwork/SimpleXsrfTokenGenerator.java generates a 64¹⁰ string which needs to be secure against XSRF. Instead, it uses java.util.Random and encodes 60 bits of random.nextLong() which is enough to know the entire state, and thus guess everyone else's XSRF token.

There are several other users of Math.random() which probably should be using Random.nextInt(int):

atlassian-core/src/test/java/com/atlassian/core/spool/AbstractSpoolTest.java:            data[x] = (byte) (Math.random() * Byte.MAX_VALUE);
atlassian-crowd/components/crowd-acceptance-test/src/main/java/com/atlassian/crowd/acceptance/tests/directory/monitor/ActiveDirectoryMonitorTest.java:    private static final String SUFFIX = "-" + Math.round(Math.random() * 1000); // introduce voodoo to reduce collisions :)
atlassian-crowd/components/crowd-acceptance-test/src/main/java/com/atlassian/crowd/acceptance/utils/AcceptanceTestHelper.java:                fos.write((int) (Math.random() * Byte.MAX_VALUE));
atlassian-crowd/components/crowd-plugins/src/main/java/com/atlassian/crowd/plugin/web/CrowdWebResourceIntegration.java:            return (String.valueOf((int) (Math.random() * 100000) + 1));
atlassian-user/src/test/java/com/atlassian/user/impl/osuser/security/password/TestOSUPasswordEncryptor.java:            int ch = (int) (Math.random() * 127);
atlassian-user/src/test/java/com/atlassian/user/impl/osuser/security/password/TestOSUPasswordEncryptor.java:            int x = (int) (Math.random() * 1);

And a bunch of others:

• atlassian-crowd/components/crowd-core/src/main/java/com/atlassian/crowd/authentication/TokenFactoryImpl.java is easy to use incorrectly, since it accepts a Random in the constructor (apparently entirely to facilitate testing). The preferred constructor should use a new SecureRandom().
• atlassian-crowd/components/crowd-integration/crowd-integration-saml/src/main/java/com/atlassian/crowd/plugin/saml/Util.java pulls out 160 bits from java.util.Random, even though java.util.Random only has 48 bits of state. If (as I suspect to be the case) that it's intended for preventing xsd:ID collisions, then this is insufficient; instead, use a 160-bit seed (from SecureRandom at load time) and an AtomicLong counter, and output SHA-1(seed, counter.incrementAndGet()) where the counter value is represented as 8 bytes in little-endian. The choice of 160 bits really ought to read as "SHA-1 something and give me the output".
• atlassian-seraph/src/main/java/com/atlassian/seraph/util/EncryptionUtils.java is highly suspicious. It uses a fixed "salt" (this is often no better than using zeroes), DES, and encodePageNumber() uses Math.random() for no apparent added security.

We'll shortly be looking at fixing the entire workflow around this.