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

Jump to bottom

x/crypto/poly1305: deprecate public package #36646

Closed
FiloSottile opened this issue Jan 20, 2020 · 10 comments
Closed

x/crypto/poly1305: deprecate public package #36646

FiloSottile opened this issue Jan 20, 2020 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Proposals (old)
Milestone
Unreleased

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Jan 20, 2020 •
edited

Poly1305 is an extremely delicate tool that should never be used directly. If a key is used twice, it breaks catastrophically. If the key is all zeroes, the tag is always zeroes. If the message is empty, the tag is the second half of the key.

All these things are fine only if Poly1305 is composed correctly, and there is exactly one widely used (or otherwise) composition: ChaCha20Poly1305 as implemented by x/crypto/chacha20poly1305 (or by SSH in their weird variant).

I know of no uses of Poly1305 standalone, and I'd argue x/crypto/poly1305 would have been x/crypto/chacha20poly1305/internal/poly1305, if not for historical reasons. (One exception, restic uses AES-CTR+Poly1305-AES, which no other library supports. This is exactly the kind of mistake we don't want to encourage.)

If anyone is considering using the x/crypto/poly1305 package, they should be dissuaded and pointed towards HMAC, which acts much more like people expect it to. If anyone is using the x/crypto/poly1305 package, they should be notified they are doing something nonstandard and dangerous. I think deprecating the package would be the right way to send that message.

Excluding forks, here are all the packages that import x/crypto/poly1305 from godoc.org at the moment:

  • github.com/amaizfinance/secreter/pkg/crypto/xchacha20poly1305 — and a number of other [X]ChaCha20Poly1305 implementations, which is now provided by x/crypto/chacha20poly1305
  • github.com/RoPe93/govpn — which rolled their own salsa20poly1305 for no apparent reason
  • github.com/euphoria-io/heim — using Poly1305 as a general MAC without key derivation, which is extremely suspicious — this is exactly the uses we want to nudge towards HMAC
  • github.com/danielhavir/go-ecies — possibly broken because it reuses Poly1305 keys (Possible Poly1305 misuse danielhavir/go-ecies#1) uses Poly1305 with a key derived from Ephemeral-Static ECDH
  • github.com/restic/restic/internal/crypto — weird AES-CTR+Poly1305-AES, see above
  • github.com/empirefox/confy/xps, github.com/empirefox/gotool/crypt, github.com/fastcat/wirelink, github.com/FiloSottile/age/internal/stream, github.com/hyperledger/aries-framework-go/pkg/didcomm/packer/jwe/authcrypt, github.com/keybase/saltpack, github.com/marcboeker/supertar/crypto, git.zx2c4.com/wireguard-go/device — just using the TagSize constant
  • github.com/karantin2020/jwtis — forked from Restic, but JWT?
  • github.com/kevinburke/nacl/onetimeauth — a very thin wrapper, because apparently NaCl exposed Poly1305
  • github.com/mad-day/go-various-ciphers/edge/lioness/salsapoly — another salsa20poly1305
  • github.com/Randomsock5/Randomsocks5 — a self-rolled XChaCha20 + Poly1305 that uses hashes to derive a key
  • github.com/safing/jess — possibly broken because it reuses Poly1305 keys (Possible Poly1305 misuse safing/jess#1)
  • github.com/xiaokangwang/KKEncSTar — a self-rolled XChaCha20 + Poly1305 that uses hashes to derive a key
  • lukechampine.com/adiantum — a legitimate non-standard construction for disk encryption
  • git.schwanenlied.me/yawning/basket2/framing/tentp — a self-rolled XChaCha20 + Poly1305

Note how out of a dozen total usages, there are two one probably broken ones. That does not clear the bar of safety of golang.org/design/cryptography-principles.

Warning on TagSize is a bit unfortunate, as there's nothing wrong with using that. Maybe we can just Deprecate New, Sum and Verify, but that sounds wrong as it's not the APIs that are deprecated but the whole primitive that should be avoided.

/cc @katiehockman @agl
@gopherbot gopherbot added this to the Proposal milestone Jan 20, 2020
@kevinburke
Copy link
Contributor

kevinburke commented Jan 20, 2020 •
edited

Frankly I'm surprised by this, given Nacl's reputation and the author's apparent interest in designing libraries that are difficult to misuse.

Note also Nacl's secretbox (both in my fork and in x/crypto/nacl/secretbox) uses poly1305 for authentication - in my library, secretbox imports onetimeauth, which is a wrapper around poly1305. I suppose that would still be OK if poly1305 was an internal package in the same project.

I'm open to other ideas about what to do, perhaps panicking if a zero key is passed?

@FiloSottile
Copy link
Contributor Author

Note also Nacl's secretbox (both in my fork and in x/crypto/nacl/secretbox) uses poly1305 for authentication - in my library, secretbox imports onetimeauth, which is a wrapper around poly1305. I suppose that would still be OK if poly1305 was an internal package in the same project.

Yes, to clarify, it would still be totally ok for x/crypto/chacha20poly1305, x/crypto/nacl/secretbox, and x/crypto/ssh to use this package. Deprecation warnings are not meant to be recursive.

I'm open to other ideas about what to do, perhaps panicking if a zero key is passed?

The major issue is the one-time use key, and anyway the right answer is using an HMAC, not trying to tame Poly1305. Also, there are a total of a dozen uses in open source, there is empirically not much need for this.

@kaey
Copy link

kaey commented Jan 20, 2020

Canonical url for govpn is http://www.govpn.info/ and author uses chacha20 in the latest version. However they still use x/crypto/poly1305 and x/crypto/internal/chacha20 directly instead of x/crypto/chacha20poly1305 I don't know why.

@aead
Copy link
Contributor

aead commented Jan 20, 2020 •
edited

In general, I agree that poly1305 is a low-level primitive and should only be used to build higher-level APIs.

Just for clarification:
Poly1305 is an AXU (almost XOR-universal hash function) and not a PRF. (The fact that you have to be aware of this is already an argument against exposing poly1305) However, Poly1305 is an excellent construction when used correctly. In particular, its an information-theoretic MAC. So it's security can & has be proven unconditionally when the key (r and s) are chosen properly - like GHASH (the GCM in AES-GCM). (This is not - and never will be - true for HMAC 😉 - leaving aside the performance advantage of poly1305).
For whoever is interested: https://cr.yp.to/mac/poly1305-20050329.pdf

Nevertheless, users of Poly1305 should be aware of the crypto. theory to use it correctly - which AFAIK (and agree) is not what we want in x/crypto. Therefore, marking it as "not safe for general purpose use" seems justified to me.

@dhaavi
Copy link

dhaavi commented Jan 20, 2020

I am the author of one of the projects listed in the original post and I'm all for the deprecation. Keeping the quality of the (extended) standard library high is really important.

We are considering removing direct usage of Poly1305 anyway. For more details, see issue raised by @FiloSottile in the project: safing/jess#1

@rsc rsc added this to Incoming in Proposals (old) Jan 28, 2020
@rsc
Copy link
Contributor

rsc commented Jan 28, 2020

Discussed with @FiloSottile. I am uncomfortable with marking a package deprecated when we have legitimate uses of it in our own code. It would make me feel better about setting the right example if we:

  • move x/crypto/poly1305 to x/crypto/internal/poly1305
  • leave behind in x/crypto/poly1305 a thin wrapper around x/crypto/internal/poly1305
  • make our own code in x/crypto use x/crypto/internal/poly1305
  • since poly1305.TagSize is sometimes needed by clients of chacha20poly1305,
    expose the constant in the latter package under an appropriate name
    (that's just good API design)
  • mark x/crypto/poly1305 as "Deprecated: The Poly1305 algorithm is a cryptographic
    building block that is not safe for general purpose use. Use golang.org/x/crypto/chacha20poly1305 instead."

Thoughts?

@rsc
Copy link
Contributor

rsc commented Feb 5, 2020

Adding to proposal minutes to increase visibility.

@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 5, 2020
@rsc rsc mentioned this issue Feb 5, 2020
Open
@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Feb 11, 2020
@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

The reactions to #36646 (comment) seem positive.
This seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Feb 12, 2020
@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Feb 26, 2020
@rsc rsc modified the milestones: Proposal, Unreleased Feb 26, 2020
@rsc rsc changed the title proposal: x/crypto/poly1305: deprecate public package x/crypto/poly1305: deprecate public package Feb 26, 2020
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 26, 2020
@FiloSottile FiloSottile self-assigned this Feb 26, 2020
@gopherbot
Copy link

Change https://golang.org/cl/345649 mentions this issue: poly1305: deprecate public package

fastcat added a commit to fastcat/wirelink that referenced this issue Mar 16, 2022
@fastcat
fix: address poly1305 package deprecation
a374656 
golang/go#36646
@johnnybubonic johnnybubonic mentioned this issue Jan 9, 2023
Open
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
poly1305: deprecate public package
24a3b31 
Fixes golang/go#36646

Change-Id: Ic19dd2171c84472fc9d3f44803224b87fc5c0417
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/345649
Trust: Filippo Valsorda <filippo@golang.org>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
poly1305: deprecate public package
2072ce5 
Fixes golang/go#36646

Change-Id: Ic19dd2171c84472fc9d3f44803224b87fc5c0417
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/345649
Trust: Filippo Valsorda <filippo@golang.org>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
@golang golang locked and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Proposals (old)
Accepted
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@rsc @kevinburke @FiloSottile @kaey @gopherbot @dhaavi @aead