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

Adding queries related to the Solorigate campaign #5083

Merged
merged 18 commits into from
Feb 18, 2021

Conversation

raulgarciamsft
Copy link
Contributor

No description provided.

@raulgarciamsft raulgarciamsft requested a review from a team as a code owner February 3, 2021 23:29
@intrigus-lgtm
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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 February 4, 2021 11:18
@hvitved
Copy link
Contributor

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
Copy link
Collaborator

@sj sj left a comment

Choose a reason for hiding this comment

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

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 😄

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

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 :-)

Changing the TimeBomb query to path-problem (any suggestions to improve it would be welcomed, no previous experience iwth path-problem queries)
@hvitved
Copy link
Contributor

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?

Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

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?)

@joshbw
Copy link

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 February 8, 2021 17:51
@raulgarciamsft
Copy link
Contributor Author

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

sj
sj previously requested changes Feb 10, 2021
Copy link
Collaborator

@sj sj left a comment

Choose a reason for hiding this comment

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

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.

raulgarciamsft and others added 2 commits February 10, 2021 13:30
…te/Solorigate.qhelp

Co-authored-by: Bas van Schaik <5082246+sj@users.noreply.github.com>
@raulgarciamsft
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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 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

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 previously approved these changes Feb 15, 2021
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

I believe all my comments have now been addressed. Thanks for your patience @raulgarciamsft .

@tamasvajk
Copy link
Contributor

@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 February 17, 2021 17:38
@raulgarciamsft raulgarciamsft dismissed hvitved’s stale review February 17, 2021 17:38

The base branch was changed.

@raulgarciamsft
Copy link
Contributor Author

@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.

Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

All changes appear to be addressed.

@tamasvajk tamasvajk dismissed sj’s stale review February 18, 2021 12:50

All changes appear to be addressed.

@tamasvajk tamasvajk merged commit 8e7a823 into github:main Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants