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/tls: limit number of consecutive warning alert #22543

Closed
JinWuZhao opened this issue Nov 2, 2017 · 6 comments
Closed

crypto/tls: limit number of consecutive warning alert #22543

JinWuZhao opened this issue Nov 2, 2017 · 6 comments

Comments

@JinWuZhao
Copy link

@JinWuZhao JinWuZhao commented Nov 2, 2017

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

1.9.1

Does this issue reproduce with the latest release?

I don't know how to reproduce this issue.

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

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\jinzhao\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\jinzhao\AppData\Local\Temp\go-build649610695=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

What did you do?

My clients scanned their server which running reverse proxy written in go by third party security organization. They got the SSL-Death-Alert vulnerability CVE-2016-8610. This is the detail: https://security.360.cn/cve/CVE-2016-8610 .

I hope this vulnerability can be fixed. Now I use other reverse proxy to replace my own implementation.

@nussjustin

This comment has been minimized.

Copy link
Contributor

@nussjustin nussjustin commented Nov 2, 2017

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 2, 2017

Go's TLS implementation does not use OpenSSL and you linked to a CVE for OpenSSL. I assume the scanner has a false positive.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 2, 2017

Technically, the Go stack exhibits the same behavior: warning alerts are dropped on the floor unlimitedly, so an attacker can keep sending them and keep the machine busy. So it's not a false-positive to be fair.

go/src/crypto/tls/conn.go

Lines 674 to 678 in 1cb86e2

switch data[0] {
case alertLevelWarning:
// drop on the floor
c.in.freeBlock(b)
goto Again

The OpenSSL team did not consider this a security vulnerability, and I don't either. There's a number of ways to just keep sending traffic to a server. I'm not even sure this is a more efficient attack than just sending a lot of ClientHello.

However, I guess this might bypass TCP or HTTP layer rate-limits. We can add a counter like OpenSSL did, if nothing else to make scanners happy.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 2, 2017

Title suggestion: crypto/tls: limit number of consecutive warning alerts.

I wouldn't tag this Security.

@JinWuZhao JinWuZhao changed the title CVE-2016-8610: SSL-Death-Alert vulnerability in TLS crypto/tls: limit number of consecutive warning alert Nov 2, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Nov 2, 2017
@filewalkwithme

This comment has been minimized.

Copy link
Contributor

@filewalkwithme filewalkwithme commented Nov 3, 2017

Hi, if it's ok for everyone, I would like to work on this issue.

I created a CL to address this, implementing an warning alert counter limit in the same way that OpenSSL did.

For now, the tests are passing, but if there are any problems, please let me know.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 3, 2017

Change https://golang.org/cl/75750 mentions this issue: crypto/tls: limit number of consecutive warning alerts

JinWuZhao pushed a commit to imyoudu/go that referenced this issue Nov 3, 2017
@gopherbot gopherbot closed this in d8ee5d1 Nov 8, 2017
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
In the current implementation, it is possible for a client to
continuously send warning alerts, which are just dropped on the floor
inside readRecord.

This can enable scenarios in where someone can try to continuously
send warning alerts to the server just to keep it busy.

This CL implements a simple counter that triggers an error if
we hit the warning alert limit.

Fixes golang#22543

Change-Id: Ief0ca10308cf5a4dea21a5a67d3e8f6501912da6
Reviewed-on: https://go-review.googlesource.com/75750
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
In the current implementation, it is possible for a client to
continuously send warning alerts, which are just dropped on the floor
inside readRecord.

This can enable scenarios in where someone can try to continuously
send warning alerts to the server just to keep it busy.

This CL implements a simple counter that triggers an error if
we hit the warning alert limit.

Fixes golang#22543

Change-Id: Ief0ca10308cf5a4dea21a5a67d3e8f6501912da6
Reviewed-on: https://go-review.googlesource.com/75750
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.