Re: Discussions on Security Enhancements

From: Daniel Stenberg via curl-library <curl-library_at_lists.haxx.se>
Date: Thu, 10 Nov 2022 00:28:19 +0100 (CET)

On Wed, 9 Nov 2022, Diogo Sant'Anna wrote:

Hi,

I think your last response didn't reach the curl-library list as you weren't
subscribed yet. I will therefore include more of your response in the quotes
below so that readers can still see what was written.

> Well I'm happy you tried on Scorecards, I appreciate the trust! It's
> unfortunate that the action was not helpful for you, but your
> detailed feedbacks are already of huge value for us and I'll get all of
> them to the OpenSSF team.

I want to be clear and say that I'm not dissing the tool nor the idea, I just
don't think it is very helpful *to us*.

> I would like to use this reply to complement our discussion about
> pinned-dependencies and get some details to try to understand why some
> checks did not work properly on your repo, so that we can possibly work to
> enhance the tool.
>
>> 1 - "Code-Review High"
>
> I think this check did not get a good result because the last commits were
> not merged through PRs, as I could see in this example
> <https://github.com/curl/curl/pull/9691>, and the Scorecards is yet not able
> to verify reviews like this. In order to give a more complete feedback and
> reason to add this feature, could you explain to me how the review process
> works (or send me a link)?

We never click the merge button in the GitHub UI, we always merge
pull-requests "manually" using git. This allows us to make sure the commit
messages are exactly the way we want them and is a much easier/faster way to
squash/cleanup commits etc before merge.

We only fairly infrequently add a "Reviewed-by:" line in the commit message,
most commits are actually done without recording any review details. Most (but
far from all) commits are still reviewed by someone else than its author. I
think primarily my commits are at times not reviewed by anyone else.

>> 2 - "Token-Permissions High"
>>
> I think there was a misunderstanding because of the name of the check. The
> check is not verifying if you set up a token for the job, it looks for the
> permissions given to the GITHUB_TOKEN that's automatically generated for
> your workflows, so those permissions turns out to be called
> "token-permissions" >< . Please forgive me if I understood incorrectly, but
> this confusion makes perfect sense to me and I'm already sending this as
> feedback to the Scorecards team.
> In any case, the "token-permissions" check verifies if the top-level
> permissions are set to read-only, following the principle of least
> privilege. Those permissions are related to our discussion about
> pinned-dependencies, so I'll join these subjects on the 4th point.

Thanks, I went back and read the descriptions again and the linked-to github
documentation and I think I'm starting to understand what you are talking
about.

I now understand that you point out that the github workflows as they are
setup right now for curl allow the jobs to write contents back into the git
repo as hosted on github?

If so, then we really need to stop that. It was however not immediately clear
to me exactly what we want the permissions to be set to. Is 'read-all' too
restrictive ?

>> 3 - "Dependency-Update-Tool High"
>>
> Could you tell which dependency-update-tool the project is currently using?
> I can raise an issue to ask for its support.

We don't have depdencies like that so there is no such tool. You build curl to
use the dependencies that are installed in the machine where curl is
built/run. We cannot scan for those on GitHub.

>> 4 - "Pinned-Dependencies Medium"
>>
> So, the thing is that, depending on the workflow permissions, an external
> GitHub Action actually could modify your code. Take a look at this action
> <https://github.com/marketplace/actions/add-commit>, for example; it's an
> action created for the purpose of committing into the code of whoever calls
> it. Of course this one is intended to be used for good and is trustworthy,
> but if a potential malicious user compromises the spell-checker, for
> example, he could try similar things and modify your code.
>
> Against this kind of threat, two solutions can be applied (ideally both of
> them):
> 1. Limit the privileges of the workflows, setting them as read-only or
> giving only the permissions they need. If no privilege is explicitly set on
> the workflow, the default one is used. If you have not changed the default
> privilege on your GitHub settings (I can not evaluate that, as this setting
> is not public), the default is write-all, which can be dangerous. Adding
> these explicit workflow permissions correspond to the "Token-Permissions"
> check on Scorecards.
> 2. Pin the dependencies to specific and unmodifiable versions of the job.
> This would mean hash-pinning the dependencies, as even when referring by
> version (as _at_vX.Y.Z), the version can be modified to point to different
> code. The downside of this solution is that the job would not be updated
> automatically, but some dependency-updates tools(such as dependabot and
> renovatebot) are already able to suggest updates on this scenario.

Is read-all what we want?

BTW, in the settings/actions section for the curl/curl repo under the
"Workflow persmissions" title I have selected "Read respository contents
persmission" which is described as: "Workflows have read permissions in the
repository for the contents scope only."

As opposed to the other alternative which says "Workflows have read and write
permissions in the repository for all scopes"

*read permissions* vs *read and write permissions*. If that doesn't restrict
the workflows from writing content, then what does this option mean?

-- 
  / daniel.haxx.se
  | Commercial curl support up to 24x7 is available!
  | Private help, bug fixes, support, ports, new features
  | https://curl.se/support.html
-- 
Unsubscribe: https://lists.haxx.se/listinfo/curl-library
Etiquette:   https://curl.se/mail/etiquette.html

Received on 2022-11-10