JS: Extends CredentialsNode class mostly related to JWT authentication packages #14666

am0o0 · 2023-11-02T15:12:39Z

No description provided.

… results now, ConstantString is suggested as a better alternative for finding constant sources

add new additional global taint and dataflow steps update tests of CWE-798 add a new sanitizer for `semmle.javascript.security.dataflow.HardcodedCredentialsQuery`

…dentials

This reverts commit bdee99a.

p- · 2024-05-29T15:23:04Z

javascript/ql/lib/semmle/javascript/security/dataflow/HardcodedCredentialsCustomizations.qll

+      this.getFile()
+          .getLocation()
+          .hasLocationInfo(any(string s |
+              s.regexpMatch(["/.*test[.].*", "/.*demo[.].*", "/.*example[.].*", "/.*sample[.].*"])


I'm not sure if this should be added as a sanitizer on this level.

please tell me where I should put this sanitizer because I encountered a lot of FPs before applying it so I must use it but don't know exactly where.

Let's wait for the CodeQL team to review it. But I fear if it adds a lot of FP's without this sanitizer it might not be perfectly suited as addition for this prod query.
On the other hand: excluding test files for less FP's might help to reduce FP's for the hardcoded-credentials query as a whole.

Two things.

This shouldn't be added as a sanitizer, instead you should probably add a condition in the where statement in the .ql file.

Use the isTestFile predicate from ClassifyFiles.qll instead.

The performance is this current sanitizer is terrible on some large projects, so this definitely cannot be merged as is.

erik-krogh

The idea seems OK.
I haven't looked through the models yet, but I did run a small internal evaluation on them, and that looked OK.

But first there is a problem with the sanitizer.

I'll take a closer look at the models once that's resolved.

erik-krogh · 2024-06-04T11:54:28Z

javascript/ql/lib/semmle/javascript/security/dataflow/HardcodedCredentialsCustomizations.qll

+      this.getFile()
+          .getLocation()
+          .hasLocationInfo(any(string s |
+              s.regexpMatch(["/.*test[.].*", "/.*demo[.].*", "/.*example[.].*", "/.*sample[.].*"])


Two things.

This shouldn't be added as a sanitizer, instead you should probably add a condition in the where statement in the .ql file.

Use the isTestFile predicate from ClassifyFiles.qll instead.

The performance is this current sanitizer is terrible on some large projects, so this definitely cannot be merged as is.

use a simpler where condition(the former sanitizer) for overcoming performance problems

am0o0 · 2024-06-06T12:32:09Z

I only included the new where condition on sinks because the constant hardcoded creds can be loaded from a test or example directory. However, the sink using these hardcoded creds should not be in a test or example directory.

erik-krogh · 2024-06-06T18:13:43Z

I only included the new where condition on sinks because the constant hardcoded creds can be loaded from a test or example directory. However, the sink using these hardcoded creds should not be in a test or example directory.

Can you try to use ClassifyFiles.qll instead? I know those heuristics probably don't match exactly what you're looking for, but those can be improved later.
(I very much suggest that you don't look into that now, but do that in a later PR if needed, just to keep the scope of this PR low).

If you don't want to do that, then an option is to move your contributions into the experimental folder.

Note: The performance issue I mentioned previously is definitely gone with the filter in the where part of the query.

…condition, update tests accordingly

am0o0 · 2024-06-25T06:08:44Z

@erik-krogh if the changes are not good and it is better to move my changes to experimental please let me know.

erik-krogh

@erik-krogh if the changes are not good and it is better to move my changes to experimental please let me know.

No, I think that is fine to get it in the standard suite.
But I did discuss internally, and agreed that you have some changes to make (beyond the comments below).

Somehow the isTestFile filter should only apply to the JWT keys you've added.
Probably by creating a new kind for getCredentialsKind, and then applying the filter only for that credentials-kind.

javascript/ql/lib/semmle/javascript/frameworks/JWT.qll

…steps with test cases, update test cases and expected test results

… query taint step

since we want to check if a jwt related sink is in this dir or not

… from with the isTestFile predicate. According to expected test results, with a new query, the jwt sinks of __test__/ dir have been exluded from query results.

javascript/ql/src/Security/CWE-798/HardcodedCredentials.ql

erik-krogh · 2024-07-11T08:24:30Z

javascript/ql/src/Security/CWE-798/HardcodedCredentials.ql

+    not isTestFile(sink.getNode().getFile()) and
+    updateMessageWithSourceValue(value, source.getNode(), sink.getNode())
+  else
+    // sink kind is "jwt key" and source is not constant string
+    if
+      sink.getNode().(Sink).(DefaultCredentialsSink).getKind() = "jwt key" and
+      not source.getNode().asExpr() instanceof ConstantString
+    then not isTestFile(sink.getNode().getFile()) and value = "This hard-coded value"
+    else
+      // sink kind is not "jwt key" and source is  constant string
+      if
+        not sink.getNode().(Sink).(DefaultCredentialsSink).getKind() = "jwt key" and
+        source.getNode().asExpr() instanceof ConstantString
+      then updateMessageWithSourceValue(value, source.getNode(), sink.getNode())
+      else value = "This hard-coded value"


Can you revert these changes, and then instead put the filter on the Sink class instead?

Can you give me some tips about this? I couldn't figure out a solution.

@erik-krogh Could you help me with this, I have free time this week, so I can fix the issues quicker.

I'll have a look this week. I just came back from a few weeks of vacation.

You can e.g. do it like this: erik-krogh@38e5c90
(I tried to push directly to your PR, but I can't do that).

There should probably also be a comment on the new characteristic predicate in DefaultCredentialsSink.

erik-krogh · 2024-08-05T12:04:52Z

The autoformatter checks are failing on javascript/ql/src/Security/CWE-798/HardcodedCredentials.ql and javascript/ql/lib/semmle/javascript/security/dataflow/HardcodedCredentialsCustomizations.qll.
You can use codeql query format -i <paths-to-files> to run the autoformatter.

erik-krogh · 2024-08-05T12:48:48Z

There is still javascript/ql/src/Security/CWE-798/HardcodedCredentials.ql, it's still failing the autoformatter checks.

erik-krogh

There is a single debug-line it looks like you forgot to remove.
But otherwise I think we're ready to merge 🎉

erik-krogh · 2024-08-07T07:52:21Z

javascript/ql/lib/semmle/javascript/security/dataflow/HardcodedCredentialsQuery.qll

+    |
+      src = n.getArgument(0) and
+      trg = n and
+      n.getLocation().getFile().getRelativePath().matches("%HardcodedCredentials.js%")


This line looks like some debug code you forgot to remove?

am0o0 and others added 7 commits August 29, 2023 21:23

V1

65b9774

add test cases

4f04dc8

fix qhelps

3f64cc8

change Source to ConstantString, it seems that we have some duplicate…

7a577dd

… results now, ConstantString is suggested as a better alternative for finding constant sources

add sanitizers to hardcoded query

7891e64

remove noverification query

8e0f52c

move new secret key sinks to existing CredentialsNode class,

e1d42fa

add new additional global taint and dataflow steps update tests of CWE-798 add a new sanitizer for `semmle.javascript.security.dataflow.HardcodedCredentialsQuery`

github-actions bot added JS documentation labels Nov 2, 2023

remove my Hardcoded secret key query in favor of CWE-798:HardcodedCre…

01fb29e

…dentials

github-actions bot removed the documentation label Nov 2, 2023

This was referenced Nov 2, 2023

[JS]: Decoding JWT without any signature Verification github/securitylab#784

Closed

[JS]: Signing and verifying JWT signature with a constant key github/securitylab#799

Closed

am0o0 and others added 10 commits May 13, 2024 14:37

stash

bdee99a

fix conflict

1860af0

Revert "stash"

c299b56

This reverts commit bdee99a.

update tests

20c087c

fix conflict

4e365e2

Merge branch 'main' into amammad-js-hardcodedJWTKey

2226f51

update tests

d775135

stash: add debug query

5d98ec3

fix a document

c2f96a1

remove the debug query

71dfdfa

p- reviewed May 29, 2024

View reviewed changes

ghsecuritylab marked this pull request as ready for review May 30, 2024 13:52

ghsecuritylab requested a review from a team as a code owner May 30, 2024 13:52

erik-krogh reviewed Jun 6, 2024

View reviewed changes

am0o0 added 3 commits June 6, 2024 14:10

Or to or in docs

61a11c6

remove sanitnzer and add a where condition instead

ee05ec0

use a simpler where condition(the former sanitizer) for overcoming performance problems

add tests for new where condition, update expected test results

e4ffdb8

use isTestFile from ClassifyFiles module file instead previous where …

5a69bbf

…condition, update tests accordingly

erik-krogh reviewed Jun 27, 2024

View reviewed changes

javascript/ql/lib/semmle/javascript/frameworks/JWT.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/JWT.qll Outdated Show resolved Hide resolved

javascript/ql/lib/semmle/javascript/frameworks/JWT.qll Outdated Show resolved Hide resolved

am0o0 added 6 commits July 1, 2024 11:38

move jose SharedTaintStep to a local taint step, add more additional …

65fdb8c

…steps with test cases, update test cases and expected test results

implement TextEncoderStep taint step with globalVarRef predicate

60aa711

move the TextEncoder and Buffer jose.base64url taint steps to a local…

fa8c457

… query taint step

add new default cred kind

6ecd8b7

update test cases of __tests__/ dir

5a18775

since we want to check if a jwt related sink is in this dir or not

Update hardcodedCredentials query file to only exclude 'jwt key' kind…

b360c8a

… from with the isTestFile predicate. According to expected test results, with a new query, the jwt sinks of __test__/ dir have been exluded from query results.

github-advanced-security bot found potential problems Jul 11, 2024

View reviewed changes

javascript/ql/src/Security/CWE-798/HardcodedCredentials.ql Fixed Show fixed Hide fixed

erik-krogh reviewed Jul 11, 2024

View reviewed changes

apply changes from @erik-krogh

354fcbe

apply autoformat to HardcodedCredentialsCustomizations.qll

fce183c

apply autoformating for HardcodedCredentials.ql

e4deb7d

erik-krogh added the no-change-note-required This PR does not need a change note label Aug 7, 2024

erik-krogh reviewed Aug 7, 2024

View reviewed changes

remove a part of code related to debugging :)

b64cb4d

erik-krogh approved these changes Aug 8, 2024

View reviewed changes

erik-krogh merged commit 41506fb into github:main Aug 8, 2024
13 checks passed

am0o0 deleted the amammad-js-hardcodedJWTKey branch September 14, 2024 11:13

JS: Extends CredentialsNode class mostly related to JWT authentication packages #14666

JS: Extends CredentialsNode class mostly related to JWT authentication packages #14666

Uh oh!

Conversation

am0o0 commented Nov 2, 2023

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!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am0o0 commented Jun 6, 2024

Uh oh!

erik-krogh commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am0o0 commented Jun 25, 2024

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

erik-krogh commented Aug 5, 2024

Uh oh!

erik-krogh commented Aug 5, 2024

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erik-krogh commented Jun 6, 2024 •

edited

Loading