Eas 1965 #28

aliner-wang · 2023-11-06T18:01:40Z

Added new pathway for scanning for packages requiring secrets to include * imported packages.

Updated secretdata.yaml with new packages to scan for. Commented out packages use require import method that we don't scan for.

Added package import to DVFA.

Todo: definitely can clean up code from comments and stuff in DVFA and around main. But as_intrinsic in definition.rs is done and cleaned up.

Co-authored-by: Joshua Wong <[email protected]>

…for secret scanner

…ar method

…cts. And deleted excess debug statements

…VFA with test cases

jwong101 · 2023-11-07T17:08:13Z

secretdata.yaml

+
+- package_name: jsonwebtoken
+  identifier: default
+  function_name: Some


shouldn't this be sign?

crates/forge_analyzer/src/definitions.rs

jwong101 · 2023-11-07T17:24:56Z

crates/forge_analyzer/src/definitions.rs

        match *callee {
            [PropPath::Unknown((ref name, ..))] if *name == *"fetch" => Some(Intrinsic::Fetch),
+            // [PropPath::Def(def)] => {}]


Don't forget to handle the case where it's just a "regular" function call, i.e. with just an identifier. Otherwise, we wouldn't catch this:

import { sign } from 'jsonwebtoken'; sign('foo', 'secret');

How would you handle a case where sign is not from jsonwebtoken? Like in an instance where a function is names sign but not necessarily imported from jsonwebtoken or it could be defined locally? I think the only way to verify for sure is to check for the specific import of the package. Would calling self.res.is_imported_from be enough to verify?

Yeah, that would return false if it's not an import or if it's imported from a different project.

jwong101 · 2023-11-07T17:27:01Z

test-apps/jira-damn-vulnerable-forge-app/src/utils.js

+
+  // testing all the libraries
+  // var token = jwt.sign({ foo: 'bar' }, 'peek a boo');
+  // var hmac = HmacMD5('Secret Message', 'HMAC PASSWORD');


Can you add test cases for named imports as well? Also for cases where an identifier could shadow an import to see if we don't raise a false positive there.

…t exports. added new scanning pathway for star imports where prop path is identifier or method with children object imports.

…ed packages. Working on case to catch functions called from packages in nameed imports

…ases and add more tests

jwong101 · 2023-11-10T00:21:15Z

crates/forge_analyzer/src/definitions.rs

+                    false
+                }
+            }
+            _ => false,


You shouldn't need this catchall case.

jwong101 · 2023-11-10T00:31:00Z

crates/forge_analyzer/src/definitions.rs

@@ -2981,11 +3503,42 @@ impl Environment {
            DefKind::Undefined => DefKind::Undefined,
        }
    }
+    pub fn is_import(&self, import_kind: ImportKind, named_import: Option<String>) -> bool {


This should probably just be a method an ImportKind, since we don't use the resolver here. Also, this could probably just take in a &str and check if the import is the same identifier as what was passed in.

jwong101 · 2023-11-10T00:31:50Z

crates/forge_analyzer/src/definitions.rs

+        // debug!("{named_import}");
+        match import_kind {
+            ImportKind::Star => true,
+            ImportKind::Default => true,


This should be true iff named_import is "default", since this function is for checking if the import matches whatever identifier/string we pass in.

…dded more test cases for named imports. todo: add more edge cases and review is_import

…aml. Todo: resolve edge case where same secret scanner vuln got reported twice - after adding jsonwebtoken funtion verify

…s passes in utils.js.

jwong101

Looks good overall. Mostly stylistic changes asides from moving the match arms.

jwong101 · 2023-11-28T06:50:38Z

.gitignore

@@ -1 +1,2 @@
 /target
+/test-apps


Can you remove this from the .gitignore?

jwong101 · 2023-11-29T17:13:28Z

crates/forge_analyzer/src/definitions.rs

-            _ => false,
-        }
-    }
+    // pub fn is_named_or(&self, import_kind: Option<String>) -> bool {


Can you remove from this if it's unused?

jwong101 · 2023-11-29T17:21:37Z

crates/forge_analyzer/src/definitions.rs

+                // case where function requires object to be invoked first
+                // example: import * as cryptoJS from 'crypto-js';
+                // var aes = cryptoJS.AES.encrypt('Secret message', 'secret password');
+                if let Some((package_name, import_kind)) = self.res.as_foreign_import(def) {


Suggested change

if let Some((package_name, import_kind)) = self.res.as_foreign_import(def) {

let (package_name, import_kind) = self.res.as_foreign_import(def)?;

See this for an explanation of how this works.

jwong101 · 2023-11-29T17:24:41Z

crates/forge_analyzer/src/definitions.rs

+
+                None
+            }
+
            [PropPath::Def(def), PropPath::Static(ref s), ..] if is_storage_read(s) => {


Can you move these two cases above the cases for lowering the secret function intrinsics?

ran into a problem with this one. Moving the last case above all the secret scanner checks led to the secret scanner checks to never get caught.

jwong101 · 2023-11-29T17:26:31Z

crates/forge_analyzer/src/definitions.rs

+                        // Aes.encrypt(blah, blah);
+                        let package_found = self.secret_packages.iter().find(|&package_data| {
+                            if let Some(method) = &package_data.method {
+                                return package_name == package_data.package_name


Suggested change

return package_name == package_data.package_name

package_name == package_data.package_name

Of course don't forget to remove the semicolon.

jwong101 · 2023-11-29T19:35:19Z

crates/forge_analyzer/src/definitions.rs

@@ -1077,7 +1322,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
        match n {
            Pat::Ident(BindingIdent { id, .. }) => {
                let id = id.to_id();
-                let def = self.res.get_or_insert_sym(id, self.module);
+                let def = self.res.get_or_insert_sym(id.clone(), self.module);


Suggested change

let def = self.res.get_or_insert_sym(id.clone(), self.module);

let def = self.res.get_or_insert_sym(id, self.module);

jwong101 · 2023-11-29T19:37:40Z

crates/forge_analyzer/src/definitions.rs

                                _ => {}
                            }
                        }
                    }
                }
-            } else if &ident.sym == "exports" {
+            } else if ident.sym.to_string() == "exports" {


Suggested change

} else if ident.sym.to_string() == "exports" {

} else if ident.sym == "exports" {

jwong101 · 2023-11-29T19:44:01Z

crates/forge_analyzer/src/definitions.rs

+            // adding the default export, so we can resolve it during the lowering
+            self.res_table
+                .exported_names
+                .insert((ident.sym.clone(), self.curr_mod), self.default.unwrap());


Could you change the ExportCollector::set_default method to return the DefId that was set to self.default at the end of that function?; That way you can do let default_id = self.add_default(DefRes::Undefined, None); and replace self.default.unwrap() with default_id.

There is no set_default function.

Tldr on my efforts: For this one, I couldn't find set_default under ExportCollector. Nor did a default_id variable exist.

So I tried to return defid in add_default. But that also messes up some self.add_default(DefRes::Undefined, None); implementations elsewhere because it's expected that nothing gets returned.

I did try creating self.set_default, but it seemed a little repetitive to call it if add_default is already setting self.default. and calling set_default to return a defId add_default is pretty much the same result as the approach described above.

Otherwise, the easiest way to not use .unwrap() might be to use a let statement grab the value in a Some() object instead of returning it.

Do you want me to break a lil and adapt like other places where self.add_default is called?

Ah, I meant change add_default to return a DefId. Yeah, that should be fine, since looking at the breakage, it's just match arms that are now returning a DefId instead of (). To fix it, you just need to wrap the arm in braces and add a semicolon.

jwong101 · 2023-11-29T19:44:28Z

crates/forge_analyzer/src/definitions.rs

+                                    // adding the default export, so we can resolve it during the lowering
+                                    self.res_table.exported_names.insert(
+                                        (ident.sym.clone(), self.curr_mod),
+                                        self.default.unwrap(),


Could you change the ExportCollector::set_default method to return the DefId that was set to self.default at the end of that function?; That way you can do let default_id = self.add_default(DefRes::Undefined, None); and replace self.default.unwrap() with default_id.

jwong101 · 2023-11-29T19:45:03Z

crates/forge_analyzer/src/definitions.rs

                if let Some(mem_expr) = mem_expr_from_assign(n) {
                    if let MemberProp::Ident(ident_property) = &mem_expr.prop {
-                        if &ident_property.sym == "exports" {
+                        if ident_property.sym.to_string() == "exports" {


Suggested change

if ident_property.sym.to_string() == "exports" {

if ident_property.sym == "exports" {

aliner-wang · 2023-11-29T20:51:29Z

crates/forge_analyzer/src/definitions.rs

+            [PropPath::Def(def), ..] => match self.res.is_imported_from(def, "@forge/api") {
+                Some(ImportKind::Named(ref name)) if *name == *"authorize" => {
+                    Some(Intrinsic::Authorize(IntrinsicName::Other))
+                }
+                _ => None,
+            },


Specifically moving this case above the secret scanner checks I think catches all statements before secret scanner checks.

Ah, mb I thought that match was a guard statement for some reason. If you change it to a guard statement with if self.res.is_imported_from(def, "@forge/api") == *"authorize" => Some(Intrinsic::Authorize(IntrinsicName::Other)),, then moving it above shouldn't swallow the secret scanner checks.

…match statement logic to now swallow rest of secret checker

…a DefId

Eas 1965

gersbach and others added 30 commits June 16, 2023 10:30

basic checker

1373a02

moving inst checking to the transfer block

23d9e13

removing bad variable names

103eb3f

wip

7c16809

definition analysis

654978d

wip

cc084a1

removing launch.json

470882b

updates to parsing worklist arguments

2a7b121

correclty resolving permissions

75fccc4

cleaning up examples

82eaccd

fix bug with adding arguments

80b0425

fix to the worklist

38edf85

converting defid to varid

2fc49f1

various updates

d574af0

merge

2c6be3e

wip

c32f369

Update crates/forge_analyzer/src/checkers.rs

ba9245e

Co-authored-by: Joshua Wong <[email protected]>

wip

faef25d

resolve merge conflicts

4197eaf

wip

7603453

wip

f69b55f

fix to not reading the second argument

cb6999f

wip

a93f8e7

fix to unlimited looping beteween visit blocks

e8a5e3f

fix to unlimited looping beteween visit blocks

4d8e3e4

cleaning up

b6329a1

wip

0a501c9

fix to the issue of not resolving default exports

f3dc413

removing unneeded comments

8d3c42e

small updates

1c5fed9

gersbach and others added 10 commits October 16, 2023 16:11

cleaning up codebase

093d8ac

refractoring to a value manager

1d3d5b2

resolving merge conflicts

6e51d52

resolving merge conflicts with main branch

0428400

formatting

5abcd6f

chore: added additional known common package functions in forge apps …

86f625b

…for secret scanner

feat: added scanning in secret scanner for packages imported using st…

fdfb0c4

…ar method

chore: removed reptitive return statement

30acd97

chore: removed repetitive code and simplified returned Intrinsic obje…

43c36b6

…cts. And deleted excess debug statements

feat: added crypto-js to secretdata.yaml scanning list. And updated D…

a388faf

…VFA with test cases

jwong101 reviewed Nov 7, 2023

View reviewed changes

awang7 added 3 commits November 7, 2023 22:42

feat: updated secretdata.yaml with new flag identifier to catch objec…

2759a2d

…t exports. added new scanning pathway for star imports where prop path is identifier or method with children object imports.

feat: added feature to catch secrets in functions from default import…

812adcb

…ed packages. Working on case to catch functions called from packages in nameed imports

feat: added secret scanning for Named imports. TODO: check for edge c…

2e8d8c7

…ases and add more tests

jwong101 reviewed Nov 10, 2023

View reviewed changes

awang7 and others added 6 commits November 20, 2023 17:04

fixed: revised fix_import to check for default and named functions. A…

69305ca

…dded more test cases for named imports. todo: add more edge cases and review is_import

chore: cleaned up test cases in util.js. Added verify to secretdata.y…

2a5ac9d

…aml. Todo: resolve edge case where same secret scanner vuln got reported twice - after adding jsonwebtoken funtion verify

Merge branch 'main' into EAS-1965

edce2a4

fix: swc compile errors

cf17a53

refactor: rebuilt as_intrinsic to be sleaker and more organized. Test…

af95baf

…s passes in utils.js.

chore: deleted is_import implementation

88da4d6

jwong101 requested changes Nov 29, 2023

View reviewed changes

fix: resolving PR comments. Pushing fixes to see which comments are left

c07f919

aliner-wang commented Nov 29, 2023

View reviewed changes

awang7 and others added 4 commits November 29, 2023 17:23

fix: addressed more PR comments. updated let statements, and rewrote …

f3dea9f

…match statement logic to now swallow rest of secret checker

chore: not all changes went through the first time

891cb08

fix: removed the unwrap() on self.default and made add_defult return …

35b605c

…a DefId

fix: clean up some code

2466d71

jwong101 merged commit b2d0f9d into atlassian-labs:main Nov 30, 2023
1 check passed

aliner-wang pushed a commit to aliner-wang/FSRT that referenced this pull request Jan 11, 2024

Merge pull request atlassian-labs#28 from aliner-wang/EAS-1965

f03e238

Eas 1965

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eas 1965 #28

Eas 1965 #28

aliner-wang commented Nov 6, 2023

jwong101 Nov 7, 2023

jwong101 Nov 7, 2023

aliner-wang Nov 7, 2023 •

edited

Loading

jwong101 Nov 10, 2023

jwong101 Nov 7, 2023

jwong101 Nov 10, 2023

jwong101 Nov 10, 2023

jwong101 Nov 10, 2023

jwong101 left a comment

jwong101 Nov 28, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

aliner-wang Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

aliner-wang Nov 29, 2023

aliner-wang Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

jwong101 Nov 29, 2023

aliner-wang Nov 29, 2023

jwong101 Nov 29, 2023

	if let Some((package_name, import_kind)) = self.res.as_foreign_import(def) {
	let (package_name, import_kind) = self.res.as_foreign_import(def)?;

	return package_name == package_data.package_name
	package_name == package_data.package_name

	let def = self.res.get_or_insert_sym(id.clone(), self.module);
	let def = self.res.get_or_insert_sym(id, self.module);

	} else if ident.sym.to_string() == "exports" {
	} else if ident.sym == "exports" {

	if ident_property.sym.to_string() == "exports" {
	if ident_property.sym == "exports" {

Eas 1965 #28

Eas 1965 #28

Conversation

aliner-wang commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliner-wang Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwong101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliner-wang Nov 7, 2023 •

edited

Loading