Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

proposal: x/crypto/poly1305: deprecate public package #36646

Open
FiloSottile opened this issue Jan 20, 2020 · 6 comments
Open

proposal: x/crypto/poly1305: deprecate public package #36646

FiloSottile opened this issue Jan 20, 2020 · 6 comments
Labels
Projects
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jan 20, 2020

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 (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 (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
@gopherbot gopherbot added the Proposal label Jan 20, 2020
@kevinburke

This comment has been minimized.

Copy link
Contributor

@kevinburke kevinburke commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Member Author

@FiloSottile FiloSottile commented Jan 20, 2020

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

This comment has been minimized.

Copy link

@kaey 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

This comment has been minimized.

Copy link
Contributor

@aead aead commented Jan 20, 2020

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

This comment has been minimized.

Copy link

@dhaavi 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 Jan 28, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
7 participants
You can’t perform that action at this time.