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: Server Name Indication. ServerNameListLength field contains length of list in bytes #10793

Closed
v-lavrentikov opened this issue May 12, 2015 · 1 comment
Assignees
Milestone

Comments

@v-lavrentikov
Copy link

@v-lavrentikov v-lavrentikov commented May 12, 2015

Source file: crypto/tls/handshake_messages.go
Function: func (m *clientHelloMsg) unmarshal(data []byte) bool {...}
Code fragment:

switch extension {
    case extensionServerName:
        if length < 2 {
            return false
        }
        numNames := int(data[0])<<8 | int(data[1])
        d := data[2:]
        for i := 0; i < numNames; i++ {
...

In this fragment during processing SNI extension the ServerNameListLength field is processed as a count of list elements (variable numNames). But this field contains the size of list in bytes (tested in wireshark with TLS 1.2).

This code will work in case SNI contains only one element with type HostName in the list, otherwise the handshake will fail.

@minux minux added this to the Go1.5 milestone May 12, 2015
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 14, 2015

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

@agl agl closed this in 6a34206 Jun 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The previous code had a brain fart: it took one of the length prefixes
as an element count, not a length. This didn't actually affect anything
because the loop stops as soon as it finds a hostname element, and the
hostname element is always the first and only element. (No other element
types have ever been defined.)

This change fixes the parsing in case SNI is ever changed in the future.

Fixes golang#10793.

Change-Id: Iafdf3381942bc22b1f33595315c53dc6cc2e9f0f
Reviewed-on: https://go-review.googlesource.com/11059
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
The previous code had a brain fart: it took one of the length prefixes
as an element count, not a length. This didn't actually affect anything
because the loop stops as soon as it finds a hostname element, and the
hostname element is always the first and only element. (No other element
types have ever been defined.)

This change fixes the parsing in case SNI is ever changed in the future.

Fixes golang#10793.

Change-Id: Iafdf3381942bc22b1f33595315c53dc6cc2e9f0f
Reviewed-on: https://go-review.googlesource.com/11059
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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
4 participants
You can’t perform that action at this time.