wandenberg / nginx-push-stream-module

A pure stream http push technology for your Nginx setup. Comet made easy and really scalable.
Other
2.22k stars 295 forks source link

Fixed simple DoS in message formatting #161

Closed buglloc closed 10 years ago

buglloc commented 10 years ago

I've found easy way to DoS attack with not very large text message. I think the problem in two things: 1 Replacement pattern ~text~ is not the last step when formatting the message 2 Recursion ngx_http_push_stream_str_replace

Look at ngx_http_push_stream_format_message:

    txt = ngx_http_push_stream_str_replace(message_template, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_ID, char_id, 0, temp_pool);
    txt = ngx_http_push_stream_str_replace(txt, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_EVENT_ID, event_id, 0, temp_pool);
    txt = ngx_http_push_stream_str_replace(txt, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_EVENT_TYPE, event_type, 0, temp_pool);
    txt = ngx_http_push_stream_str_replace(txt, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_CHANNEL, channel_id, 0, temp_pool);
    txt = ngx_http_push_stream_str_replace(txt, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_TEXT, text, 0, temp_pool);
    txt = ngx_http_push_stream_str_replace(txt, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_TIME, time, 0, temp_pool);
    txt = ngx_http_push_stream_str_replace(txt, &NGX_HTTP_PUSH_STREAM_TOKEN_MESSAGE_TAG, tag, 0, temp_pool);

After replacing the ~text~ is replacing ~time~ and ~tag~, so if the message containts ~time~ or ~tag~ these two patterns will also be replaced. And now look at ngx_http_push_stream_str_replace:

        u_char *ret = (u_char *) ngx_strnstr(org->data + offset, (char *) find->data, org->len - offset);
        if (ret != NULL) {
            ngx_str_t *tmp = ngx_http_push_stream_create_str(pool, org->len + replace->len - find->len);
            if (tmp == NULL) {
                ngx_log_error(NGX_LOG_ERR, pool->log, 0, "push stream module: unable to allocate memory to apply text replace");
                return NULL;
            }

            off_t offset_found = ret - org->data;
            ngx_memcpy(tmp->data, org->data, offset_found);
            ngx_memcpy(tmp->data + offset_found, replace->data, replace->len);
            ngx_memcpy(tmp->data + offset_found + replace->len, org->data + offset_found + find->len, org->len - offset_found - find->len);

            result = ngx_http_push_stream_str_replace(tmp, find, replace, offset_found + replace->len, pool);
        }

For each occurrence of a pattern in a string, allocate memory for result string. So if there will be many occurrences - it takes a decent amount of time and memory. For example... Send message:

$ ruby -e 'print "~time~"*30000' | http 192.168.1.229/pub id==testchannel

And watch in htop: 2014-10-16-220325

Of course, changing the order in ngx_http_push_stream_format_message enough to solve the problem, but PR included fixes for both problem.

wandenberg commented 10 years ago

@buglloc I just not committed the refactor removing the recursion, since it does not really solve the problem and I'm going to change the way the message is applied to the template. Thanks for reporting this problem.