998524 - SNI handling for NameType is lame

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

This is pretty harmless, but the code in ssl3_HandleServerNameXtn attempts to implement the "at most one SNI of each type" rule from RFC 6066 and fails.

The offending code compares the NameType value from the each entry in the extension with a SECItem::type that hasn't been set.  Since the SECItem is zeroed on allocation, this coincidentally works for the defined "host_name" type, because that compares successfully with SECItemType::siBuffer.

This means that a value with a NameType other than host_name forces an otherwise valid host_name value that appears after it to be ignored.

Also, the allocation is unnecessarily large when there are duplicate types.

The API currently does not convey the NameType and is incapable of doing so safely.  Overloading SECItemType might work, but that seems like a bad idea.

My suggested solution is to ignore all non-host_name SNI values and to defer support for other SNI NameType values.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Patch at: https://codereview.appspot.com/269100043

The above patch is based on the one in bug 1210484. It builds fine and passes ssl_gtests.

What do you think? As the callback receives an array of SECItems and (as you described above) can't figure out the NameType of an entry, it doesn't seem to make sense to try and support multiple NameTypes if we don't know what a future API could look like.

I wasn't sure how to write a test for this, after all I think we can't find out which SNI entry the server picked - so we can't confirm the choice.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

New patch up at: https://codereview.appspot.com/269100043

Comment 3

[Martin Thomson [:mt:]]

Comments on rietveld.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Eric: do you think you can take a look at this soon? Should I ask someone else?

Comment 5

[Eric Rescorla (:ekr)]

I'm at IETF this week. Will try to take a look next week

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

BTW, if this conflicts with the TLS 1.3 patch you have we can totally postpone or discard it.

Comment 7

[Martin Thomson [:mt:]]

I think that this is just a matter of priority.  ekr is looking at landing TLS 1.3, both spec and implementation.  I don't think that this is urgent though.  I'm sure we'll get around to it.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Rebased now that the TLS 1.3 patch landed, no changes needed. EKR, any chance you could take a look?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/ac7580602cef