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

x/crypto/salsa20/salsa: confusing docs #21279

Closed
bradfitz opened this issue Aug 2, 2017 · 10 comments
Closed

x/crypto/salsa20/salsa: confusing docs #21279

bradfitz opened this issue Aug 2, 2017 · 10 comments

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Aug 2, 2017

The docs for XORKeyStream say:

// XORKeyStream crypts bytes from in to out using the given key and counters.
// In and out may be the same slice but otherwise should not overlap. Counter
// contains the raw salsa20 counter bytes (both nonce and block counter).
func XORKeyStream(out, in []byte, counter *[16]byte, key *[32]byte) {

What does this even mean: ?

may be the same slice but otherwise should not overlap

If they're the same slice (same 3-tuple ptr/len/cap) then they by definition overlap.

Fix this and any copy/pastes of this wording we find.

@gopherbot gopherbot added this to the Unreleased milestone Aug 2, 2017
@ALTree
Copy link
Member

ALTree commented Aug 2, 2017

I think the key word is otherwise

In and out may be the same slice but otherwise should not overlap.

Same slice is ok, and it that case they'll overlap (obviously), but otherwise they shouldn't overlap.

@bradfitz
Copy link
Contributor Author

bradfitz commented Aug 2, 2017

Huh? I don't think that helps.

@FiloSottile
Copy link
Contributor

How about

in and out must overlap entirely or not at all.

@kreichgauer
Copy link
Contributor

if in and out overlap, they must be identical

is how it's usually worded in BoringSSL

@agl
Copy link
Contributor

agl commented Aug 3, 2017

The intent is to describe that there are only two valid situations:

  1. in and out are the same slice.
  2. in and out do not alias at all.

Other configurations (i.e. in and out partially overlap) may not produce the correct result. As Martin notes, we often use a slightly different wording in BoringSSL. I fear that I'm too close to the trees to have a valid judgement about what would be clearer so someone else is free to pick something and change the wording.

@FiloSottile
Copy link
Contributor

FiloSottile commented Aug 3, 2017 via email

@crvv
Copy link
Contributor

crvv commented Aug 9, 2017

Maybe
out[0:1] and in[1:] must not overlap.

I think this is accurate but a little obscure.

@FiloSottile
Copy link
Contributor

We should also put that text in the cipher.Stream.XORKeyStream docs.

The current text probably means this, but leaves too much open to interpretation, and implementations are already more strict anyway:

Dst and src may point to the same memory.

For the sake of breaking the bikeshed, I'm going to CL "must overlap entirely or not at all" tomorrow unless anyone objects.

@gopherbot
Copy link

Change https://golang.org/cl/61133 mentions this issue: all: make overlap rules wording consistent

@gopherbot
Copy link

Change https://golang.org/cl/61132 mentions this issue: crypto/cipher, crypto/rc4: make overlap rules wording consistent

gopherbot pushed a commit to golang/crypto that referenced this issue Sep 11, 2017
Updates golang/go#21279

Change-Id: I686835c644f52e3d5ea2b7e6431ef096d188c19d
Reviewed-on: https://go-review.googlesource.com/61133
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Oct 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants