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.

Sign up for GitHub

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

Jump to bottom

[RFC 0034] Expression Integrity #34

Closed
wants to merge 3 commits into from
Closed

[RFC 0034] Expression Integrity #34

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f31e80
add initial draft of 0034 Package Integrity
lrvick Sep 29, 2018
a5c9854
limit scope to only nix expression integrity
lrvick Sep 29, 2018
d178f63
phrasing/typos
lrvick Sep 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 142 additions & 0 deletions rfcs/0034-expression-integrity.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
---
feature: expression-integrity
start-date: 2018-09-28
author: lrvick
co-authors:
related-issues:
- https://github.com/NixOS/nix/issues/404
- https://github.com/NixOS/nix/issues/613
- https://github.com/NixOS/nix/issues/748
---

# Summary
[summary]: #summary

This RFC seeks to provide a strategy to allow NixOs to strongly attest who
authored a nix expression, who reviewed it, and that it has not been tampered
with outside the review flow.

# Motivation
[motivation]: #motivation

Due to the lack of VCS integrety on NixOS today a bad actor can gain remote
code execution on NixOS systems if any of the following are true:

* A Github employee is coerced or malicious
* The Github account credentials of any maintainer are compromised
* A successful (even limited) MITM attack on github.com

Essentially NixOS has many single points of trust, and thus single points of
failure.

This is a serious design flaw and we can learn lessons from other package
management systems that have been burned by similarly poor package management
designs.

See examples of major security incidents in other package managers:

* Gentoo: https://archives.gentoo.org/gentoo-announce/message/dc23d48d2258e1ed91599a8091167002
* Debian: https://lists.debian.org/debian-devel-announce/2006/07/msg00003.html
* NPM: https://eslint.org/blog/2018/07/postmortem-for-malicious-package-publishes
* PyPi: https://www.reddit.com/r/Python/comments/8hvzja/backdoor_in_sshdecorator_package/
* Ubuntu Snap: https://github.com/canonical-websites/snapcraft.io/issues/651
* Arch Linux AUR: https://lists.archlinux.org/pipermail/aur-general/2018-July/034153.html

# Detailed design
[design]: #detailed-design

## Package Contributor

### Workflow

1. Author and test a nix expression
2. PR a signed commit adding nix expression to NixOS/nixpkgs repo

### Notes

* Can be enforced by mandating all commits be signed in VCS settings
* Contributors who choose not to sign will need someone else to PR for them

## Package Maintainer

### Workflow

1. Verify signing key ID is listed in maintainers list
* Add key ID to maintainers list if not already present
2. Verify signature on PR matches public key id in contributors list
* Add key ID to contributors list if not already present
3. Review content of new PR for general best practices
4. Ensure signatures/hashes verified for third party code referenced
5. Make signed merge commit to master of NixOS/nixpkgs

### Notes

* Maintainer signatures should be a hard requirement
* Maintainer and Contributor should never be the same person.

## Nix Clients

### Workflow

1. Pull latest nix expressions from VCS repo
2. Verify author/reviewer commit signatures for all nix expressions
3. Build/Install nix expression

### Notes

* Local building is required for integrity as no trusted cache system exists
Copy link
Member

Choose a reason for hiding this comment

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

eh?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the misunderstanding lies in "cache system I can trust"/"trust-less cache system" vs "cache system that requires trust"

Copy link
Author

@lrvick lrvick Sep 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If the current artifact hosting system is compromised, it is game over and clients will blindly trust malicious builds.

It is only when you have signing by each maintainer -or- a highly transparent reproducible build system that clients will have a way to notice if an artifact server is compromised. Much the same way debian users can trust any random http/ftp mirror today, because the client will verify the hash and the maintainer signature on that package. Any system that trusts a single individual or computer is not compromised, is bound to be abused.

This RFC if implemented would only protect users building from source as only the nix expressions themselves would have signatures by both the creator and reviewer.

Copy link
Member

@grahamc grahamc Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think maybe you misunderstand what our Hydra is? It doesn't host artifacts, it is a build server. Its results are uploaded to our binary cache. Nothing a maintainer builds goes to the binary cache. Maintainers cannot upload to the binary cache. There is no way for a maintainer to sign the NAR in the end, because the maintainer has no part in its creation. Other distributions do this because they put a great deal of trust in the maintainer to not backdoor the artifacts. We don't, because, well, we don't.

Copy link
Author

@lrvick lrvick Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding is that it blindly pulls code from github without verifying it, then blindly builds/signs artifacts. This is honestly awful security. Instead of an individual maintainer/reviewer pairs that can at worst only backdoor their own package, you have a central system that can backdoor any package at any time, and so can those with remote access to that build system.

Because hydra signs anything without code review or verification of signatures from code reviewers it means you need absolute trust in Github, in Hydra, the network that connects them, every person with shell access on the Hydra machines, and every one with push access. This is imo highly irresponsible and honestly puts every individual with that much solitary power in a non obvious position of personal risk.

As one practical example: If I had a remote shell exec 0-day I would use it gain remote access to hydra to inject a small hard to detect change just before compilation in major bitcoin wallet packages that compromises the random number generator allowing me to factor keys. I would then wait for a big enough wallet balance years later and take it all. I could also blackmail a hydra maintainer to do it for me as the potential payout is worth it. I could also phish the github credentials of one person with commit access, turn off notifications, and rewrite history quickly just after the next change to that package. Hydra can't hope to deliver trusted outputs until it can verify its inputs and independant copies of it can be run with different maintainers that all get the same results (reproducible builds). We can't even begin to solve that problem until we have a trusted signed tree of package definitions without a SPOF.


# Drawbacks
[drawbacks]: #drawbacks

Some contributors to NixOS may no longer contribute if doing so requires some
additional security work.

# Alternatives
[alternatives]: #alternatives

## Git Notes signing

Reviewer/maintainer signatures could be added to the Git Notes interface on
a given ref allowing m-of-n signing for security critical expressions.

This would additionally negate the need for merge commits and would allow
VCS automatic merging to be used if desired.

## Patch ID

One could chose to sign a Git "patch-id" instead of a given ref hash. This
would allow signatures to still be valid even if a git rebase was done that
didn't add any LOC changes to a given changeset. This could add flexibility
but will need more testing.

Example:

```
git diff-tree -p "someref"..HEAD | git patch-id --stable | gpg -as
```

## Detached signatures

We could avoid using VCS level signing at all and simply mandate maintainers
add their detached .nix.sig files to a PR before it merges.

# Unresolved questions
[unresolved]: #unresolved-questions

Currently this scheme does not attempt to solve how to tie trusted cached
binaries to their signed VCS commits. Until a solution for this is reached
users will have to build every expression locally which makes securely using
NixOS impractical on low-spec systems or users with limited time to wait on
compiles.

There are other proposals in progress to address this.

# Future work
[future]: #future-work

It may be desireable to continue to have an untrusted expression repo like the
one used today that users can install from by hand as they please.

This could be an analogue of the Arch Linux User Repository (AUR) vs the
trusted/signed official repos.
Copy link
Member

Choose a reason for hiding this comment

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

We actually have something similar already at https://github.com/nix-community/nur