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

crypto/cipher: out of bounds write #21104

Closed
TocarIP opened this issue Jul 20, 2017 · 10 comments
Closed

crypto/cipher: out of bounds write #21104

TocarIP opened this issue Jul 20, 2017 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@TocarIP
Copy link
Contributor

TocarIP commented Jul 20, 2017

What version of Go are you using (go version)?

go version devel +4c98ecb Thu Jul 13 17:53:10 2017 +0000 linux/amd64

Also reproducible on 1.8

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/localdisk/itocar/gopath"
GORACE=""
GOROOT="/localdisk/itocar/golang"
GOTOOLDIR="/localdisk/itocar/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build497666732=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Run following program:

package main

import (
        "crypto/aes"
        "crypto/cipher"
        "fmt"
        "syscall"
)

func main() {
        const pageSize = 4 << 10
        data, err := syscall.Mmap(0, 0, 2*pageSize, syscall.PROT_READ|syscall.PROT_WRITE, syscall.MAP_ANON|syscall.MAP_PRIVATE)
        if err != nil {
                panic(err)
        }
        if err := syscall.Mprotect(data[pageSize:], syscall.PROT_NONE); err != nil {
                panic(err)
        }
        buf1 := data[pageSize-3 : pageSize]
        buf2 := make([]byte, 32)
        var key [16]byte
        var iv [16]byte
        aes, _ := aes.NewCipher(key[:])
        ctr := cipher.NewCTR(aes, iv[:])
        ctr.XORKeyStream(buf1, buf2)
        fmt.Println(buf1[5])
}

What did you expect to see?

Out of bounds write is detected and program panics before write.

What did you see instead?

unexpected fault address 0x7fdedc27b000
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x2 addr=0x7fdedc27b000 pc=0x459fb2]

goroutine 1 [running]:
runtime.throw(0x4a4dc7, 0x5)
/localdisk/itocar/golang/src/runtime/panic.go:605 +0x95 fp=0xc420045dc0 sp=0xc420045da0 pc=0x426f05
runtime.sigpanic()
/localdisk/itocar/golang/src/runtime/signal_unix.go:374 +0x227 fp=0xc420045e10 sp=0xc420045dc0 pc=0x439dd7
crypto/cipher.fastXORBytes(0x7fdedc27affd, 0x3, 0x1003, 0xc420014160, 0x20, 0x20, 0xc42008c000, 0x200, 0x200, 0x10)
/localdisk/itocar/golang/src/crypto/cipher/xor.go:29 +0x52 fp=0xc420045e20 sp=0xc420045e10 pc=0x459fb2
crypto/cipher.xorBytes(0x7fdedc27affd, 0x3, 0x1003, 0xc420014160, 0x20, 0x20, 0xc42008c000, 0x200, 0x200, 0xc4200800f0)
/localdisk/itocar/golang/src/crypto/cipher/xor.go:55 +0x8e fp=0xc420045e80 sp=0xc420045e20 pc=0x45a0ee
crypto/cipher.(*ctr).XORKeyStream(0xc4200800f0, 0x7fdedc27affd, 0x3, 0x1003, 0xc420014160, 0x20, 0x20)
/localdisk/itocar/golang/src/crypto/cipher/ctr.go:78 +0x18d fp=0xc420045ee8 sp=0xc420045e80 pc=0x459edd
main.main()
/localdisk/itocar/golang/src/foo.go:28 +0x22c fp=0xc420045f80 sp=0xc420045ee8 pc=0x47791c
runtime.main()
/localdisk/itocar/golang/src/runtime/proc.go:185 +0x20d fp=0xc420045fe0 sp=0xc420045f80 pc=0x4285dd
runtime.goexit()
/localdisk/itocar/golang/src/runtime/asm_amd64.s:2337 +0x1 fp=0xc420045fe8 sp=0xc420045fe0 pc=0x450cd1
exit status 2

This problem is due to fastXORBytes writing to dst using unsafe cast into []uintptr
Without protection syscalls fastXORBytes will happily write up-to len(dst)*8 - len(dst) bytes after dst and panic inside fastXORBytes on bound check or alternatively panic in XORKeyStream on
dst = dst[n:]
This isn't that bad unless dst is located at the end of page, in which case panic message is rather unhelpful or users tries to recover, in which case several uncaught out of bounds writes have already happened.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 20, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 20, 2017
@bradfitz
Copy link
Contributor

Marking this Go 1.10 because we can't delay Go 1.9 forever and this isn't a regression, but this is likely a candidate for a cherry-pick back to Go 1.9 later, once fixed.

@gopherbot
Copy link

CL https://golang.org/cl/50690 mentions this issue.

@crvv
Copy link
Contributor

crvv commented Jul 28, 2017

This bug also exists in ctr_s390x.go. xorBytes writes to dst before bounds checking.

@mundaym
Copy link
Member

mundaym commented Jul 28, 2017

@crvv Thanks, yeah it does. I'm happy to send a change unless you want to.

Does anyone have a preference as to whether we do an explicit panic call like CL 50690 does or whether we just force a bounds check like CL 51670 does? We should probably do it the same way everywhere.

@crvv
Copy link
Contributor

crvv commented Jul 29, 2017

In xor.go safeXORBytes, this will panic on a runtime bounds check, and _ = dst[len(src)-1] have same behavior.

@gopherbot
Copy link

Change https://golang.org/cl/52050 mentions this issue: crypto/{aes, cipher}: fix out of bounds write in CTR cipher

@crvv
Copy link
Contributor

crvv commented Aug 1, 2017

On AMD64, this bug also exists in CTR, OFB, CFB, RC4 and x/crypto Salsa20.

@gopherbot
Copy link

Change https://golang.org/cl/52391 mentions this issue: salsa20/salsa: fix out of bounds write

@mundaym
Copy link
Member

mundaym commented Aug 2, 2017

Re-opening because CL 52391 doesn't fix the crypto/cipher package (it fixes the x/crypto/salsa20/salsa package).

@mundaym mundaym reopened this Aug 2, 2017
@FiloSottile
Copy link
Contributor

I'd like to see this fixed (also?) in the common xorBytes function. I don't think we care about when we panic exactly and what we did to the buffer before panic'ing, but I'd like this not to be re-introduced by the next xorBytes caller.

@golang golang locked and limited conversation to collaborators Aug 9, 2018
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
Fixes golang/go#21104

Change-Id: I59054f9e2beed8a0c7efd513eb84795dc0308353
Reviewed-on: https://go-review.googlesource.com/52391
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Fixes golang/go#21104

Change-Id: I59054f9e2beed8a0c7efd513eb84795dc0308353
Reviewed-on: https://go-review.googlesource.com/52391
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
Fixes golang/go#21104

Change-Id: I59054f9e2beed8a0c7efd513eb84795dc0308353
Reviewed-on: https://go-review.googlesource.com/52391
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
Fixes golang/go#21104

Change-Id: I59054f9e2beed8a0c7efd513eb84795dc0308353
Reviewed-on: https://go-review.googlesource.com/52391
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants