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

SASL: handle fragmentation #506

Merged
merged 1 commit into from Sep 26, 2016

Conversation

Projects
None yet
4 participants
@kruton
Contributor

kruton commented Jul 4, 2016

The IRCv3 SASL extension says that AUTHENTICATION payloads of exactly
400 bytes in length indicate that the message is fragmented and will
continue in a subsequent message. Handle the reassembly and splitting of
these messages so that we are compliant with the specification.

@dequis

This comment has been minimized.

Member

dequis commented Jul 4, 2016

Travis failed because you used g_string_free_to_bytes which was added in glib 2.34, a bit too recent.

/* Check if there is an existing fragment to prepend. */
buffer = g_hash_table_lookup(sasl_buffers, server->tag);
if (buffer != NULL) {
if (!g_strcmp0("+", enc_req)) {

This comment has been minimized.

@LemonBoy

LemonBoy Jul 4, 2016

Member

Use an explicit comparison (== 0), same as below.

@kruton

This comment has been minimized.

Contributor

kruton commented Jul 5, 2016

Okay, think my latest commit in this PR addresses the comment.

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Jul 6, 2016

Why not use a GBytes in sasl_buffers instead of a plain string ? And what do you think about adding a upper limit on the data that we collect ?

if (g_strcmp0("+", enc_req) == 0) {
enc_req = buffer;
} else {
enc_req = g_strconcat(buffer, enc_req, NULL);

This comment has been minimized.

@dequis

dequis Jul 6, 2016

Member

This replaces the received end_req with a longer version, so the check for strlen(enc_req) == AUTHENTICATE_CHUNK_SIZE below will never pass after the first chunk.

It also sets a string that should be freed (and is leaking) in a variable that shouldn't.

enc_req = g_base64_encode((const guchar *)req->str, req->len);
/* This could be g_string_free_to_bytes */
resp_len = resp->len;
resp_bytes = g_bytes_new_take(g_string_free(resp, FALSE), resp_len);

This comment has been minimized.

@dequis

dequis Jul 6, 2016

Member

Added in glib 2.32 (current requirement is 2.16, shouldn't be bumped past 2.28 because rhel 6)

return;
}
p = g_bytes_get_data(response, &len);

This comment has been minimized.

@dequis

dequis Jul 6, 2016

Member

Added in glib 2.32, ditto.

@dequis

This comment has been minimized.

Member

dequis commented Jul 6, 2016

Uh, you know, just avoid GBytes entirely. I thought it was some functions but all of it was added in glib 2.32

@kruton

This comment has been minimized.

Contributor

kruton commented Jul 7, 2016

Doh! I rewrote this using GString instead. I also wrote unit tests to confirm everything works as expected.

@kruton

This comment has been minimized.

Contributor

kruton commented Jul 8, 2016

I removed a now-superfluous comment; should be ready for another look.

@dequis

This comment has been minimized.

Member

dequis commented Jul 8, 2016

Instead of using a hash table from tag to gstring, you could just add a GString * field to IRC_SERVER_REC, which would also make it easier to free the GString buffer if the authentication is interrupted for whatever reason. Also, I think that in those cases with the current version the partial sasl payload would persist across disconnections. That's not desirable.

Also +1 for an upper limit to stop servers from making this buffer grow forever. Atheme seems to use 8192. https://github.com/atheme/atheme/blob/59ca306d8faf1b7a69a82e3d37bb3c751e680f13/modules/saslserv/main.c#L318

SASL: handle fragmentation
The IRCv3 SASL extension says that AUTHENTICATION payloads of exactly
400 bytes in length indicate that the message is fragmented and will
continue in a subsequent message. Handle the reassembly and splitting of
these messages so that we are compliant with the specification.
@kruton

This comment has been minimized.

Contributor

kruton commented Aug 30, 2016

Added the SASL buffer to IRC_SERVER_REC and set an upper limit to 8192.

@ailin-nemui ailin-nemui merged commit 8d4d313 into irssi:master Sep 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment