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

internal/crypto: Decrypt overlap rules are a bit off #1190

Closed
FiloSottile opened this Issue Aug 28, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@FiloSottile
Contributor

FiloSottile commented Aug 28, 2017

There seems to be a problem with the Decrypt overlap rules:

plaintext and ciphertext may point to (exactly) the same slice.

The cipher.Stream interface implemented by CTR mode says this at the method XORKeyStream:

Dst and src may point to the same memory.

Which isn't that helpful. There is already a discussion about it at golang/go#21279 but I think it means that dst and src may overlap entirely (without misalignment) or not at all.

However, calling Decrypt with the same buffer for plaintext and ciphertext (as done throughout the code and allowed by the docs) means that buf[16:] is decrypted into buf[:] because of the IV. That's theoretically(?) not ok.

I don't think it's a problem right now, but it might become one as the CTR implementation is optimized.

Also, I'm not sure what this is supposed to mean in the Encrypt docs, but overlapping buffers are never used in the codebase anyway.

ciphertext and plaintext may not point to (exactly) the same slice or non-intersecting slices.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 29, 2017

Contributor

Looks like Decrypt will panic when golang/go#21624 happens.

Contributor

FiloSottile commented Aug 29, 2017

Looks like Decrypt will panic when golang/go#21624 happens.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 29, 2017

Contributor

A nice standard API to consider might be to make Key implement cipher.AEAD with AES256-Poly1305-AES, and move IV splitting to the callers.

Contributor

FiloSottile commented Aug 29, 2017

A nice standard API to consider might be to make Key implement cipher.AEAD with AES256-Poly1305-AES, and move IV splitting to the callers.

@fd0

This comment has been minimized.

Show comment
Hide comment
@fd0

fd0 Aug 29, 2017

Member

Ah, that's indeed a thing we should resolve, good catch!

And I like the idea of implementing cipher.AEAD, we just need to see what we do with additionalData. Since we don't need it in restic, return an error if additional data is supplied? Or just throw it away?

Member

fd0 commented Aug 29, 2017

Ah, that's indeed a thing we should resolve, good catch!

And I like the idea of implementing cipher.AEAD, we just need to see what we do with additionalData. Since we don't need it in restic, return an error if additional data is supplied? Or just throw it away?

@fd0 fd0 added the enhancement label Aug 29, 2017

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 30, 2017

Contributor

I'd definitely return an error 👍

Contributor

FiloSottile commented Aug 30, 2017

I'd definitely return an error 👍

@cstockton

This comment has been minimized.

Show comment
Hide comment
@cstockton

cstockton Aug 31, 2017

@FiloSottile Happened to read on r/golang about restic and saw this issue. It's not immediately clear to me right now if the buffer reuse which I try to avoid in general is being FORBIDDEN or ALLOWED here? When I interpreted the doc comments in #731 (comment) the language seemed to be granting specific permission to ALLOW buffer reuse, was it meant to be interpreted as a implementation detail of the interface which would imply such use is unsafe?

cstockton commented Aug 31, 2017

@FiloSottile Happened to read on r/golang about restic and saw this issue. It's not immediately clear to me right now if the buffer reuse which I try to avoid in general is being FORBIDDEN or ALLOWED here? When I interpreted the doc comments in #731 (comment) the language seemed to be granting specific permission to ALLOW buffer reuse, was it meant to be interpreted as a implementation detail of the interface which would imply such use is unsafe?

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 31, 2017

Contributor

Buffer reuse is allowed, if and only if the two slices overlap entirely. This is globally defined by the interface. golang/go#21279 is about to make the wording clearer.

Contributor

FiloSottile commented Aug 31, 2017

Buffer reuse is allowed, if and only if the two slices overlap entirely. This is globally defined by the interface. golang/go#21279 is about to make the wording clearer.

@fd0 fd0 added this to the 0.8.0 milestone Oct 28, 2017

@fd0 fd0 referenced this issue Oct 28, 2017

Merged

Small improvements #1395

@fd0 fd0 closed this in #1395 Oct 28, 2017

@fd0 fd0 reopened this Oct 28, 2017

@fd0

This comment has been minimized.

Show comment
Hide comment
@fd0

fd0 Oct 28, 2017

Member

Meh, closed prematurely ;)

Member

fd0 commented Oct 28, 2017

Meh, closed prematurely ;)

@fd0

This comment has been minimized.

Show comment
Hide comment
@fd0

fd0 Oct 28, 2017

Member

@FiloSottile I've added changes in #1397 which makes *crypto.Key implement cipher.AEAD. I like the code! Would you mind reviewing the changes if you have time?

Member

fd0 commented Oct 28, 2017

@FiloSottile I've added changes in #1397 which makes *crypto.Key implement cipher.AEAD. I like the code! Would you mind reviewing the changes if you have time?

@fd0 fd0 closed this in #1397 Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment