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

Clarify OSPS-BR-01 further #150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

david-a-wheeler
Copy link
Contributor

This attempts to clarify OSPS-BR-01 further.

In particular, the term "arbitrary" currently used in the criterion doesn't really mean anything. Any sequence of bits could be "arbitrary". It doesn't matter if the bits are arbitrary or not.

What matters is that the undesired inputs are untrusted. In fact, the rationale uses the term "untrusted", but the criterion and details aren't consistent with the rationale. I think that was what was meant anyway :-).

Also: using "code execution" as a privileged resource is a terrible example. Any CI/CD pipeline does code execution, so expressly listing it here confuses things.
Let's instead list "secret exfiltration" and "final release" as privileged operations. Those are much clearer as examples of things you should be concerned about :-).

Not everyone has a separate "final release" step, and instead consider "merge to main" the same as final release. But even in that case, "merge to main" is a privileged operation that is not granted to everyone, so I think it still works.

This attempts to clarify OSPS-BR-01 further.

In particular, the term "arbitrary" currently used in the criterion
doesn't really mean anything. Any sequence of bits could be
"arbitrary". It doesn't matter if the bits are arbitrary or not.

What matters is that the undesired inputs are *untrusted*.
In fact, the *rationale* uses the term "untrusted",
but the criterion and details aren't consistent with the rationale.
I think that was what was meant anyway :-).

Also: using "code execution" as a privileged resource is a
terrible example. Any CI/CD pipeline does code execution, so
expressly listing it here *confuses* things.
Let's instead list "secret exfiltration" and "final release"
as privileged operations. Those are *much* clearer as
examples of things you should be concerned about :-).

Not everyone has a separate "final release" step, and instead
consider "merge to main" the same as final release. But even in that
case, "merge to main" is a privileged operation that
is not granted to everyone, so I think it still works.

Signed-off-by: David A. Wheeler <[email protected]>
Copy link
Contributor

@funnelfiasco funnelfiasco left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

This does seem clearer, though the criteria and rationale still feels a little bit like "please don't write security bugs in your CI", rather than "avoid these common pitfalls".

to access privileged resources (code execution,
secret exfiltration, etc.)
to access privileged resources
(secret exfiltration, final release, etc.)
details: |
Ensure that any build and release pipeline actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ensure that any build and release pipeline actions
Ensure that any integration or release pipeline actions

I feel like continuous integration (particularly pre-merge pipelines) are particular targets here. I feel like we should call them out explicitly.

@david-a-wheeler
Copy link
Contributor Author

This does seem clearer, though the criteria and rationale still feels a little bit like "please don't write security bugs in your CI", rather than "avoid these common pitfalls".

Any suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants