Skip to content

Commit

Permalink
Properly handling stream/crypto frames while tracing
Browse files Browse the repository at this point in the history 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20914)
  • Loading branch information
@mattcaswell
mattcaswell committed May 24, 2023
1 parent 8aff8f8 commit b09e246
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 33 deletions.
12 changes: 9 additions & 3 deletions include/internal/quic_wire.h
View file
Expand Up @@ -557,9 +557,11 @@ int ossl_quic_wire_decode_frame_stop_sending(PACKET *pkt,
* Decodes a QUIC CRYPTO frame.
*
* f->data is set to point inside the packet buffer inside the PACKET, therefore
* it is safe to access for as long as the packet buffer exists.
* it is safe to access for as long as the packet buffer exists. If nodata is
* set to 1 then reading the PACKET stops after the frame header and f->data is
* set to NULL.
*/
int ossl_quic_wire_decode_frame_crypto(PACKET *pkt,
int ossl_quic_wire_decode_frame_crypto(PACKET *pkt, int nodata,
OSSL_QUIC_FRAME_CRYPTO *f);

/*
Expand All @@ -573,6 +575,10 @@ int ossl_quic_wire_decode_frame_new_token(PACKET *pkt,
/*
* Decodes a QUIC STREAM frame.
*
* If nodata is set to 1 then reading the PACKET stops after the frame header
* and f->data is set to NULL. In this case f->len will also be 0 in the event
* that "has_explicit_len" is 0.
*
* If the frame did not contain an offset field, f->offset is set to 0, as the
* absence of an offset field is equivalent to an offset of 0.
*
Expand All @@ -595,7 +601,7 @@ int ossl_quic_wire_decode_frame_new_token(PACKET *pkt,
* f->is_fin is set according to whether the frame was marked as ending the
* stream.
*/
int ossl_quic_wire_decode_frame_stream(PACKET *pkt,
int ossl_quic_wire_decode_frame_stream(PACKET *pkt, int nodata,
OSSL_QUIC_FRAME_STREAM *f);

/*
Expand Down
4 changes: 2 additions & 2 deletions ssl/quic/quic_rx_depack.c
View file
Expand Up @@ -196,7 +196,7 @@ static int depack_do_frame_crypto(PACKET *pkt, QUIC_CHANNEL *ch,

*datalen = 0;

if (!ossl_quic_wire_decode_frame_crypto(pkt, &f)) {
if (!ossl_quic_wire_decode_frame_crypto(pkt, 0, &f)) {
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_FRAME_ENCODING_ERROR,
OSSL_QUIC_FRAME_TYPE_CRYPTO,
Expand Down Expand Up @@ -395,7 +395,7 @@ static int depack_do_frame_stream(PACKET *pkt, QUIC_CHANNEL *ch,

*datalen = 0;

if (!ossl_quic_wire_decode_frame_stream(pkt, &frame_data)) {
if (!ossl_quic_wire_decode_frame_stream(pkt, 0, &frame_data)) {
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_FRAME_ENCODING_ERROR,
frame_type,
Expand Down
24 changes: 11 additions & 13 deletions ssl/quic/quic_trace.c
View file
Expand Up @@ -110,7 +110,7 @@ static int frame_crypto(BIO *bio, PACKET *pkt)
{
OSSL_QUIC_FRAME_CRYPTO frame_data;

if (!ossl_quic_wire_decode_frame_crypto(pkt, &frame_data))
if (!ossl_quic_wire_decode_frame_crypto(pkt, 1, &frame_data))
return 0;

BIO_printf(bio, " Offset: %lu\n", frame_data.offset);
Expand Down Expand Up @@ -173,12 +173,19 @@ static int frame_stream(BIO *bio, PACKET *pkt, uint64_t frame_type)
return 0;
}

if (!ossl_quic_wire_decode_frame_stream(pkt, &frame_data))
if (!ossl_quic_wire_decode_frame_stream(pkt, 1, &frame_data))
return 0;

BIO_printf(bio, " Stream id: %lu\n", frame_data.stream_id);
BIO_printf(bio, " Offset: %lu\n", frame_data.offset);
BIO_printf(bio, " Len: %lu\n", frame_data.len);
/*
* It would be nice to find a way of passing the implicit length through
* to the msg_callback. But this is not currently possible.
*/
if (frame_data.has_explicit_len)
BIO_printf(bio, " Len: %lu\n", frame_data.len);
else
BIO_puts(bio, " Len: <implicit length>\n");

return 1;
}
Expand Down Expand Up @@ -532,6 +539,7 @@ int ossl_quic_trace(int write_p, int version, int content_type,

case SSL3_RT_QUIC_FRAME_PADDING:
case SSL3_RT_QUIC_FRAME_FULL:
case SSL3_RT_QUIC_FRAME_HEADER:
{
BIO_puts(bio, write_p ? "Sent" : "Received");
BIO_puts(bio, " Frame: ");
Expand All @@ -545,16 +553,6 @@ int ossl_quic_trace(int write_p, int version, int content_type,
}
break;

case SSL3_RT_QUIC_FRAME_HEADER:
{
BIO_puts(bio, write_p ? "Sent" : "Received");
BIO_puts(bio, " Frame Data\n");

/* TODO(QUIC): Implement me */
BIO_puts(bio, " <content skipped>\n");
}
break;

default:
/* Unrecognised content_type. We defer to SSL_trace */
return 0;
Expand Down
33 changes: 23 additions & 10 deletions ssl/quic/quic_wire.c
View file
Expand Up @@ -591,6 +591,7 @@ int ossl_quic_wire_decode_frame_stop_sending(PACKET *pkt,
}

int ossl_quic_wire_decode_frame_crypto(PACKET *pkt,
int nodata,
OSSL_QUIC_FRAME_CRYPTO *f)
{
if (!expect_frame_header(pkt, OSSL_QUIC_FRAME_TYPE_CRYPTO)
Expand All @@ -599,13 +600,17 @@ int ossl_quic_wire_decode_frame_crypto(PACKET *pkt,
|| f->len > SIZE_MAX /* sizeof(uint64_t) > sizeof(size_t)? */)
return 0;

if (PACKET_remaining(pkt) < f->len)
return 0;
if (nodata) {
f->data = NULL;
} else {
if (PACKET_remaining(pkt) < f->len)
return 0;

f->data = PACKET_data(pkt);
f->data = PACKET_data(pkt);

if (!PACKET_forward(pkt, (size_t)f->len))
return 0;
if (!PACKET_forward(pkt, (size_t)f->len))
return 0;
}

return 1;
}
Expand Down Expand Up @@ -633,6 +638,7 @@ int ossl_quic_wire_decode_frame_new_token(PACKET *pkt,
}

int ossl_quic_wire_decode_frame_stream(PACKET *pkt,
int nodata,
OSSL_QUIC_FRAME_STREAM *f)
{
uint64_t frame_type;
Expand All @@ -658,14 +664,21 @@ int ossl_quic_wire_decode_frame_stream(PACKET *pkt,
if (!PACKET_get_quic_vlint(pkt, &f->len))
return 0;
} else {
f->len = PACKET_remaining(pkt);
if (nodata)
f->len = 0;
else
f->len = PACKET_remaining(pkt);
}

f->data = PACKET_data(pkt);
if (nodata) {
f->data = NULL;
} else {
f->data = PACKET_data(pkt);

if (f->len > SIZE_MAX /* sizeof(uint64_t) > sizeof(size_t)? */
|| !PACKET_forward(pkt, (size_t)f->len))
return 0;
if (f->len > SIZE_MAX /* sizeof(uint64_t) > sizeof(size_t)? */
|| !PACKET_forward(pkt, (size_t)f->len))
return 0;
}

return 1;
}
Expand Down
4 changes: 2 additions & 2 deletions test/quic_txp_test.c
View file
Expand Up @@ -1337,7 +1337,7 @@ static int run_script(const struct script_op *script)
goto err;
break;
case OSSL_QUIC_FRAME_TYPE_CRYPTO:
if (!TEST_true(ossl_quic_wire_decode_frame_crypto(&h.pkt, &h.frame.crypto)))
if (!TEST_true(ossl_quic_wire_decode_frame_crypto(&h.pkt, 0, &h.frame.crypto)))
goto err;
break;

Expand All @@ -1349,7 +1349,7 @@ static int run_script(const struct script_op *script)
case OSSL_QUIC_FRAME_TYPE_STREAM_OFF_FIN:
case OSSL_QUIC_FRAME_TYPE_STREAM_OFF_LEN:
case OSSL_QUIC_FRAME_TYPE_STREAM_OFF_LEN_FIN:
if (!TEST_true(ossl_quic_wire_decode_frame_stream(&h.pkt, &h.frame.stream)))
if (!TEST_true(ossl_quic_wire_decode_frame_stream(&h.pkt, 0, &h.frame.stream)))
goto err;
break;

Expand Down
6 changes: 3 additions & 3 deletions test/quic_wire_test.c
View file
Expand Up @@ -261,7 +261,7 @@ static int encode_case_6_dec(PACKET *pkt, ossl_ssize_t fail)
{
OSSL_QUIC_FRAME_CRYPTO f = {0};

if (!TEST_int_eq(ossl_quic_wire_decode_frame_crypto(pkt, &f), fail < 0))
if (!TEST_int_eq(ossl_quic_wire_decode_frame_crypto(pkt, 0, &f), fail < 0))
return 0;

if (fail >= 0)
Expand Down Expand Up @@ -358,7 +358,7 @@ static int encode_case_8_dec(PACKET *pkt, ossl_ssize_t fail)
*/
return 1;

if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, &f), fail < 0))
if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, 0, &f), fail < 0))
return 0;

if (fail >= 0)
Expand Down Expand Up @@ -413,7 +413,7 @@ static int encode_case_9_dec(PACKET *pkt, ossl_ssize_t fail)
{
OSSL_QUIC_FRAME_STREAM f = {0};

if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, &f), fail < 0))
if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, 0, &f), fail < 0))
return 0;

if (fail >= 0)
Expand Down

0 comments on commit b09e246

Please sign in to comment.