ejabberd development
  1. ejabberd development
  2. EJAB-1498

Fix Denial of Service when user sends malformed <publish> stanza

    Details

      Description

      Install ejabberd 2.1.8 with empty database and default config. Create user, login and send the following stanza:

      <iq id="hmARY-12" to="pubsub.localhost" type="set">
      <pubsub xmlns="http://jabber.org/protocol/pubsub">
      <publish>
      <item>
      <body>Hi!</body>
      </item>
      </publish>
      </pubsub>
      </iq>
      

      This stanza is not correct: "node" attribute of "publish" tag is omitted.

      ejabberd will enter an infinite loop, filling database with randomly named nodes. CPU usage will jump to 100%.

      This is a pretty nasty bug since any user can cause DoS with just a single stanza!

      The problem is in mod_pubsub behavior. If no target node specified, it creates random node and immediately forgets its name, instead of publishing to it. The following patch fixes the bug:

      diff --git a/src/mod_pubsub/mod_pubsub.erl b/src/mod_pubsub/mod_pubsub.erl
      index e4441f6..020c3b2 100644
      --- a/src/mod_pubsub/mod_pubsub.erl
      +++ b/src/mod_pubsub/mod_pubsub.erl
      @@ -2098,8 +2098,9 @@ publish_item(Host, ServerHost, Node, Publisher, ItemId, Payload) ->
                  case lists:member("auto-create", features(Type)) of
                      true ->
                          case create_node(Host, ServerHost, Node, Publisher, Type) of
      -                       {result, _} ->
      -                           publish_item(Host, ServerHost, Node, Publisher, ItemId, Payload);
      +                       {result, [{xmlelement, "pubsub", [{"xmlns", ?NS_PUBSUB}],
      +                                   [{xmlelement, "create", [{"node", NewNode}], []}]}]} ->
      +                           publish_item(Host, ServerHost, list_to_binary(NewNode), Publisher, ItemId, Payload);
                              _ ->
                                  {error, ?ERR_ITEM_NOT_FOUND}
                          end;
      diff --git a/src/mod_pubsub/mod_pubsub_odbc.erl b/src/mod_pubsub/mod_pubsub_odbc.erl
      index 4649aac..3b02c17 100644
      --- a/src/mod_pubsub/mod_pubsub_odbc.erl
      +++ b/src/mod_pubsub/mod_pubsub_odbc.erl
      @@ -1914,8 +1914,9 @@ publish_item(Host, ServerHost, Node, Publisher, ItemId, Payload) ->
                  case lists:member("auto-create", features(Type)) of
                      true ->
                          case create_node(Host, ServerHost, Node, Publisher, Type) of
      -                       {result, _} ->
      -                           publish_item(Host, ServerHost, Node, Publisher, ItemId, Payload);
      +                       {result, [{xmlelement, "pubsub", [{"xmlns", ?NS_PUBSUB}],
      +                                  [{xmlelement, "create", [{"node", NewNode}], []}]}]} ->
      +                           publish_item(Host, ServerHost, list_to_binary(NewNode), Publisher, ItemId, Payload);
                              _ ->
                                  {error, ?ERR_ITEM_NOT_FOUND}
                          end;
      

        Activity

        Hide
        Christophe Romain
        added a comment - - edited

        thanks reporting that issue, we're working on it.
        the patch seems correct. thanks !

        Show
        Christophe Romain
        added a comment - - edited thanks reporting that issue, we're working on it. the patch seems correct. thanks !
        Hide
        Christophe Romain
        added a comment -

        patch applied and commited with thanks !

        Show
        Christophe Romain
        added a comment - patch applied and commited with thanks !
        Hide
        Oleg Smirnov
        added a comment -

        Wow, that was fast! Thanks for quick reaction.

        Show
        Oleg Smirnov
        added a comment - Wow, that was fast! Thanks for quick reaction.
        Hide
        Dingding Ye
        added a comment -

        With the fixed code, after a PEP node created, it will reply an error instead of publish_item. That's because the create_node response is

        {result, []}.

        So we should either allow {result, []}

        in publish_item or create_node which I prefer to fix in create_node. The diff is as follows,

        diff --git a/src/mod_pubsub/mod_pubsub.erl b/src/mod_pubsub/mod_pubsub.erl
        index d7643c2..f884990 100644
        — a/src/mod_pubsub/mod_pubsub.erl
        +++ b/src/mod_pubsub/mod_pubsub.erl
        @@ -1789,6 +1789,8 @@ create_node(Host, ServerHost, Node, Owner, GivenType, Access, Configuration) ->
        end;
        {result, {_NodeId, _SubsByDepth, default}} ->

        {result, Reply};
        + {result, {_NodeId, _SubsByDepth, []}} ->
        + {result, Reply}

        ;
        {result, {_NodeId, _SubsByDepth, Result}} ->

        {result, Result};
        Error ->
        diff --git a/src/mod_pubsub/mod_pubsub_odbc.erl b/src/mod_pubsub/mod_pubsub_odbc.erl
        index b8cf489..63931c2 100644
        — a/src/mod_pubsub/mod_pubsub_odbc.erl
        +++ b/src/mod_pubsub/mod_pubsub_odbc.erl
        @@ -1604,6 +1604,8 @@ create_node(Host, ServerHost, Node, Owner, GivenType, Access, Configuration) ->
        end;
        {result, {_NodeId, _SubsByDepth, default}} ->
        {result, Reply};
        + {result, {_NodeId, _SubsByDepth, []}} ->
        + {result, Reply};
        {result, {_NodeId, _SubsByDepth, Result}} ->
        {result, Result}

        ;
        Error ->

        Show
        Dingding Ye
        added a comment - With the fixed code, after a PEP node created, it will reply an error instead of publish_item. That's because the create_node response is {result, []}. So we should either allow {result, []} in publish_item or create_node which I prefer to fix in create_node. The diff is as follows, diff --git a/src/mod_pubsub/mod_pubsub.erl b/src/mod_pubsub/mod_pubsub.erl index d7643c2..f884990 100644 — a/src/mod_pubsub/mod_pubsub.erl +++ b/src/mod_pubsub/mod_pubsub.erl @@ -1789,6 +1789,8 @@ create_node(Host, ServerHost, Node, Owner, GivenType, Access, Configuration) -> end; {result, {_NodeId, _SubsByDepth, default}} -> {result, Reply}; + {result, {_NodeId, _SubsByDepth, []}} -> + {result, Reply} ; {result, {_NodeId, _SubsByDepth, Result}} -> {result, Result}; Error -> diff --git a/src/mod_pubsub/mod_pubsub_odbc.erl b/src/mod_pubsub/mod_pubsub_odbc.erl index b8cf489..63931c2 100644 — a/src/mod_pubsub/mod_pubsub_odbc.erl +++ b/src/mod_pubsub/mod_pubsub_odbc.erl @@ -1604,6 +1604,8 @@ create_node(Host, ServerHost, Node, Owner, GivenType, Access, Configuration) -> end; {result, {_NodeId, _SubsByDepth, default}} -> {result, Reply}; + {result, {_NodeId, _SubsByDepth, []}} -> + {result, Reply}; {result, {_NodeId, _SubsByDepth, Result}} -> {result, Result} ; Error ->

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              2 years, 20 weeks ago

              Issue deployment