The Wayback Machine - https://web.archive.org/web/20210301173857/https://github.com/github/codeql/pull/5083
Skip to content

/ codeql

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

Jump to bottom

Adding queries related to the Solorigate campaign #5083

Merged
merged 18 commits into from Feb 18, 2021
Merged

Adding queries related to the Solorigate campaign #5083

merged 18 commits into from Feb 18, 2021
+1,953 −0

Conversation

@raulgarciamsft
Copy link
Contributor

@raulgarciamsft raulgarciamsft commented Feb 3, 2021

No description provided.

@raulgarciamsft
Adding queries related to the Solorigate campaign
Loading status checks…
5e1e27c
@raulgarciamsft raulgarciamsft requested a review from github/codeql-csharp as a code owner Feb 3, 2021
@intrigus-lgtm
Copy link
Contributor

@intrigus-lgtm intrigus-lgtm commented Feb 3, 2021

@raulgarciamsft I think you have to auto-format the ql code (Or is it formatted? Then ignore this)
You can do that by running codeql query format --in-place on the changed files or by using the CodeQL extension for VS Code (I think it's called Format Document in VS Code).
@raulgarciamsft
Fixing incorrect file extension & adding suite
Loading status checks…
86a2aa9
@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 3, 2021

@raulgarciamsft I think you have to auto-format the ql code (Or is it formatted? Then ignore this)
You can do that by running codeql query format --in-place on the changed files or by using the CodeQL extension for VS Code (I think it's called Format Document in VS Code).

I completely forgot about that during our internal review. I will fix the formatting right now.
@raulgarciamsft
Fixed format
Loading status checks…
53ab787
@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 4, 2021

Question to the GitHub team. We are seeing a number of failures in the CodeQL task on files we did not modify. Is there anything we can/should do in this case? Thanks
@tibbes tibbes changed the base branch from master to main Feb 4, 2021
@hvitved
Copy link
Contributor

@hvitved hvitved commented Feb 4, 2021

Question to the GitHub team. We are seeing a number of failures in the CodeQL task on files we did not modify. Is there anything we can/should do in this case? Thanks

No, those errors are not caused by your changes, but instead a combination of this PR initially targeting master instead of main, and CodeScanning not being configured properly (fixed by #5086).
@hvitved hvitved added the C# label Feb 4, 2021
@sj
sj requested changes Feb 4, 2021
View changes
Copy link
Collaborator

@sj sj left a comment

Thank you very much for this, @raulgarciamsft! I've done a very cursory review with some initial comments. I''ll leave the in-depth review to the experts 😄
.../Security Features/campaign/Solorigate/ModifedFnvFunctionDetection.qhelp Outdated Show resolved Hide resolved
...tal/Security Features/campaign/Solorigate/ModifedFnvFunctionDetection.ql Outdated Show resolved Hide resolved
...experimental/Security Features/backdoor/ProcessNameToHashTaintFlow.qhelp Outdated Show resolved Hide resolved
...xperimental/Security Features/backdoor/DangerousNativeFunctionCall.qhelp Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/campaign/Solorigate-Readme.md Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/campaign/Solorigate-Readme.md Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/backdoor/PotentialTimeBomb.ql Show resolved Hide resolved
...experimental/Security Features/backdoor/ProcessNameToHashTaintFlow.qhelp Outdated Show resolved Hide resolved
@raulgarciamsft
Addressing multiple comments
Loading status checks…
979fdd2
@github-actions github-actions bot added the documentation label Feb 4, 2021
@hvitved
hvitved requested changes Feb 4, 2021
View changes
Copy link
Contributor

@hvitved hvitved left a comment

Thanks a lot for your contributions, Raul. This is a very interesting an novel use of CodeQL, at least not something I have seen before or thought about myself :-)
...ql/src/experimental/Security Features/campaign/Solorigate/Solorigate.qll Outdated Show resolved Hide resolved
.../src/experimental/Security Features/campaign/Solorigate/Solorigate.qhelp Outdated Show resolved Hide resolved
...ql/src/experimental/Security Features/campaign/Solorigate/Solorigate.qll Outdated Show resolved Hide resolved
...curity Features/campaign/Solorigate/NumberOfKnownHashesAboveThreshold.ql Show resolved Hide resolved
csharp/ql/src/microsoft/code/csharp/Cryptography/NonCryptographicHashes.qll Outdated Show resolved Hide resolved
...rc/experimental/Security Features/backdoor/ProcessNameToHashTaintFlow.ql Outdated Show resolved Hide resolved
...c/experimental/Security Features/backdoor/DangerousNativeFunctionCall.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/backdoor/PotentialTimeBomb.ql Outdated Show resolved Hide resolved
...al/Security Features/campaign/Solorigate/ModifiedFnvFunctionDetection.ql Outdated Show resolved Hide resolved
...al/Security Features/campaign/Solorigate/ModifiedFnvFunctionDetection.ql Show resolved Hide resolved
@raulgarciamsft
Updated Readme file
Loading status checks…
8e85145
@raulgarciamsft
Fixes to address some of the comments during PR
Loading status checks…
1d8f828
@raulgarciamsft
Fixing format
Loading status checks…
d5c9db4
@raulgarciamsft
Changing location for NonCryptographicHash qll
Loading status checks…
9ef4aef 
Changing the TimeBomb query to path-problem (any suggestions to improve it would be welcomed, no previous experience iwth path-problem queries)
@raulgarciamsft
Changing ProcessNameToHash query to path-problem. Any additional feed…
Loading status checks…
3dc1b81 
…back will be welcomed
@hvitved
Copy link
Contributor

@hvitved hvitved commented Feb 5, 2021

I forgot to mention it on the first review, but would it be possible to add some tests for the new queries?
@raulgarciamsft
Adding Solorigate context for the generic backdoor queries.
Loading status checks…
681e6a9
@raulgarciamsft
Fixing cutom edges predicate
Loading status checks…
d48a713
@shati-patel
shati-patel reviewed Feb 5, 2021
View changes
Copy link
Contributor

@shati-patel shati-patel left a comment

Thank you for this! I've had a look on behalf of the documentation team, to keep some of the style and terminology similar to our existing queries.

Since the "Solorigate-Readme" contains lots of helpful information, I've only suggested small additions to the .qhelp files. (As a future task, we could perhaps move more details into the .ql and .qhelp files themselves?)
...c/experimental/Security Features/backdoor/DangerousNativeFunctionCall.ql Outdated Show resolved Hide resolved
...xperimental/Security Features/backdoor/DangerousNativeFunctionCall.qhelp Outdated Show resolved Hide resolved
...xperimental/Security Features/backdoor/DangerousNativeFunctionCall.qhelp Outdated Show resolved Hide resolved
...p/ql/src/experimental/Security Features/backdoor/PotentialTimeBomb.qhelp Outdated Show resolved Hide resolved
...experimental/Security Features/backdoor/ProcessNameToHashTaintFlow.qhelp Outdated Show resolved Hide resolved
...curity Features/campaign/Solorigate/NumberOfKnownHashesAboveThreshold.ql Outdated Show resolved Hide resolved
...rity Features/campaign/Solorigate/NumberOfKnownLiteralsAboveThreshold.ql Outdated Show resolved Hide resolved
...rity Features/campaign/Solorigate/NumberOfKnownLiteralsAboveThreshold.ql Outdated Show resolved Hide resolved
...y Features/campaign/Solorigate/NumberOfKnownMethodNamesAboveThreshold.ql Outdated Show resolved Hide resolved
...ity Features/campaign/Solorigate/SwallowEverythingExceptionHandler.qhelp Outdated Show resolved Hide resolved
@raulgarciamsft
Fixes on multiple files.
Loading status checks…
d775528
@joshbw
Copy link

@joshbw joshbw commented Feb 6, 2021

I forgot to mention it on the first review, but would it be possible to add some tests for the new queries?

Yeah - we will do that early next week. We have been testing detection against the actual Orion implant so had overlooked our normal test dev. Sorry about that
@raulgarciamsft raulgarciamsft changed the base branch from main to lgtm.com Feb 8, 2021
@raulgarciamsft
Adding unit tests
Loading status checks…
f114ef1
@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 9, 2021

I forgot to mention it on the first review, but would it be possible to add some tests for the new queries?

Done. I added some unit tests. I tried to keep the tests simple, but let me know if you want me to make any changes. Thanks
@raulgarciamsft raulgarciamsft requested review from hvitved and sj Feb 9, 2021
@sj
sj reviewed Feb 10, 2021
View changes
Copy link
Collaborator

@sj sj left a comment

It turns out that I wrote a bunch of review comments last week that were never actually submitted in a review. Very sorry about that. Here they are. Let me know if you think it's feasible to make some of these changes in the next day or two; if not we'll note them for making later.
...p/ql/src/experimental/Security Features/backdoor/PotentialTimeBomb.qhelp Outdated Show resolved Hide resolved
...experimental/Security Features/backdoor/ProcessNameToHashTaintFlow.qhelp Outdated Show resolved Hide resolved
...experimental/Security Features/backdoor/ProcessNameToHashTaintFlow.qhelp Outdated Show resolved Hide resolved
.../src/experimental/Security Features/campaign/Solorigate/Solorigate.qhelp Outdated Show resolved Hide resolved
...Security Features/campaign/Solorigate/ModifiedFnvFunctionDetection.qhelp Outdated Show resolved Hide resolved
...curity Features/campaign/Solorigate/NumberOfKnownHashesAboveThreshold.ql Outdated Show resolved Hide resolved
...rity Features/campaign/Solorigate/NumberOfKnownLiteralsAboveThreshold.ql Outdated Show resolved Hide resolved
...y Features/campaign/Solorigate/NumberOfKnownMethodNamesAboveThreshold.ql Outdated Show resolved Hide resolved
raulgarciamsft and others added 2 commits Feb 10, 2021
@raulgarciamsft @sj
Update csharp/ql/src/experimental/Security Features/campaign/Soloriga…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
190164c 
…te/Solorigate.qhelp

Co-authored-by: Bas van Schaik <5082246+sj@users.noreply.github.com>
@raulgarciamsft
Addressing a few comments
Loading status checks…
ef0d372
@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 10, 2021

I think we have addressed all comments (BTW. Thanks a lot for all your feedback). Please let me know if we missed anything and I will fix it ASAP.
@raulgarciamsft
Addressing comments we missed earlier
Loading status checks…
710ca21
@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 11, 2021

I fixed the comments we were missing from @hvitved (for some reason I couldn't find them in the GitHub view, but we found them via email). Please reply to this message if there are any other comments we are missing and I will try to find the corresponding emails. Thanks a lot.
@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 12, 2021

There is a QHelp preview check that is failing. Not sure if it is caused by the Solorigate.qhelp file (that is shared for all the queries in this folder & only intended as an include for the individual .qhelp files) or something else. Please let us know what is the error & what can we do to fix it. Thanks
@shati-patel
Copy link
Contributor

@shati-patel shati-patel commented Feb 12, 2021 •
edited

There is a QHelp preview check that is failing. Not sure if it is caused by the Solorigate.qhelp file (that is shared for all the queries in this folder & only intended as an include for the individual .qhelp files) or something else. Please let us know what is the error & what can we do to fix it. Thanks

I asked about this internally, and it looks like this is a limitation of the QHelp preview check itself. A possible fix would be to change the name of the included file to Solorigate.qhelp.inc (and change the includes to <include src="Solorigate.qhelp.inc" />).

If that doesn't work, you might have to duplicate the shared snippet as a way to unblock the check, and we can tidy it up later on!
@hvitved
hvitved reviewed Feb 12, 2021
View changes
.../ql/src/experimental/code/csharp/Cryptography/NonCryptographicHashes.qll Outdated Show resolved Hide resolved
@raulgarciamsft
Fixing shared .qhelp issue (renaming to .qhelp.inc)& addressing a fix
Loading status checks…
782f4bc
@hvitved
hvitved reviewed Feb 15, 2021
View changes
Copy link
Contributor

@hvitved hvitved left a comment

I believe all my comments have now been addressed. Thanks for your patience @raulgarciamsft .
@raulgarciamsft raulgarciamsft requested a review from sj Feb 16, 2021
@tamasvajk
Copy link
Contributor

@tamasvajk tamasvajk commented Feb 17, 2021

@raulgarciamsft Can you tell me why the PR targets the lgtm.com branch? Could you please change it to main?
@raulgarciamsft raulgarciamsft changed the base branch from lgtm.com to main Feb 17, 2021
@raulgarciamsft raulgarciamsft dismissed hvitved’s stale review Feb 17, 2021

The base branch was changed.

@raulgarciamsft
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft commented Feb 17, 2021

@raulgarciamsft Can you tell me why the PR targets the lgtm.com branch? Could you please change it to main?

Fixing the PR target... I changed the branch by mistake.
@raulgarciamsft
Changes to the Readme file
Loading status checks…
cba9f42
@tamasvajk
tamasvajk approved these changes Feb 18, 2021
View changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

All changes appear to be addressed.
@tamasvajk tamasvajk dismissed sj’s stale review Feb 18, 2021

All changes appear to be addressed.

@tamasvajk tamasvajk merged commit 8e7a823 into github:main Feb 18, 2021
7 checks passed
7 checks passed
CodeQL-Build CodeQL-Build
Details
triage
Details
C# Language Tests (GitHub Actions) Internal CI run started by Qlucie
Details
C# QLDoc Checks (GitHub Actions) By qlucie
Details
Checks (GitHub Actions) Internal CI run started by Qlucie
Details
CodeQL No new or fixed alerts
Details
QHelp Preview Checks (GitHub Actions) Internal CI run started by Qlucie
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# documentation
7 participants
@raulgarciamsft @intrigus-lgtm @hvitved @joshbw @shati-patel @tamasvajk @sj