Java: JWT decoding without verification #14089

am0o0 · 2023-08-29T12:43:17Z

this PR is related to github/securitylab#783

intrigus-lgtm · 2023-08-29T13:08:37Z

java/ql/src/experimental/Security/CWE/CWE-347/Example.java

+
+    // only decode, no verification
+    String JwtToken2 = request.getParameter("JWT2");
+    decodeToken(JwtToken2);


Since nobody is using the result of decodeToken why does it matter whether we verify it or not?

:) @intrigus-lgtm really fast response!
it is just an example if it is mandatory that I wrote a complete example then please let me know.

the sink is the jwt claims values so I think the query is OK but the example can be wrong I can extend the code or write more comment too.

It is a good point made by @intrigus-lgtm. As you see in the similar query for JS it can be decisive factor if the decoded payload is used or not. I think you should update your tests and query to raise the alert if it is used and do not raise if it is discarded.

BTW, I think your query already discards the alert if the result of decode is not used, but it would be good to confirm it with tests.

the only idea that I have is to add another global taint module and use that to reach from the current sink JWT.decode(sink) to item.getClaim("sth")
if you agree with this and you think having a third global taint module is not a problem I'll implement this too.
please note the third global taint module can be a small path so maybe there are no performance issues against it.

I don't think you need changes to the existing codeql query. Have you tried just adding two tests: one that doesn't do anything with the result of decodeToken (should not alert) and another one that calls getClaim for example?

I think we can use flowState in one of the two taint configurations of my query and set the sink to getClaim instead of JWT.decode I'll test it.

About tests i'll check more tests first.

JarLob · 2023-11-06T20:36:42Z

We have already https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql (Which doesn't detect the CVE), but I'll leave it to the CodeQL team to decide if it is better to merge in this PR or later.

…ntations for ql classes

am0o0 · 2023-11-22T08:28:46Z

@JarLob except for the tests that I don't know how to create the tests in java, I think everything is good now.

JarLob · 2024-05-13T15:45:54Z

@am0o0 Could you please mark this pull request as ready for review? Have you looked already how tests for java are made in https://github.com/github/codeql/tree/main/java/ql/test/query-tests/security/CWE-347?

am0o0 · 2024-05-13T16:27:31Z

@JarLob Yes I tried to learn to create test cases by comparing them to a similar part of current query tests but I failed, I'll try again.

JarLob · 2024-06-26T21:08:42Z

@am0o0 We are sunsetting our codeql bug bounty program. To be eligible for the reward please work on the pull request ASAP.

am0o0 · 2024-07-01T16:38:09Z

@JarLob I'm learning how to create stubs for Java, otherwise the tests are the same as the example file, so when I finish the work of creating the test for the other java PR I'll quickly create a test case here.

am0o0 · 2024-07-13T16:02:13Z

@JarLob @owen-mc this PR is ready for review as I finalized the tests.

smowton · 2024-07-18T08:44:03Z

Do you need anything particular to servlet-api-4.0.1, or can you just use java/ql/test/stubs/servlet-api-2.4 instead of introducing new stubs?

am0o0 · 2024-07-18T19:02:22Z

I tried it just now, and it failed:

codeql test run Auth0NoVerifier.qlref --show-extractor-output
Executing 1 tests in 1 directories.
Extracting test database in /home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347.
[2024-07-18 20:59:59] [build-err] Exception in thread "main" Compilation errors were reported by javac.
[2024-07-18 20:59:59] [build-err]       at com.semmle.extractor.java.JavaExtractor.main(JavaExtractor.java:810)
[2024-07-18 20:59:59] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/home/am/CodeQL-home/java/tools/linux/jdk-extractor-java/bin/java, -cp, /home/am/CodeQL-home/java/tools/semmle-extractor-java.jar:/home/am/CodeQL-home/java/tools/lombok-javac-extend.jar, -Xmx500m, com.semmle.extractor.java.JavaExtractor, --strict-javac-errors, --javac-args, -source, 8, -target, 8, -cp, /home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347/../../../stubs/auth0-java-jwt-4.4.0:/home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347/../../../../../test/stubs/servlet-api-2.4/, -encoding, UTF-8, JwtNoVerifier.java])
Could not extract a dataset in /home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347: Extraction command /home/am/CodeQL-home/java/tools/linux/jdk-extractor-java/bin/java failed with status 1.
Extraction command /home/am/CodeQL-home/java/tools/linux/jdk-extractor-java/bin/java failed with status 1.
[1/1] FAILED(EXTRACTION) /home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347/Auth0NoVerifier.qlref
Compiling queries in /home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347.
Completed in 3.8s (extract 2.2s comp 0ms eval 0ms).
0 tests passed; 1 tests failed:
  FAILED: /home/am/CodeQL-home/codeql-repo-amammad/java/ql/test/experimental/query-tests/security/CWE-347/Auth0NoVerifier.qlref

owen-mc · 2024-07-18T20:31:49Z

@am0o0 Can you be more specific about what you tried?

am0o0 · 2024-07-18T20:37:32Z

I simply replaced my servlet api stub with "java/ql/test/stubs/servlet-api-2.4" in the options file, I double-checked that the new path of the stub was correct too.

am0o0 · 2024-07-18T20:39:04Z

it is a very old version i dont expect to work honestly. Also we can think in this way that we have the stubs for new version now.😅

smowton · 2024-07-19T10:08:58Z

You can see what the extractions are in CWE-347/CWE-347.testproj/log/javac-output-*.log -- in this case it was simply that WebServlet is new, and getWriter throws IOException in Servlet 2.5. I have taken the liberty of creating #17020 to fix that and polish this PR.

smowton · 2024-07-19T10:33:33Z

So, looking at this I suggest this could be simplified:

All you're trying to do is notice when the result of decode flows to getClaim or similar ("get-payload"), but does NOT flow to a verify method.

To do this you only need one dataflow config, and you don't need flow states.

Suggest simply:

Source = result of the decode method
Sink = either a verify or a get-payload method
No additional flow steps needed

Then the query result is simply those (source, sink) pairs where the sink is a get-payload method and there does not exist a (source2, sink2) such that sink2 is a verify method and source.getNode = source2.getNode.

If it should turn out there are false positives due to decoding JWTs that don't come from a remote source, then flow-state can be used to track RemoteFlowSource -> decode -> verify/get-payload -- in that case decode is an additional flow step, but you still don't need flow-state because the type system guarantees that if we're verifying something from a remote flow source, it must have been decoded at some point.

Finally I would remove the flow sources that seem to regard any string-typed variable initializer as a source. In that case you might as well call any string at all a source (what's special about initializing a variable?), and in that case you might as well just start tracking flow from the decode method instead, since clearly it got a string input from somewhere.

smowton · 2024-07-19T11:00:59Z

Other simple things to fix:

MethodAccess -> MethodCall

DataFlow::FlowState -> string (but as mentioned above this should be removed anyway)

Change the query id and description to reflect this being an auth0-specific query (and not clash with the non-experimental query that also checks for JWT mistakes)

github-actions · 2024-07-30T11:20:18Z

QHelp previews:

java/ql/src/experimental/Security/CWE/CWE-347/Auth0NoVerifier.qhelp

Missing JWT signature check

A JSON Web Token (JWT) is used for authenticating and managing users in an application. It must be verified in order to ensure the JWT is genuine.

Recommendation

Don't use information from a JWT without verifying that JWT.

Example

The following example illustrates secure and insecure use of the Auth0 `java-jwt` library.

package com.example.JwtTest;

import java.io.*;
import java.security.NoSuchAlgorithmException;
import java.util.Objects;
import java.util.Optional;
import javax.crypto.KeyGenerator;
import javax.servlet.http.*;
import javax.servlet.annotation.*;
import com.auth0.jwt.JWT;
import com.auth0.jwt.JWTVerifier;
import com.auth0.jwt.algorithms.Algorithm;
import com.auth0.jwt.exceptions.JWTCreationException;
import com.auth0.jwt.exceptions.JWTVerificationException;
import com.auth0.jwt.interfaces.DecodedJWT;

@WebServlet(name = "JwtTest1", value = "/Auth")
public class auth0 extends HttpServlet {

  public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {
    response.setContentType("text/html");
    PrintWriter out = response.getWriter();

    // OK: first decode without signature verification
    // and then verify with signature verification
    String JwtToken1 = request.getParameter("JWT1");
    String userName =  decodeToken(JwtToken1);
    verifyToken(JwtToken1, "A Securely generated Key");
    if (Objects.equals(userName, "Admin")) {
      out.println("<html><body>");
      out.println("<h1>" + "heyyy Admin" + "</h1>");
      out.println("</body></html>");
    }

    out.println("<html><body>");
    out.println("<h1>" + "heyyy Nobody" + "</h1>");
    out.println("</body></html>");
  }

  public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
    response.setContentType("text/html");
    PrintWriter out = response.getWriter();

    // NOT OK:  only decode, no verification
    String JwtToken2 = request.getParameter("JWT2");
    String userName = decodeToken(JwtToken2);
    if (Objects.equals(userName, "Admin")) {
      out.println("<html><body>");
      out.println("<h1>" + "heyyy Admin" + "</h1>");
      out.println("</body></html>");
    }

    // OK:  no clue of the use of unsafe decoded JWT return value
    JwtToken2 = request.getParameter("JWT2");
    JWT.decode(JwtToken2);


    out.println("<html><body>");
    out.println("<h1>" + "heyyy Nobody" + "</h1>");
    out.println("</body></html>");
  }

  public static boolean verifyToken(final String token, final String key) {
    try {
      JWTVerifier verifier = JWT.require(Algorithm.HMAC256(key)).build();
      verifier.verify(token);
      return true;
    } catch (JWTVerificationException e) {
      System.out.printf("jwt decode fail, token: %s", e);
    }
    return false;
  }


  public static String decodeToken(final String token) {
    DecodedJWT jwt = JWT.decode(token);
    return Optional.of(jwt).map(item -> item.getClaim("userName").asString()).orElse("");
  }

}

References

The incorrect use of JWT in ShenyuAdminBootstrap allows an attacker to bypass authentication.
Common Weakness Enumeration: CWE-347.

JarLob

The query doesn't detect the original CVE anymore.

am0o0 · 2024-07-30T16:31:42Z

@JarLob Could you please upload the DB, I lost it and I can't create it again because of some errors :)

…021-37580

…to amammad-java-JWT

am0o0 · 2024-07-31T09:05:37Z

Hi, I tried to fix my mistake with minimum changes, you can check the changes with commits, I tried to have a good commit history for this to make the review easier.

am0o0 · 2024-07-31T09:07:00Z

@JarLob the CVE can be detected with the local source version of the final query now. Also I managed to create the DB.

JarLob · 2024-07-31T12:03:08Z

Hi, it still doesn't work. Here is the database https://we.tl/t-WOpOJykqkl

…

On Wed, Jul 31, 2024 at 11:07 AM Am ***@***.***> wrote: [image: am0o0]*am0o0* left a comment (github/codeql#14089) <#14089 (comment)> @JarLob <https://github.com/JarLob> the CVE can be detected with the local source version of the final query now. Also I managed to create the DB. — Reply to this email directly, view it on GitHub <#14089 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGLK53EU4HOXZICURTKYW3DZPCSMZAVCNFSM6AAAAAA4C4SPJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQGAZDIOBQGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

am0o0 · 2024-07-31T12:10:39Z

@JarLob I think I'm wrong maybe, could you confirm that this was the sink :) ?

JarLob · 2024-07-31T12:20:39Z

Maybe my codeql branch is outdated. I'll check out the pr branch and try again.

…

On Wed, Jul 31, 2024 at 2:11 PM Am ***@***.***> wrote: [image: am0o0]*am0o0* left a comment (github/codeql#14089) <#14089 (comment)> @JarLob <https://github.com/JarLob> I think I'm wrong maybe, could you confirm that this was the sink :) ? image.png (view on web) <https://github.com/user-attachments/assets/aae8b2c1-4a33-4192-bf9d-d77a35f9a659> — Reply to this email directly, view it on GitHub <#14089 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGLK53GYOEPL5HSKBJRV2VDZPDH5PAVCNFSM6AAAAAA4C4SPJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQGM3TIMRXGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

JarLob · 2024-07-31T12:48:01Z

I have opened your branch in a codespace and got 0 results: ![image.png](https://github.com/user-attachments/assets/38461912-325b-499b-9a03-f835056a38e2) On Wed, Jul 31, 2024 at 2:20 PM Jaroslav Lobacevski ***@***.***> wrote:

…

Maybe my codeql branch is outdated. I'll check out the pr branch and try again. On Wed, Jul 31, 2024 at 2:11 PM Am ***@***.***> wrote: > [image: am0o0]*am0o0* left a comment (github/codeql#14089) > <#14089 (comment)> > > @JarLob <https://github.com/JarLob> I think I'm wrong maybe, could you > confirm that this was the sink :) ? > image.png (view on web) > <https://github.com/user-attachments/assets/aae8b2c1-4a33-4192-bf9d-d77a35f9a659> > > — > Reply to this email directly, view it on GitHub > <#14089 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AGLK53GYOEPL5HSKBJRV2VDZPDH5PAVCNFSM6AAAAAA4C4SPJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQGM3TIMRXGE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

JarLob · 2024-07-31T13:00:23Z

Ah, I missed that local.ql. I can only blame GitHub UI for cutting the file list :) It gives the result.

…

Message ID: ***@***.***> >> >

smowton · 2024-07-31T17:28:50Z

java/ql/src/experimental/Security/CWE/CWE-347/Auth0NoVerifierLocalSource.ql

+    exists(Variable v |
+      source.asExpr() = v.getInitializer() and
+      v.getType().hasName("String")
+    ) and


This is still a bad source. If your source is just "any string variable" then there is essentially no point in having a source at all. In this case just track flow starting at the decode operation.

(on the other hand is there an actual source that we don't call a RemoteFlowSource but ought to be considered dangerous in order to get the CVE hit you're looking for? If so can you say what it is?)

this is the source:
https://github.com/apache/shenyu/blob/dc841a2720ea0b5a6ca71f1b9c1aa7958d241f16/shenyu-admin/src/main/java/org/apache/shenyu/admin/shiro/config/ShiroRealm.java#L75

I think we can set this authenticationToken.getCredentials() as a remote flow source since the Shiro is a package and it can be imported into other applications I guess. so it can be utilized in other projects too and this remote flow source is not specific to only this source code.

I think you should just consider any JWT token potentially interesting, since they're inherently things used in a security-sensitive context. I would recommend making the result of the decode method your source, then just measuring flow from there to either verification or to information access, rather than worry about the ultimate flow source being remote or otherwise.

I think it is not possible to use the decoding result as the source because from the result we don't have a flow to a JWT verify method, we should have the original JWT( here is the token variable) as the source and check if this token has a flow to a decoding method and doesn't have a flow to verify method.

Ah yes I see. I'm not familiar enough with Shiro to know if doGetAuthenticationInfo is a reasonable method to assume is generally called with an untrusted parameter. If so, I would be fine with adding as a source, any method implementing AuthenticatingRealm.doGetAuthenticationInfo (note Authenticating not Authorizing; Authenticating defines the interface and Authorizing inherits it). Use Method.overrides+ to find implementations, like

exists(Method base, Method override | base.hasQualifiedName("package", "Class", "methodName") and override.overrides+(base))

Oh or better yet, add it as an experimental models-as-data source, like you did in #16708 but defining a sourceModel not a sinkModel.

I think this method can be the source: https://github.com/apache/shenyu/blob/dc841a2720ea0b5a6ca71f1b9c1aa7958d241f16/shenyu-admin/src/main/java/org/apache/shenyu/admin/shiro/config/ShiroRealm.java#L75

authenticationToken.getCredentials();

about the AuthenticationToken interface, the document defines this interface as follows (link):

An AuthenticationToken is a consolidation of an account's principals and supporting credentials submitted by a user during an authentication attempt.

I think it is fine to add some methods of this interface as remote sources.

Sounds good to me-- if you go ahead and add them as sources, we'll be able to get this PR merged

smowton · 2024-08-19T10:40:49Z

@JarLob are you able to confirm this latest draft is satisfactory from your point of view?

JarLob · 2024-08-19T15:03:40Z

Yes, it is.

owen-mc · 2024-08-20T10:59:35Z

java/ql/test/experimental/query-tests/security/CWE-347/Auth0NoVerifier.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-347/Auth0NoVerifier.ql


Suggested change

experimental/Security/CWE/CWE-347/Auth0NoVerifier.ql

query: experimental/Security/CWE/CWE-347/Auth0NoVerifier.ql

postprocess: TestUtilities/PrettyPrintModels.ql

Try making this change and re-generating the test results. It should remove the absolute model numbers, which is what's causing tests to fail at the moment.

V1

664890a

github-actions bot added documentation Java labels Aug 29, 2023

am0o0 mentioned this pull request Aug 29, 2023

[Java]: JWT decoding without verification github/securitylab#783

Closed

2 tasks

intrigus-lgtm reviewed Aug 29, 2023

View reviewed changes

update example to include more logical vulnerable pattern, add docume…

97eb7b7

…ntations for ql classes

stash

0d0dc51

am0o0 marked this pull request as ready for review May 13, 2024 16:26

am0o0 requested a review from a team as a code owner May 13, 2024 16:26

finilize tests

71e1d63

remove redundent stubs

bf506f8

smowton mentioned this pull request Jul 19, 2024

Java: JWT decoding without verification [smowton fork] #17020

Closed

smowton and others added 3 commits July 28, 2024 10:26

Make test compatible with Servlet 2.5; use old Servlet stubs

142d7ae

Adjust test expectations

e3559d8

comply with PascalCase/camelCase, remove redundant import

14cf47b

Copyedit documentation

a781522

smowton previously approved these changes Jul 30, 2024

View reviewed changes

Fix link

8f52b2c

smowton dismissed their stale review via 8f52b2c July 30, 2024 11:23

JarLob suggested changes Jul 30, 2024

View reviewed changes

am0o0 added 3 commits July 31, 2024 10:58

add same query but with local source support to comply with the CVE-2…

701e3d7

…021-37580

merge duplicate module into a module file

c6814fc

Merge branch 'amammad-java-JWT' of https://github.com/am0o0/codeql in…

9110df6

…to amammad-java-JWT

fix formatting

d560c1e

smowton reviewed Jul 31, 2024

View reviewed changes

am0o0 added 2 commits August 16, 2024 15:41

add getCredentials method of AuthenticationToken as a remote source

d88b310

update tests to contain the new source, delete query with local sources

f476437

smowton previously approved these changes Aug 19, 2024

View reviewed changes

owen-mc reviewed Aug 20, 2024

View reviewed changes

am0o0 dismissed smowton’s stale review via b001c24 August 20, 2024 18:57

update tests to pass the github actions

b001c24

owen-mc approved these changes Aug 20, 2024

View reviewed changes

smowton merged commit 15989ce into github:main Aug 21, 2024
10 checks passed

am0o0 deleted the amammad-java-JWT branch September 14, 2024 11:13

		@@ -0,0 +1 @@
		experimental/Security/CWE/CWE-347/Auth0NoVerifier.ql

	experimental/Security/CWE/CWE-347/Auth0NoVerifier.ql
	query: experimental/Security/CWE/CWE-347/Auth0NoVerifier.ql
	postprocess: TestUtilities/PrettyPrintModels.ql

Java: JWT decoding without verification #14089

Java: JWT decoding without verification #14089

Uh oh!

Conversation

am0o0 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JarLob commented Nov 6, 2023

Uh oh!

am0o0 commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JarLob commented May 13, 2024

Uh oh!

am0o0 commented May 13, 2024

Uh oh!

JarLob commented Jun 26, 2024

Uh oh!

am0o0 commented Jul 1, 2024

Uh oh!

am0o0 commented Jul 13, 2024

Uh oh!

smowton commented Jul 18, 2024

Uh oh!

am0o0 commented Jul 18, 2024

Uh oh!

owen-mc commented Jul 18, 2024

Uh oh!

am0o0 commented Jul 18, 2024

Uh oh!

am0o0 commented Jul 18, 2024

Uh oh!

smowton commented Jul 19, 2024

Uh oh!

smowton commented Jul 19, 2024

Uh oh!

smowton commented Jul 19, 2024

Uh oh!

github-actions bot commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Missing JWT signature check

Recommendation

Example

References

Uh oh!

JarLob left a comment

Choose a reason for hiding this comment

Uh oh!

am0o0 commented Jul 30, 2024

Uh oh!

am0o0 commented Jul 31, 2024

Uh oh!

am0o0 commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JarLob commented Jul 31, 2024 via email

Uh oh!

am0o0 commented Jul 31, 2024

Uh oh!

JarLob commented Jul 31, 2024 via email

Uh oh!

JarLob commented Jul 31, 2024 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

am0o0 commented Aug 29, 2023 •

edited

Loading

am0o0 commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Jul 30, 2024 •

edited

Loading

am0o0 commented Jul 31, 2024 •

edited

Loading

JarLob commented Jul 31, 2024 via email •

edited

Loading