SecretKey algorithm did not equal one of the three required JCA

[rodmaz]

Hi there,

We have struggled for over 2 hours to get this library to work (for a such a simple task (JWT) as to generate a simple JWT). Not a good start!

We end up figuring out the final problem to be this exception:

SecretKey algorithm did not equal one of the three required JCA algorithm names of HmacSHA256, HmacSHA384, or HmacSHA512.

The collection PREFERRED_HMAC_ALGS has jcaNames internally as HmacSHA256 etc. using camelcase. However when we generate a key using the library:

val key = Keys.secretKeyFor(SignatureAlgorithm.HS256)

The internal jcaName generated in the key is HMACSHA256, all uppercase. The method SignatureAlgorithm() throws an exception [here]

jjwt/api/src/main/java/io/jsonwebtoken/SignatureAlgorithm.java

Line 560 in 1839ebf

if (alg.jcaName.equals(secretKeyAlg)) {

Are we doing something wrong? It can't be this library has such a stupid string comparison bug and nobody else has notice it?!

We are using the version 0.9.0 from master.

[lhazlewood]

0.9.0 does not have the logic you describe. Here is the source for that release: https://github.com/jwtk/jjwt/blob/0.9.0/src/main/java/io/jsonwebtoken/SignatureAlgorithm.java

I understand why you'd be frustrated after two hours, but we're here to help, please try not to be too confrontational. The entire codebase has 100% code coverage, so we try extremely hard to ensure that everything works perfectly, but even code coverage assertions have their faults.

The internal JCA provider should have identical names on both creation and key.getAlgorithm() results. Could you please tell us if you're running standard JDK, Android or BouncyCastle?

[lhazlewood]

I just ran a test and this succeeds:

@Test
    void testSecretKeyFor() {

        for (SignatureAlgorithm alg : SignatureAlgorithm.values()) {

            String name = alg.name()

            if (alg.isHmac()) {
                SecretKey key = Keys.secretKeyFor(alg)
                assertEquals alg.minKeyLength, key.getEncoded().length * 8 //convert byte count to bit count
                assertEquals alg.jcaName, key.algorithm
                alg.assertValidSigningKey(key)
                alg.assertValidVerificationKey(key)
                assertEquals alg, SignatureAlgorithm.forSigningKey(key)
            } else {
                try {
                    Keys.secretKeyFor(alg)
                    fail()
                } catch (IllegalArgumentException expected) {
                    assertEquals "The $name algorithm does not support shared secret keys." as String, expected.message
                }

            }
        }
    }

Notice the lines:

alg.assertValidSigningKey(key)
alg.assertValidVerificationKey(key)
assertEquals alg, SignatureAlgorithm.forSigningKey(key)

These assert that the key (created from Keys.secretKeyFor(alg)) passes the validation rules in the SignatureAlgorithm class.

The only thing I can think of: what version of the JDK are you using?

[lhazlewood]

Even with the test the JCA name is less important than the key length, so I'll use this ticket to change the assertion from a name to a key length. We'll get a quick 0.10.3 release out asap to address this.

Even so, what version of the JDK are you using?

[rodmaz]

Hi Les,

Thanks for your prompt feedback. Our engineer directly in charge of this actually already left for today. We found this issue by actually stepping into the class SignatureAlgorithm and the string comparison fails, as we described.
We are actually running this on Android and the project is written in Kotlin. Perhaps we have an issue here. I will let you know more details tomorrow.

Honestly, we also found very strange that a library so popular has such a stupid issue, but we can actually reproduce it at least on Android.

TTY tomorrow.

Thanks again!

[rodmaz]

Anyway, I agree that directly string comparison (case-sensitive) is not the best solution. On Android we may have it in camelcase, as it seems to be the case. Are we the first ones to use it on Android? 🤔

[lhazlewood]

Hi @rodmaz

Thanks for letting me know.

It's not a stupid issue in JJWT however - the release where you're seeing the issue is only a week or so old. I have had feedback from Android devs that are using JJWT 0.10.x in prod - they probably just haven't come across this particular edge case based on how they use JJWT.

What is stupid IMO is that the Android JVM has an internal discrepancy on how it names JCA algorithms. This problem doesn't exist on any JDK that we test on (7, 8, 9 and 10).

Our build system tests on all of these JDKs, but, to the best of my knowledge, there is no Travis CI ability to run both JDK and Android VMs in a single Travis CI project, otherwise we would have caught this (IMO bug) that is Android's fault.

If anyone knows how to add Android VMs into our existing Travis setup, I'd love to know how. Otherwise, we try really hard to test things based on Android's public APIs and cursory testing from the Android community. I'd love some extra Android CI help here if possible.

[lhazlewood]

P.S. I'm cutting a 0.10.3 release tonight so you shouldn't see this issue (since the assertion will be on key lengths only). Your engineer should be able to try tomorrow with the new 0.10.3 release and have it work out. If not, don't worry - I'm happy to help and we'll get you guys up and running asap. :)

[rodmaz]

@lhazlewood Thanks Les! We were about to fork it and do a pull-request tomorrow. We will analyse and let you know.

[rodmaz]

We also saw some (in fact many) situations in which the JJWT actually freezes on Android when we called the compact() method (and no exception is thrown). This may be yet another issue w/ Android.

[lhazlewood]

@rodmaz any information on those freezes would be greatly appreciated. There are a ton of people using JJWT in Android, so if we can fill in any remaining gaps, all the better.

[lhazlewood]

Released with 0.10.3 - please allow 30 min or so to propagate to Maven Central.

[rodmaz]

@lhazlewood This is what we see on Android 5.1 (old) and Android 8 (Samsung). Even when using Android emulator on macOS the problem exists with camelcase.

We cannot understand how this could possibly have worked on Android before.
Right now we are testing the latest release version and will provide you a feedback ASAP.

[rodmaz]

@lhazlewood We just tested version 0.10.3 and it turns out the problem persists. The class SignatureAlgorithm checks against the jcaName in other places. Now the problem occurs in another line in the same class.
See this screenshot.

[lhazlewood]

Thanks for the follow up - I really appreciate it.

In my haste to get you up and running, I only fixed what was reported. I'll do a full usage search of jcaName and ensure comparisons are case-insensitive. The check still should be there to prevent using invalid keys for a different algorithm for which it was intended, but clearly Android likes to change standard, documented JCA name casing at their whim.

I'll fix this asap and get 0.10.4 out. Sorry for the trouble.

[lhazlewood]

P.S. Does anyone know how to test this in CI? We put in a great deal of work to put in 100% code coverage on every build specifically to catch these types of 'gotchas', but that's not guaranteed unless Android is part of the CI runs.

As an example, I found this Android project on GH, and looked at its travis config:

The language is android as expected, but it says the jdk is oraclejdk8.

If that's the case (using Oracle JDK 8), and all JJWT Oracle JDK 8 builds pass without exposing this problem, wouldn't this problem stay hidden and not be resolved with TravisCI?

Or does oraclejdk8 with language android not really mean Oracle's JDK but the Android SDK that is compatible with Oracle JDK 8 language features. Does anyone know?

[rodmaz]

@lhazlewood Thanks!

P.S. We do not use TravisCI but you can also check out this post.

[lhazlewood]

@rodmaz I read the article but the examples there also show using oraclejdk8. So my question still stands. ;)

[lhazlewood]

This is pretty clear to me that this is an Android bug.

The standard names are defined here:

https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#KeyGenerator

And those are the ones JJWT uses. That Android returns something different is a violation of the spec names.

[lhazlewood]

Wow - from that document, nestled in one little innocuous line:

Note: Standard names are not case-sensitive.

Well, this is a good confirmation of the .equalsIgnoreCase change!

[lhazlewood]

Released in 0.10.4. Please allow 30 minutes to propagate to Maven Central.

[rpanak-generalbytes]

@lhazlewood I've encountered a similar bug with loading the key in a PKCS 12 keystore. The algorithm name of the secret key algorithm gets for some reason (guess it's because it's mandated by the PKCS12 spec) converted to an OID (e.g. HmacSHA512 becomes 1.2.840.113549.2.11; see http://www.oid-info.com/cgi-bin/display?oid=1.2.840.113549.2.11&action=display). After loading the key, the algorithm identifier stays in the OID format and thus your check fails. I've solved this problem by switching to a different keystore format (JCEKS in my case). Couldn't make the PKCS12 one work. The sad thing is that PKCS12 is the default since some relatively recent Java release (9 I guess; my problem was on JDK11).

[lhazlewood]

@rpanak-generalbytes can you please help me understand how/where JJWT is involved in what you're seeing? JJWT doesn't load keys from keystores. Do you have any example code that demonstrates a problem? Thanks!

[rpanak-generalbytes]

Sure. You are right, JJWT is not involved with keystores. However, that is just an information about how to reproduce the problem (to be precise, how to unwillingly create a valid SecretKey that is not usable with JJWT).

The problem itself is that you can have a valid SecretKey for HmacSHA512, but the string returned by calling SecretKey#getAlgorithm() will be the OID (1.2.840.113549.2.11), not the actual string "HmacSHA512". Your key validity check (SignatureAlgorithm#assertValid) will thus fail (the OID is not equal to any of the expected algorithm names). I guess SunJCE and other crypto providers do some mapping internally to a canonical name and thus the key works (take a look at this source file, 90 % of the content is OID mapping).

Also, I have found that SignatureAlgorithm#forSigningKey finds the algorithm based on the key length. This is incorrect in my opinion. If I have a 512-bit key for HmacSHA256 (which is a valid key length), the algorithm returns the first HMAC for which the key length is enough, i.e.HmacSHA512. I think that is confusing behavior since the algorithm information is present with the key. I'd understand this alg matching heuristic if the key was given as raw bytes (thus no alg information). This is, however, an unrelated problem so I can create a new issue for this if you'd like. In case you'd be fixing this, the OID matching problem will be present as well.

[fibsifan]

@lhazlewood This is closed now, but I too am experiencing issues with a HmacSha512 Key loaded from a PKCS 12. I'm unsure, whether this is an Issue with the JDK I'm using (OpenJDK in this case) or a problem with jjwt.

The following example might illustrate the problem:

package de.jball.crypt.playground;

import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.SignatureAlgorithm;
import io.jsonwebtoken.security.Keys;

import java.security.Key;
import java.security.KeyStore;
import java.security.cert.Certificate;

public class Main {
	public static void main(String[] args) throws Exception {
		KeyStore pkcs12 = KeyStore.getInstance("pkcs12");
		pkcs12.load(null, "keystorepassword".toCharArray());

		Key key = Keys.secretKeyFor(SignatureAlgorithm.HS512);
		pkcs12.setKeyEntry("testkey", key, "keypassword".toCharArray(), new Certificate[0]);
		key = pkcs12.getKey("testkey", "keypassword".toCharArray());

// Following line fails with
// io.jsonwebtoken.security.InvalidKeyException: The signing key's algorithm '1.2.840.113549.2.11' does not equal a valid HmacSHA* algorithm name and cannot be used with HS512.
		System.out.println(Jwts.builder().signWith(key).setSubject("123").compact());
	}
}

Edit: formatting and reducing example

[rpanak-generalbytes]

I think SignatureAlgorithm#assertValid is still behaving incorrectly. The issue should be reopened.

[fibsifan]

Created a Test Project with the given code example, which can be used to reproduce the issue. Could reproduce with OpenJDK11 as well.

Looking at Java Security Standard Algorithm Names though, I can't find the String '1.2.840.113549.2.11' anywhere in the document. I would have expected the KeyStore to load the key with the Standard Algorithm name. I might be overlooking something though in the JCA Reference, which would indicate this behaviour.

[fibsifan]

Meanwhile I found the following Workaround:

Key key = keyStore.getKey(...);
key = Keys.hmacShaKeyFor(key.getEncoded());

[fibsifan]

Since the corresponding bug report at OpenJDK seems to be accepted and we have a workaround, IMHO this issue can stay closed.

[lhazlewood]

The alternative is to remove these lines from the assertion check entirely:

jjwt/api/src/main/java/io/jsonwebtoken/SignatureAlgorithm.java

Lines 353 to 359 in c09deaa

	// These next checks use equalsIgnoreCase per https://github.com/jwtk/jjwt/issues/381#issuecomment-412912272
	if (!HS256.jcaName.equalsIgnoreCase(alg) &&
	!HS384.jcaName.equalsIgnoreCase(alg) &&
	!HS512.jcaName.equalsIgnoreCase(alg)) {
	throw new InvalidKeyException("The " + keyType(signing) + " key's algorithm '" + alg +
	"' does not equal a valid HmacSHA* algorithm name and cannot be used with " + name() + ".");
	}

But I'm a little hesitant to do that: removing those lines could still allow keys that fail when calling mac.init(key) if the algorithm name isn't valid.

The safest implementation change would be to remove those lines and then actually perform a test mac calculation, e.g.

private void assertValidHmac(Key key) {
    try {
        Mac mac = Mac.getInstance(this.jcaName);
        mac.init(key);
        mac.doFinal(dummyData);
    catch (IllegalStateException ise) {
        throw new InvalidKeyException("whatever", ise);
    }
}

The problem with this approach is that it could be a performance hit due to the extra non-essential calculation. We'd have to ensure that it is used in a calculated fashion and it might be more effort than it's worth, especially with the workaround that @fibsifan posted.

Thoughts?

[rpanak-generalbytes]

@lhazlewood I think this is a no-brainer. The safest implementation is to accept the proper aliases (OIDs) as well as the current algorithm names (they're both just alternative names for the same thing). I don't see any downside (correct me if I'm wrong). Easy to implement as well, just add three more cases into the appropriate if statement. The OIDs can be Googled, or found here or the JDKs sources link I've already posted before.
Definitely don't go the test Mac route, you listed the cons yourself. The same goes for just removing the checks, there's no need to remove them.

@fibsifan Open JDK bug is a long way from fixed :). In the meantime, we're left to deal with the present problem.

[lhazlewood]

Adding the OIDs to the check is easy enough - thanks for suggesting the solution! Reopening so we can address it in a future release.

[lhazlewood]

Actually, closing this (since it is tied to the 0.10.4 release) and opening a new issue to track that work.

[lhazlewood]

OIDs will be added to the validation check via #588