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

Update IT advisories #128

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

Update IT advisories #128

wants to merge 3 commits into from

Conversation

justmurphy
Copy link
Collaborator

πŸ—£ Description

πŸ’­ Motivation and context

πŸ§ͺ Testing

βœ… Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

βœ… Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

βœ… Post-merge checklist

  • Create a release.

@justmurphy justmurphy added blocked This issue or pull request is awaiting the outcome of another issue or pull request improvement This issue or pull request will add new or improve existing functionality labels Oct 22, 2024
@justmurphy justmurphy self-assigned this Oct 22, 2024
@justmurphy justmurphy marked this pull request as draft October 22, 2024 23:15
Copy link
Collaborator

@mstrad mstrad left a comment

Choose a reason for hiding this comment

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

The CSAF itself looks good. However the hash file and signature file of the CSAF should be regenerated to reflect the changed CSAF.

@@ -94,7 +94,7 @@
"branches": [
{
"category": "product_version_range",
"name": "all versions",
"name": "<=4.8.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the above change.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC Adminer "4.8.1" happens to be a "Vers -like Specifier (vls)" so I think this change works syntactically and I understand that "all versions" is not valid.

However I'm slightly concerned with how to interpret this limit on the version range. The product is used in status and remediation elements. "<=4.8.1" might imply that there might be a more recent version, and is that more recent version fixed? We can't know the future at the time of publication, could update the CSAF if a new/fixed version is released, and the CSAF VEX profile has an explicit "Fixed" status, but at least for those familar with the CVE ecosystem, "<=4.8.1" could imply "fixed in >4.8.1, now go look to see if any such versions exist."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@amanion-cisa there is also this as an option:
Version Range Specifier (vers)
vers is an ongoing community effort to address the problem of version ranges. Its draft specification is available at [VERS].
vers MUST be used in its canonical form. To convey the term "all versions" the special string vers:all/* MUST be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I used vers:all/* for the Adminer advisory, I think this is more correct informationally, see develop...amanion-cisa:CSAF:jmfixes#diff-b68d7bbabc197e783ddff83a11b37d5c9e309aeec18c89cf5551b478586daac1L97-R166.

@@ -158,7 +158,7 @@
"product_id": "CSAFPID-0078"
},
"category": "product_version_range",
"name": "all versions"
"name": "<4.8.4"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the above change.

]
"Matthew Galligan"
],
"organization": "CISA Rapid Action Force",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://docs.oasis-open.org/csaf/csaf/v2.0/os/csaf-v2.0-os.html#311-acknowledgments-type

Properties for acknowledgements are names, organization, summary, and urls. Moved organization from names property to organization property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the above change.

]
"Matthew Galligan"
],
"organization": "CISA Rapid Action Force",
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the above change.

]
"Matthew Galligan"
],
"organization": "CISA Rapid Action Force",
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the above change.

@justmurphy
Copy link
Collaborator Author

justmurphy commented Oct 24, 2024

The CSAF itself looks good. However the hash file and signature file of the CSAF should be regenerated to reflect the changed CSAF.

@mstrad the issue I am seeing is the product tree structure: vendor -> product_family -> product_name -> product_version.

Going to revise the advisory to meet recommended structure and will ping you for another review. I believe that all the IT advisories will need to be revised for product tree structure, and then small edits to the product_version_range and acknowledgement properties.

@mstrad
Copy link
Collaborator

mstrad commented Oct 24, 2024

The CSAF itself looks good. However the hash file and signature file of the CSAF should be regenerated to reflect the changed CSAF.

@mstrad the issue I am seeing is the product tree structure: vendor -> product_family -> product_name -> product_version.

Going to revise the advisory to meet recommended structure and will ping you for another review. I believe that all the IT advisories will need to be revised for product tree structure, and then small edits to the product_version_range and acknowledgement properties.

@justmurphy I see what you're saying. Initially I was only reviewing the proposed changes in this PR itself. All of which I agree with and offer the reminder that new hashes and signature files will need to be made and pushed to the repo when a CSAF file is updated. :)

To the comment above, I agree. I took a look at the raw CSAF file and I agree that following the recommended product tree structure should be followed with the ending leaf being a product_version or product_version_range. Will this recommendation become a requirement in future CSAF versions?

Copy link
Contributor

@amanion-cisa amanion-cisa left a comment

Choose a reason for hiding this comment

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

FWIW I'm fine with the changes, they are fixing technically incorrect CSAF. I can make the updates including new hashes, signatures, and ROLIE files if you'd like.

@@ -94,7 +94,7 @@
"branches": [
{
"category": "product_version_range",
"name": "all versions",
"name": "<=4.8.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC Adminer "4.8.1" happens to be a "Vers -like Specifier (vls)" so I think this change works syntactically and I understand that "all versions" is not valid.

However I'm slightly concerned with how to interpret this limit on the version range. The product is used in status and remediation elements. "<=4.8.1" might imply that there might be a more recent version, and is that more recent version fixed? We can't know the future at the time of publication, could update the CSAF if a new/fixed version is released, and the CSAF VEX profile has an explicit "Fixed" status, but at least for those familar with the CVE ecosystem, "<=4.8.1" could imply "fixed in >4.8.1, now go look to see if any such versions exist."

@amanion-cisa
Copy link
Contributor

Here are fixes for version ranges and acknowledgments (and section headings, not initially part of this PR): develop...amanion-cisa:CSAF:jmfixes

I won't send a PR until we've reviewed these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue or pull request is awaiting the outcome of another issue or pull request improvement This issue or pull request will add new or improve existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants