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.

Sign up for GitHub

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

Jump to bottom

fix ssl_next_proto_validate #506

Closed
wants to merge 1 commit into from
Closed

fix ssl_next_proto_validate #506

wants to merge 1 commit into from

Conversation

rhenium
Copy link
Contributor

@rhenium rhenium commented Dec 12, 2015

ssl_next_proto_validate is not validating the length of each advertised protocol name.
This change was introduced by commit 50932c4.

@kroeckx
Copy link
Member

kroeckx commented Dec 12, 2015

It seems like it's also no longer checking the size.

@rhenium
Copy link
Contributor Author

rhenium commented Dec 13, 2015

OpenSSL 1.0.2 doen't allow zero-length protocol name, but current master does.
50932c4#diff-97a3239e0939cd2a1cbc669d027852f1L2352

@kroeckx
Copy link
Member

kroeckx commented Dec 13, 2015

On Sat, Dec 12, 2015 at 08:22:15PM -0800, rhenium wrote:

OpenSSL 1.0.2 doen't allow zero-length protocol name, but current master does.

I know, but 1.0.2 also checks the total size while master doesn't.

@rhenium
Copy link
Contributor Author

rhenium commented Dec 13, 2015

Seems like current master also checks it.

PACKET pkt;
PACKET_buf_init(&pkt, "\002ab\003cd", 6);
if (ssl_next_proto_validate(&pkt)) {
    /* doesn't reach here */
}

@rhenium rhenium force-pushed the fix-ssl_next_proto_validate branch from acee7ea to 1750c17 Compare March 29, 2016 15:48
@rhenium
Copy link
Contributor Author

rhenium commented Mar 29, 2016

The current code does correctly check the total length.

Since the new (current) ssl_next_proto_validate()[1] reads to the end, there is no need to worry about the extra padding in pkt.
If the data contains invalid length, that is, if len is too big, PACKET_forward() will fail.
The length of pkt itself is already validated by the caller, ssl_scan_serverhello_tlsext()[2].

[1]

openssl/ssl/t1_lib.c

Line 2334 in bbe9769

static char ssl_next_proto_validate(PACKET *pkt)

[2]

openssl/ssl/t1_lib.c

Line 2390 in bbe9769

|| !PACKET_peek_bytes(&spkt, &data, size))

@rhenium
Copy link
Contributor Author

rhenium commented Mar 29, 2016

Rebased and updated commit message.

ekasper
ekasper reviewed Mar 29, 2016
View reviewed changes
ssl/t1_lib.c
@@ -2337,6 +2337,7 @@ static char ssl_next_proto_validate(PACKET *pkt)

while (PACKET_remaining(pkt)) {
if (!PACKET_get_1(pkt, &len)
|| !len
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should in fact be using PACKET_get_length_prefixed_1 for reading length-prefixed packets. Then just check that the subpacket is not empty. Would you mind changing to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I didn't know it. I'll update soon.

@rhenium rhenium force-pushed the fix-ssl_next_proto_validate branch from 1750c17 to 87e735e Compare March 29, 2016 18:39
ekasper
ekasper reviewed Mar 30, 2016
View reviewed changes
ssl/t1_lib.c
if (!PACKET_get_1(pkt, &len)
|| !PACKET_forward(pkt, len))
if (!PACKET_get_length_prefixed_1(pkt, &tmp_protocol)
|| !PACKET_remaining(&tmp_protocol))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could write PACKET_remaining(&tmp_protocol) != 0. (Not a boolean return.)

Otherwise +1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and pushed.

@rhenium
Fix NPN protocol name list validation
5fd6db0 
Since 50932c4 "PACKETise ServerHello processing",
ssl_next_proto_validate() incorrectly allows empty protocol name.
draft-agl-tls-nextprotoneg-04[1] says "Implementations MUST ensure that
the empty string is not included and that no byte strings are
truncated."
This patch restores the old correct behavior.

[1] https://tools.ietf.org/html/draft-agl-tls-nextprotoneg-04
@rhenium rhenium force-pushed the fix-ssl_next_proto_validate branch from 87e735e to 5fd6db0 Compare March 30, 2016 10:17
@ekasper
Copy link
Contributor

ekasper commented Mar 30, 2016

+1 from me.

@richsalz
Copy link
Contributor

So @kroeckx are you okay with this?

@levitte
Copy link
Member

levitte commented May 6, 2016

Ping.

@mattcaswell
Copy link
Member

+1

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label May 16, 2016
@mattcaswell
Copy link
Member

Merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rhenium @kroeckx @ekasper @richsalz @levitte @mattcaswell