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.

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

authentication issue #2591

Closed
khodadadi opened this issue Jun 1, 2019 · 10 comments
Assignees
Milestone

Comments

@khodadadi
Copy link

@khodadadi khodadadi commented Jun 1, 2019

Environment

  • OS: UBUNTU 18.04
  • Erlang/OTP: 21
  • EMQ: 3.1.1

Description

We use emqttd 2.3 and try to upgrade to emqx 3.1.1
In emqx, when auth module return error (eg., bad_username_or_password) client socket closed before CONNACK packet received. This behaviour cause ddos attack by client.
Is this a bug or did I miss something?
client: mqttjs
client mqtt protocol: MQTT 3.1.1
emqx.conf
allow_anonymous = false

@gilbertwong96 gilbertwong96 self-assigned this Jun 2, 2019
@HJianBo

This comment has been minimized.

Copy link
Member

@HJianBo HJianBo commented Jun 5, 2019

Hi, @khodadadi Thanks for your feedback, but I can not understand this

Why does this behavior cause a DDOS attack?

I think the client process of EMQ X will shut down and release all resources of this after the client closed the connection. All resources of a client will be released.

@khodadadi

This comment has been minimized.

Copy link
Author

@khodadadi khodadadi commented Jun 8, 2019

Hi @HJianBo
1- Client tcp connection opend
2- Client send CONNECT packet with not valid auth data
3- Server send CONNACK pack with reason_code=4 (bad_username_and_password). then Tcp connection closed by server before CONNACK packet received in client and client cannot determine why connection closed, and then retry, retry and retry.
there is no problem in emq 2.3 and connection closed after CONNACK packet received in client.

@khodadadi

This comment has been minimized.

Copy link
Author

@khodadadi khodadadi commented Jun 8, 2019

hi @Gilbert-Wong
this bug only occurs in websocket mode

@tradingtrace

This comment has been minimized.

Copy link
Contributor

@tradingtrace tradingtrace commented Jun 9, 2019

I'm a newcomer to EMQX, just starting to read the code. Please correct me if I have any mistakes.

I guess that when the WebSocket is closed, the CONNACK message is still left in the WS connection process mailbox.

When CONNECT command failed, CONNACK message with the error code will be delivered here

emqx/src/emqx_protocol.erl

Lines 554 to 558 in 5025b2f

connack({ReasonCode, PState = #pstate{proto_ver = ProtoVer, credentials = Credentials}}) ->
ok = emqx_hooks:run('client.connected', [Credentials, ReasonCode, attrs(PState)]),
[ReasonCode1] = reason_codes_compat(connack, [ReasonCode], ProtoVer),
_ = deliver({connack, ReasonCode1}, PState),
{error, emqx_reason_codes:name(ReasonCode1, ProtoVer), PState}.

then return {error, Error, ProtoState1}.

And the deviler function will call the send_fun function which is defined in emqx_ws_connection.erl if we use WebSocket mode.

send_fun(WsPid) ->
fun(Data) ->
BinSize = iolist_size(Data),
emqx_metrics:inc('bytes/sent', BinSize),
put(send_oct, get(send_oct) + BinSize),
put(send_cnt, get(send_cnt) + 1),
WsPid ! {binary, iolist_to_binary(Data)}
end.

And it sends a message to itself WsPid ! {binary, iolist_to_binary(Data)}.

Let's go back to the return value {error, Error, ProtoState1}, it will be returned to emqx_ws_connection.erl line 155

case emqx_protocol:received(Packet, ProtoState) of
{ok, ProtoState1} ->
websocket_handle({binary, Rest}, reset_parser(State#state{proto_state = ProtoState1}));
{error, Error} ->
?WSLOG(error, "Protocol error - ~p", [Error], State),
{stop, State};
{error, Error, ProtoState1} ->
shutdown(Error, State#state{proto_state = ProtoState1});
{stop, Reason, ProtoState1} ->
shutdown(Reason, State#state{proto_state = ProtoState1})
end;

then call the function shutdown to terminate the current process without processing the remaining messages in its mailbox.

So I think that the line 155 or other places like this could be changed to something like WsPid ! {shutdown, Reason}.

PS: In TCP mode, it will call erlang:port_command/3 to send CONNACK to the client directly, not send a message to itself, so it doesn't have this issue.

@gilbertwong96

This comment has been minimized.

Copy link
Contributor

@gilbertwong96 gilbertwong96 commented Jun 10, 2019

Hi, @tradingtrace, Look here:

send_fun(WsPid) ->
fun(Data) ->
BinSize = iolist_size(Data),
emqx_metrics:inc('bytes/sent', BinSize),
put(send_oct, get(send_oct) + BinSize),
put(send_cnt, get(send_cnt) + 1),
WsPid ! {binary, iolist_to_binary(Data)}
end.

The CONNACK packet would be sent to websocket in send_fun.

@tradingtrace

This comment has been minimized.

Copy link
Contributor

@tradingtrace tradingtrace commented Jun 10, 2019

@Gilbert-Wong Yes, you are right. CONNACK packet would be sent to the process WsPid at line 127.
But the process WsPid is a cowboy_websocket process which has a big loop to receive messages.

And here:

emqx/src/emqx_protocol.erl

Lines 554 to 558 in 5025b2f

connack({ReasonCode, PState = #pstate{proto_ver = ProtoVer, credentials = Credentials}}) ->
ok = emqx_hooks:run('client.connected', [Credentials, ReasonCode, attrs(PState)]),
[ReasonCode1] = reason_codes_compat(connack, [ReasonCode], ProtoVer),
_ = deliver({connack, ReasonCode1}, PState),
{error, emqx_reason_codes:name(ReasonCode1, ProtoVer), PState}.

The return value of this function will cause the process WsPid termination directly. There is no change to run loop again to receive the remain messages in its mailbox.

And I made some test, here is the code and the log in debug mode:

%% cowboy_websocket.erl
-spec terminate(#state{}, any(), terminate_reason()) -> no_return().
terminate(State, HandlerState, Reason) ->
    handler_terminate(State, HandlerState, Reason),

    emqx_logger:debug("---> Process ~p terminating, process_info: ~w~n", [self(),
        erlang:process_info(self(), [current_function, message_queue_len])]),
    receive
        Message ->
            emqx_logger:debug("---> The left message: ~p~n", [Message]),
            websocket_send(Message, State)
    after 0 ->
        ignore
    end,
    
    exit(normal).
%% log
2019-06-10 11:06:45.743 [debug] 127.0.0.1:39138 [Protocol] RECV CONNECT(Q0, R0, D0, ClientId=clientId-OKRsaldJvW, ProtoName=MQIsdp, ProtoVsn=3, CleanStart=true, KeepAlive=10, Username=user1, Password=******)
2019-06-10 11:06:45.744 [warning] clientId-OKRsaldJvW@127.0.0.1:39138 [Protocol] Client clientId-OKRsaldJvW (Username: 'user1') login failed for password_error
2019-06-10 11:06:45.745 [debug] clientId-OKRsaldJvW@:39138 ---> WsPid(<0.31663.0>) ! {binary,<<32,2,0,4>>}
2019-06-10 11:06:45.745 [debug] clientId-OKRsaldJvW@127.0.0.1:39138 [Protocol] SEND CONNACK(Q0, R0, D0, AckFlags=0, ReasonCode=4)
2019-06-10 11:06:45.746 [debug] clientId-OKRsaldJvW@127.0.0.1:39138 [WS Connection] Terminated for malformed_username_or_password, sockerror: stop
2019-06-10 11:06:45.746 [debug] clientId-OKRsaldJvW@127.0.0.1:39138 ---> Process <0.31663.0> terminating, process_info: [{current_function,{cowboy_websocket,terminate,3}},{message_queue_len,1}]
2019-06-10 11:06:45.747 [debug] clientId-OKRsaldJvW@127.0.0.1:39138 ---> The left message: {binary,<<32,2,0,4>>}
@gilbertwong96 gilbertwong96 added the BUG label Jun 10, 2019
@gilbertwong96

This comment has been minimized.

Copy link
Contributor

@gilbertwong96 gilbertwong96 commented Jun 10, 2019

I suppose this is the bug of our WebSocket implementation, thanks for your feedback, I'll resolve it as quickly as possible.

@turtleDeng turtleDeng added this to the 3.2-beta.3 milestone Jun 12, 2019
@gilbertwong96

This comment has been minimized.

Copy link
Contributor

@gilbertwong96 gilbertwong96 commented Jun 12, 2019

It has been fixed, here is the pull request. #2615

@gilbertwong96

This comment has been minimized.

Copy link
Contributor

@gilbertwong96 gilbertwong96 commented Jun 12, 2019

@HJianBo

This comment has been minimized.

Copy link
Member

@HJianBo HJianBo commented Jun 14, 2019

Hi, guys! We have released it at v3.2.beta-3, please check it.

Thanks for attention there!

@HJianBo HJianBo closed this Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.