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

app-layer-ssl: fix memleak #1934

Closed
wants to merge 1 commit into from
Closed

app-layer-ssl: fix memleak #1934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions src/app-layer-ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ SCEnumCharMap tls_decoder_event_table[ ] = {
{ "INVALID_HEARTBEAT_MESSAGE", TLS_DECODER_EVENT_INVALID_HEARTBEAT },
{ "OVERFLOW_HEARTBEAT_MESSAGE", TLS_DECODER_EVENT_OVERFLOW_HEARTBEAT },
{ "DATALEAK_HEARTBEAT_MISMATCH", TLS_DECODER_EVENT_DATALEAK_HEARTBEAT_MISMATCH },
{ "MULTIPLE SNI EXTENSIONS", TLS_DECODER_EVENT_MULTIPLE_SNI_EXTENSIONS },
Copy link
Contributor

Choose a reason for hiding this comment

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

These should not contain spaces but underscores. They are used as 'tls.multiple_sni_extensions' in the app-layer-events rule keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/inliniac/suricata/blob/master/rules/tls-events.rules for how they are used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add rules to rules/tls-events.rules for each of the new events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: coffee first, then pull requests :)

Thanks for the comments. I'll fix!

{ "INVALID SNI TYPE", TLS_DECODER_EVENT_INVALID_SNI_TYPE },
{ "INVALID SNI LENGTH", TLS_DECODER_EVENT_INVALID_SNI_LENGTH },
/* Certificates decoding messages */
{ "INVALID_CERTIFICATE", TLS_DECODER_EVENT_INVALID_CERTIFICATE },
{ "CERTIFICATE_MISSING_ELEMENT", TLS_DECODER_EVENT_CERTIFICATE_MISSING_ELEMENT },
Expand Down Expand Up @@ -205,8 +208,31 @@ static int SSLv3ParseHandshakeType(SSLState *ssl_state, uint8_t *input,
switch (ext_type) {
case SSL_EXTENSION_SNI:
{
/* skip sni_list_length and sni_type */
input += 3;
/* there must not be more than one extension of the same
type (RFC5246 section 7.4.1.4) */
if (ssl_state->curr_connp->sni) {
SCLogDebug("Multiple SNI extensions");
AppLayerDecoderEventsSetEvent(ssl_state->f,
TLS_DECODER_EVENT_MULTIPLE_SNI_EXTENSIONS);
return -1;
}

/* skip sni_list_length */
input += 2;

if (!(HAS_SPACE(1)))
goto end;

uint8_t sni_type = *(input++);

/* currently the only type allowed is host_name
(RFC6066 section 3) */
if (sni_type != SSL_SNI_TYPE_HOST_NAME) {
SCLogDebug("Unknown SNI type");
AppLayerDecoderEventsSetEvent(ssl_state->f,
TLS_DECODER_EVENT_INVALID_SNI_TYPE);
return -1;
}

if (!(HAS_SPACE(2)))
goto end;
Expand All @@ -217,6 +243,16 @@ static int SSLv3ParseHandshakeType(SSLState *ssl_state, uint8_t *input,
if (!(HAS_SPACE(sni_len)))
goto end;

/* host_name contains the fully qualified domain name,
and should therefore be limited by the maximum domain
name length */
if (sni_len > 255) {
SCLogDebug("SNI length >255");
AppLayerDecoderEventsSetEvent(ssl_state->f,
TLS_DECODER_EVENT_INVALID_SNI_LENGTH);
return -1;
}

size_t sni_strlen = sni_len + 1;
ssl_state->curr_connp->sni = SCMalloc(sni_strlen);

Expand Down
6 changes: 6 additions & 0 deletions src/app-layer-ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ enum {
TLS_DECODER_EVENT_INVALID_HEARTBEAT,
TLS_DECODER_EVENT_OVERFLOW_HEARTBEAT,
TLS_DECODER_EVENT_DATALEAK_HEARTBEAT_MISMATCH,
TLS_DECODER_EVENT_MULTIPLE_SNI_EXTENSIONS,
TLS_DECODER_EVENT_INVALID_SNI_TYPE,
TLS_DECODER_EVENT_INVALID_SNI_LENGTH,
/* Certificates decoding messages */
TLS_DECODER_EVENT_INVALID_CERTIFICATE,
TLS_DECODER_EVENT_CERTIFICATE_MISSING_ELEMENT,
Expand Down Expand Up @@ -89,6 +92,9 @@ enum {
/* extensions */
#define SSL_EXTENSION_SNI 0x0000

/* SNI types */
#define SSL_SNI_TYPE_HOST_NAME 0

/* SSL versions. We'll use a unified format for all, with the top byte
* holding the major version and the lower byte the minor version */
enum {
Expand Down