The BadCBCPadding255 test from bogo failed because at most 255 trailing
bytes were checked, but for a padding of 255 there are 255 padding bytes
plus 1 length byte with value 255.
Change-Id: I7dd237c013d2c7c8599067246e31b7ba93106cf7
Reviewed-on: https://go-review.googlesource.com/68070
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
An io.Reader does not guarantee that it will read in the entire buffer.
To ensure that property, io.ReadFull should be used instead.
Change-Id: I0b863135ab9abc40e813f9dac07bfb2a76199950
Reviewed-on: https://go-review.googlesource.com/37403
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Run-TryBot: Mikio Hara <mikioh.mikioh@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
There's no need to hold the handshake lock across this call and it can
lead to deadlocks if the net.Conn calls back into the tls.Conn.
Fixes#18426.
Change-Id: Ib1b2813cce385949d970f8ad2e52cfbd1390e624
Reviewed-on: https://go-review.googlesource.com/36561
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The aim is to make the decrypt() timing profile constant, irrespective of
the CBC padding length or correctness. The old algorithm, on valid padding,
would only MAC bytes up to the padding length threshold, making CBC
ciphersuites vulnerable to plaintext recovery attacks as presented in the
"Lucky Thirteen" paper.
The new algorithm Write()s to the MAC all supposed payload, performs a
constant time Sum()---which required implementing a constant time Sum() in
crypto/sha1, see the "Lucky Microseconds" paper---and then Write()s the rest
of the data. This is performed whether the padding is good or not.
This should have no explicit secret-dependent timings, but it does NOT
attempt to normalize memory accesses to prevent cache timing leaks.
Updates #13385
Change-Id: I15d91dc3cc6eefc1d44f317f72ff8feb0a9888f7
Reviewed-on: https://go-review.googlesource.com/18130
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
In Go 1.0, the Config struct consisted only of exported fields.
In Go 1.1, it started to grow private, uncopyable fields (sync.Once,
sync.Mutex, etc).
Ever since, people have been writing their own private Config.Clone
methods, or risking it and doing a language-level shallow copy and
copying the unexported sync variables.
Clean this up and export the Config.clone method as Config.Clone.
This matches the convention of Template.Clone from text/template and
html/template at least.
Fixes#15771
Updates #16228 (needs update in x/net/http2 before fixed)
Updates #16492 (not sure whether @agl wants to do more)
Change-Id: I48c2825d4fef55a75d2f99640a7079c56fce39ca
Reviewed-on: https://go-review.googlesource.com/28075
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
The RFC is clear that the Parameters in an AlgorithmIdentifer for an RSA
public key must be NULL. BoringSSL enforces this so we have strong
evidence that this is a widely compatible change.
Embarrassingly enough, the major source of violations of this is us. Go
used to get this correct in only one of two places. This was only fixed
in 2013 (with 4874bc9b). That's why lots of test certificates are
updated in this change.
Fixes#16166.
Change-Id: Ib9a4551349354c66e730d44eb8cee4ec402ea8ab
Reviewed-on: https://go-review.googlesource.com/27312
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This fixes some 40 warnings from go vet.
Fixes#16134.
Change-Id: Ib9fcba275fe692f027a2a07b581c8cf503b11087
Reviewed-on: https://go-review.googlesource.com/24287
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
The current code, introduced after Go 1.6 to improve latency on
low-bandwidth connections, sends 1 kB packets until 1 MB has been sent,
and then sends 16 kB packets (the maximum record size).
Unfortunately this decreases throughput for 1-16 MB responses by 20% or so.
Following discussion on #15713, change cutoff to 128 kB sent
and also grow the size allowed for successive packets:
1 kB, 2 kB, 3 kB, ..., 15 kB, 16 kB.
This fixes the throughput problems: the overhead is now closer to 2%.
I hope this still helps with latency but I don't have a great way to test it.
At the least, it's not worse than Go 1.6.
Comparing MaxPacket vs DynamicPacket benchmarks:
name maxpkt time/op dyn. time/op delta
Throughput/1MB-8 5.07ms ± 7% 5.21ms ± 7% +2.73% (p=0.023 n=16+16)
Throughput/2MB-8 15.7ms ±201% 8.4ms ± 5% ~ (p=0.604 n=20+16)
Throughput/4MB-8 14.3ms ± 1% 14.5ms ± 1% +1.53% (p=0.000 n=16+16)
Throughput/8MB-8 26.6ms ± 1% 26.8ms ± 1% +0.47% (p=0.003 n=19+18)
Throughput/16MB-8 51.0ms ± 1% 51.3ms ± 1% +0.47% (p=0.000 n=20+20)
Throughput/32MB-8 100ms ± 1% 100ms ± 1% +0.24% (p=0.033 n=20+20)
Throughput/64MB-8 197ms ± 0% 198ms ± 0% +0.56% (p=0.000 n=18+7)
The small MB runs are bimodal in both cases, probably GC pauses.
But there's clearly no general slowdown anymore.
Fixes#15713.
Change-Id: I5fc44680ba71812d24baac142bceee0e23f2e382
Reviewed-on: https://go-review.googlesource.com/23487
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Currently, if a client of crypto/tls (e.g., net/http, http2) calls
tls.Conn.Write with a 33KB buffer, that ends up writing three TLS
records: 16KB, 16KB, and 1KB. Slow clients (such as 2G phones) must
download the first 16KB record before they can decrypt the first byte.
To improve latency, it's better to send smaller TLS records. However,
sending smaller records adds overhead (more overhead bytes and more
crypto calls), which slightly hurts throughput.
A simple heuristic, implemented in this change, is to send small
records for new connections, then boost to large records after the
first 1MB has been written on the connection.
Fixes#14376
Change-Id: Ice0f6279325be6775aa55351809f88e07dd700cd
Reviewed-on: https://go-review.googlesource.com/19591
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
Reviewed-by: Adam Langley <agl@golang.org>
With this in place, a TLS server is capable of selecting the correct
certificate based on the client's ServerNameIndication extension.
The need to call Config.BuildNameToCertificate is unfortunate, but
adding a sync.Once to the Config structure made it uncopyable and I
felt that was too high a price to pay. Parsing the leaf certificates
in each handshake was too inefficient to consider.
R=bradfitz, rsc
CC=golang-dev
https://golang.org/cl/5151048
This is largely based on ality's CL 2747042.
crypto/rc4: API break in order to conform to crypto/cipher's
Stream interface
cipher/cipher: promote to the default build
Since CBC differs between TLS 1.0 and 1.1, we downgrade and
support only 1.0 at the current time. 1.0 is what most of the
world uses.
Given this CL, it would be trival to add support for AES 256,
SHA 256 etc, but I haven't in order to keep the change smaller.
R=rsc
CC=ality, golang-dev
https://golang.org/cl/3659041
