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

16 cryptography models libraries and queries migration #14289

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

Conversation

ropwareJB
Copy link

Rebased microsoft#17 onto upstream/main.

@bdrodes:

Migration of C/C++ crypto models, inventory queries and example alerts.

Copy link

@github-code-scanning github-code-scanning bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

File layout 👍 for Python (did not review actual QL code yet)

Thanks for removing security tags for now, we should remember to add them when we want to publish these queries 😅 (I requested this to happen offline)

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

The C++ file-layout generally looks ok to me. However, the split between inventory/new_models and inventory/old_models eludes me, especially since inventory/old_models is empty. What is the purpose of this?

Also per our contributing guide, all the queries will need to be tagged with the experimental tag, which seems to be missing ("Experimental queries need to include experimental in their @tags").

I'm also slightly concerned by the lack of tests.

@andersfugmann
Copy link
Contributor

The security and experimental tags has been deliberately removed to avoid any risk of these queries to be included in the experimental query suite.

@bdrodes
Copy link
Contributor

bdrodes commented Sep 28, 2023

The C++ file-layout generally looks ok to me. However, the split between inventory/new_models and inventory/old_models eludes me, especially since inventory/old_models is empty. What is the purpose of this?

Also per our contributing guide, all the queries will need to be tagged with the experimental tag, which seems to be missing ("Experimental queries need to include experimental in their @tags").

I'm also slightly concerned by the lack of tests.

The new vs old models exists to keep consistency with each language. For CPP we don't have old models, but I also haven't done a deep dive to try to artificially create some. It's also possible I missed some.

The tags were removed by request to avoid automatic pack inclusion.

We have tests for some of sample alerts internally to Microsoft, but the point of these was to be examples, but we can definitely get tests for these. In terms of the CBOM queries, for this initial push, the focus was on getting our queries into a public space. I expect the underlying models will change and what is being alerted on might change in the near future as well, so our intent was that tests would be developed in the near future for these queries. In fact, we are currently figuring out what test means for bill of material queries as part of this effort (@pwntester is currently running cbom queries on various public repos). My thought was a test should compare the new vs old models so we have something to compare against for regression, which I believe is what @pwntester is currently experimenting with.

@jketema
Copy link
Contributor

jketema commented Sep 28, 2023

The new vs old models exists to keep consistency with each language. For CPP we don't have old models, but I also haven't done a deep dive to try to artificially create some. It's also possible I missed some.

That doesn't really tell me a lot. What's the purpose the the "old" models? Taking the word "old" face-value it's sound like something out dated that we do not want in the repo.

@bdrodes
Copy link
Contributor

bdrodes commented Sep 28, 2023

The new vs old models exists to keep consistency with each language. For CPP we don't have old models, but I also haven't done a deep dive to try to artificially create some. It's also possible I missed some.

That doesn't really tell me a lot. What's the purpose the "old" models? Taking the word "old" face-value it's sound like something out dated that we do not want in the repo.

In some languages there are existing models for cryptography, either partial or mature. Where those exist, we develop queries for CBOM using those models we call them "old" models compared to the current PQC bill of material effort. Queries built upon any new modeling for that purposes of PQC we are calling "new".

@jketema
Copy link
Contributor

jketema commented Sep 28, 2023

The new vs old models exists to keep consistency with each language. For CPP we don't have old models, but I also haven't done a deep dive to try to artificially create some. It's also possible I missed some.

That doesn't really tell me a lot. What's the purpose the "old" models? Taking the word "old" face-value it's sound like something out dated that we do not want in the repo.

In some languages there are existing models for cryptography, either partial or mature. Where those exist, we develop queries for CBOM using those models we call them "old" models compared to the current PQC bill of material effort. Queries built upon any new modeling for that purposes of PQC we are calling "new".

That still doesn't help a lot. Are the old models ours or yours? If they're ours they will be located somewhere else already, and a copy seems unnecessary. If they're yours, why should we become responsible for maintaining something that still sounds like it's outdated?

@bdrodes
Copy link
Contributor

bdrodes commented Sep 28, 2023

The queries in these directories are 'inventory' queries (as indicated by the parent directory). They are sub-categorized by if the inventory query uses an existing (older) model for crypto (which we simply import from the part of the repo it already exists in) or it uses a newer model developed for this PQC effort specifically (in which case the crypto models are in this PR).

The confusion is that the name of the directory is giving the impression that it contains crypto models. It does not, it contains inventory queries using existing crypto models (just calling them old, but 'classic' might be a better word) or new models.

@jketema
Copy link
Contributor

jketema commented Sep 28, 2023

Thanks for the clarification. I think there's a discussion to be had - at a later moment - as to whether we should have all these inventory queries, but the file-layout looks good as-is.

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

5 participants