When doing security code review, you sometimes come across infuriating code—code that appears to be vulnerable but isn't, due to unexpected side effects.

I recently came across such an example in the Scala library jasongoodwin/authentikat-jwt/. The library supports the None algorithm by default. At least, that's what it seems from the apply methods:

object JsonWebSignature {
  […]
  def apply(algorithm: String, data: String, key: String): Array[Byte] = {
    algorithm match {
      case "HS256" ⇒ apply(HS256, data, key)
      case "HS384" ⇒ apply(HS384, data, key)
      case "HS512" ⇒ apply(HS512, data, key)
      case "none"  ⇒ apply(NONE, data, key)
      case x       ⇒ throw new UnsupportedOperationException(x + " is an unknown or unimplemented JWT algo key")
    }
  }

  def apply(algorithm: Algorithm, data: String, key: String = null): Array[Byte] = {
    algorithm match {
      case HS256 ⇒ HmacSha("HmacSHA256", data, key)
      case HS384 ⇒ HmacSha("HmacSHA384", data, key)
      case HS512 ⇒ HmacSha("HmacSHA512", data, key)
      case NONE  ⇒ Array.empty[Byte]
      case x     ⇒ throw new UnsupportedOperationException(x + " is an unknown or unimplemented JWT algo key")
    }
  }

The following code is then used to verify if a token is valid:

  def validate(jwt: String, key: String): Boolean = {

    import org.json4s.DefaultFormats
    implicit val formats = DefaultFormats

    jwt.split("\\.") match {
      case Array(providedHeader, providedClaims, providedSignature) ⇒

        val headerJsonString = new String(decodeBase64(providedHeader), "UTF-8")
        val header = JwtHeader.fromJsonStringOpt(headerJsonString).getOrElse(JwtHeader(None, None, None))

        val signature = encodeBase64URLSafeString(
          JsonWebSignature(header.algorithm.getOrElse("none"), providedHeader + "." + providedClaims, key))

        java.security.MessageDigest.isEqual(providedSignature.getBytes(), signature.getBytes())
      case _ ⇒
        false
    }
  }

The code splits the token based on the dot (.), retrieves the algorithm from the header and use it to compute a signature signature. It then compares this signature with providedSignature. If the values match, the token is valid. The comparison uses a time-constant check by leveraging java.security.MessageDigest.isEqual().

At first glance, you might think, "This is definitely vulnerable to None algorithm attacks!" And you'd be right to suspect it. But as with many things in security code review, the devil is in the details.

If we look into it in more details:

    jwt.split("\\.") match {
      case Array(providedHeader, providedClaims, providedSignature) ⇒

Here, the code ensures there are exactly three parts when it splits the string, thanks to the line case Array(providedHeader, providedClaims, providedSignature).

If the split doesn't result in exactly three parts, the code returns false:




      case _ ⇒
        false

Now there is the catch! A call to split(String regex) is the equivalent to calling split(String regex, 0), which has the following behavior according to the documentation:

public String[] split(String regex)
Splits this string around matches of the given regular expression.
This method works as if by invoking the two-argument split method with the given expression and a limit argument of zero. Trailing empty strings are therefore not included in the resulting array.

And the documentation of public String[] split(String regex, int limit) confirms it:



If the limit is zero then the pattern will be applied as many times as possible, the array can have any length, and trailing empty strings will be discarded.

Basically, we need an empty string for the None algorithm but split() will never return an empty string.

It’s one thing to read the documentation, but another to trust it. To verify this behavior, we can quickly use a bit of fuzzing to check if the expected behavior holds:





import java.util.ArrayList;
import java.util.List;

public class DotFuzzer {
    private static final String BASE = "a.b.FUZZ";
    private static final int MAX_LENGTH = 4;

    public static void main(String[] args) {
        fuzz(BASE, 0, "");
    }

    private static void fuzz(String base, int length, String replacement) {
        // Stop recursion if the replacement string exceeds MAX_LENGTH
        if (length > MAX_LENGTH) return;

        // Replace "FUZZ" in the base string with the current replacement
        String testString = base.replace("FUZZ", replacement);

        // Split the result by dots
        String[] parts = testString.split("\\.");

        // Check if it meets the criteria: exactly 3 parts and the last part is empty
        if (parts.length == 3 && parts[2].isEmpty()) {
            System.out.println("Success with replacement: " + replacement);
            return;
        }

        // Loop through all byte values from 0x00 to 0xFF and recursively generate more characters
        for (int i = 0; i <= 0xFF; i++) {
            char currentChar = (char) i;
            fuzz(base, length + 1, replacement + currentChar);
        }
    }
}

And fortunately for the developer, it seems like we cannot have an empty trailing string and three parts.

A common recommendation for JWT parsing is usually to split based on the dot (.) to get exactly three parts using split(..., 3). Applying this recommendation to this codebase will actually make the code vulnerable.

Understanding the intricacies of string handling in JWT validation is essential for effective security code reviews. This example highlights the importance of scrutinizing assumptions about code behavior and emphasizes that even well-intentioned improvements can introduce vulnerabilities.