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

crypto/des: panics on incorrect block size #4991

Closed
crawshaw opened this issue Mar 6, 2013 · 7 comments
Closed

crypto/des: panics on incorrect block size #4991

crawshaw opened this issue Mar 6, 2013 · 7 comments
Labels
FrozenDueToAge

Comments

@crawshaw
Copy link
Member

crawshaw commented Mar 6, 2013 

If you give a []byte of invalid size to the des package to Decrypt, it panics.

http://play.golang.org/p/h4yOjQmm31

I.e.

package main

import "crypto/des"

func main() {
    block, err := des.NewTripleDESCipher(make([]byte, 24))
    if err != nil {
        panic(err)
    }

    buf := []byte{135, 29}
    block.Decrypt(buf, buf)
}

Produces:

panic: runtime error: index out of range

goroutine 1 [running]:
encoding/binary.bigEndian.Uint64(0xf840029000, 0x200000002, 0xf8400290f8, 0xf840029000,
0x406f7b, ...)
    go/src/pkg/encoding/binary/binary.go:105 +0x23
crypto/des.cryptBlock(0xf840000100, 0x1000000010, 0xf840029000, 0x200000002,
0xf840029000, ...)
    go/src/pkg/crypto/des/block.go:12 +0x50

Is this preferable to an error? If so, the documentation should probably mention it
somewhere.
@minux
Copy link
Member

minux commented Mar 6, 2013

Comment 1:

this is definitely a programming error so a panic is appropriate here.
(normally user shouldn't use cryptographic primitives like des directly,
and if they do use, they should know the basics)
we should add the documentation to crypto/cipher.Block that invalid
src and dst byte slice will cause panic.

@minux
Copy link
Member

minux commented Mar 6, 2013

Comment 2:

btw, per the Go 1 API contract, we can't eliminate the panic here, however i wonder
if we should check invalid size of src/dst so that we can give better panic message?
(this will make crypto. primitives slower, so i'm inclined to not add the check)

@rsc
Copy link
Contributor

rsc commented Mar 6, 2013

Comment 3:

I thought we changed the other crypto routines to detect this and panic
with a better error. Maybe check AES?

@minux
Copy link
Member

minux commented Mar 6, 2013

Comment 4:

AES's NewCipher check for invalid key sizes (the same for DES), but Encrypt and Decrypt
don't.

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 5:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@bradfitz
Copy link
Contributor

Comment 6:

Over to agl to decide.

Labels changed: removed priority-triage.

Owner changed to @agl.

Status changed to Accepted.

@agl
Copy link
Contributor

agl commented May 24, 2013

Comment 7:

Adding a conditional in order to have a nicer panic message is possible, but this is a
low-level call and I'd rather not, so I'm just going to call this WorkingAsIntended.

Status changed to WorkingAsIntended.

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@bradfitz @agl @rsc @minux @crawshaw @gopherbot