-
Notifications
You must be signed in to change notification settings - Fork 497
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
Add threat model #2033
Draft
dstebila
wants to merge
1
commit into
main
Choose a base branch
from
ds-threat-model
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Add threat model #2033
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little hesitant to say that we aim for constant-time behaviour on all Tier 1 platforms, given that we only run constant-time tests on Linux / x86_64. There's a lot of aarch64 code that isn't constant-time tested, and we certainly don't run the CT tests on macOS. (Does Valgrind even work on macOS?) Maybe it would be best to say that we will respond to reports of CT issues on Tier 1 platforms.
I did actually try out the CT-time tests on the Arm runner late last year, and the results were OK: just a handful of reports on Falcon and McEliece that I haven't had time to pick through. Maybe this is a signal to take up that work again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If valgrind works on macOS I've never gotten it to work. 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I was hoping to tie the threat model back to our supported platforms in some way, but I guess I didn't get it right. I guess yes, we could say that we will respond to reports of CT issues on Tier 1 platforms, and for all other tiers of platforms it is nice-to-have but not actively supported (or some other wording?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PLATFORMS.md file has a † next to platforms on which constant-time tests are run—maybe we can refer to this in the threat model and make an effort to expand the list past x86_64 / Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does raise an interesting conversation. Does it make sense that some tier 1 platforms be supported in different ways (in this case constant time checks) than others? Maybe it would be worth having a set of tier 0 platforms? Forgive me for not being super active so there might be other cases where we support some tier 1 platforms in different ways than that other tier 1 platforms, but having a tier 0 "we aim to support everything on this platform" might be a nice to have distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention with "select" was to refer to PLATFORMS.md via the link, which has more detail on which platforms have the constant-time tests. What about this wording:
"OQS tests for secret-dependent branches and memory accesses on select Tier 1 platforms. See PLATFORMS.md for details."
I think that patching efforts will vary greatly depending on the nature of the report. At a minimum, we should clearly document and disseminate information about the bug—as we have done for constant-time test failures for pre-standards algorithms like McEliece or Falcon. That said, would details about our response not be better suited for the security response process document?
I think this section clearly
(1) lays out what we do with regards to automated CT testing,
(2) explains the scope and limitations of our tests, and
(3) surfaces information about known test failures.
In my view, this is valuable information for users to have. What if "best effort" were replaced by redirecting to our (upcoming) security response process document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating on the more detailed wording. I tried to incorporate observations from the discussion in the revised text below, changing the first sentence (adding the word "Some") and the last sentence.
Regarding Michael's comment
I think the goal right now is to clearly document our current practice so that users can make their own decision. I think the text you've been iterating on is pretty close to reflecting our current practice. As to whether users will find it a satisfyingly reliable commitment -- well, that's up to them. If they want a stronger commitment, then hopefully that will drive them to participate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To move this beyond "hope", what about making this explicit, then? Maybe with an introduction to the whole document reading like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a blanket statement that applies more broadly than the threat model. Is there a better place for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we agree that it is correct statement, then of course, it might sensibly be listed in all places where we think people will try to learn about the state of the code -- and where we may be able to find new contributors.
IMO it fits perfectly at the top of SECURITY.md as that file is what GH and other docs point to where to learn about the security properties of the software -- and where OQS with this PR now documents its commitments in this regard and that the proposed text refers to.
It then of course also could be added to the main README.md, CONTRIBUTING.md and the www site for people using those to gauge the level of support that the community can provide. But those files do not contain any commitment statements such as this document, so that would arguably need different verbiage. My proposal indeed was meant to target SECURITY.md only at this time -- as that's what this PR is about.
I'd be glad to provide suitably adapted versions via PR(s) for the other places if we agree that the basic tenet is right: Please let me know if I should do.