Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swift: Unsafe JS Eval Query #11001

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Swift: Unsafe JS Eval Query #11001

wants to merge 3 commits into from

Conversation

d10c
Copy link
Contributor

@d10c d10c commented Oct 26, 2022

No description provided.

d10c added 2 commits Oct 21, 2022
Also allow clangd to use the generated `swift/compile_commands.json`
for intellisense in the codeql repository, via `.vscode/settings.json`.

The `-arch=x86_64` from `swift/rules.bzl` turns out to be unnecessary,
even on Arm-based Macs.
@github-actions github-actions bot added the Swift label Oct 26, 2022
"--header-insertion=never",
"--compile-commands-dir=${workspaceFolder}/swift",
"--query-driver=**"
]
Copy link
Contributor

@geoffw0 geoffw0 Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not all that familiar with this stuff (the first commit of this PR).


/**
* A source of untrusted, user-controlled data.
* TODO: Extend to more (non-remote) sources in the future.
Copy link
Contributor

@geoffw0 geoffw0 Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think this is a good example of a query where I would be nervous even of local user input being evaluated.

Copy link
Contributor

@geoffw0 geoffw0 Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created github/codeql-c-team#1318 to address this as follow-up.

ce.getStaticTarget() = getFunctionWithQualifiedName("JSEvaluateScript(_:_:_:_:_:_:)")
).getArgument(1).getExpr() = this.asExpr()
}
}
Copy link
Contributor

@geoffw0 geoffw0 Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great coverage with all of these sinks!

// - parentDecl = memberDecl.getDeclaringDecl() <=> parentDecl.getAMember() = memberDecl
IterableDeclContext getDeclaringDeclOf(Decl member) { result.getAMember() = member }

MethodDecl getMethodWithQualifiedName(string className, string methodName) {
Copy link
Contributor

@geoffw0 geoffw0 Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of #10996 (which was opened after you made this PR) we have MethodDecl.getQualifiedName for this, and #11034 adds AbstractFunctionDecl.hasGlobalName that can be used instead of the predicate below. Feel free to either update your QL, or leave it to me as part of the follow-up to my work.

Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants