Date and reason for banned accounts not stored when using SCRAM

[Dunedan]

The functionality to ban users implemented in mod_admin_extra works by changing their password to one starting with BANNED_ACCOUNT and which includes the date and reason of the ban, as well as a random string:

https://github.com/processone/ejabberd/blob/051bf2968a2f3d1f414b049daecaf1e959ff3b9a/src/mod_admin_extra.erl#L1000-L1005

However, when using auth_password_format: scram, this password gets hashed, which makes it impossible to figure out if an account is banned, when it got banned and what the reason was it got banned. It'd be great to also have that information available when using SCRAM as auth_password_format.

Instead of overwriting the password of an account when banning it, storing the information in other fields would be great as well, as that'd allow reverting bans.

[badlop]

I've added ban_account API comand v2, which uses the account private XML storage. This allows to store the password regardless if it's plaintext or scrammed, and keep other information too. I also added two new commands to view such ban information, and revert the ban.

I've tested this and works with mnesia and mysql, plaintext and SCRAM passwords.

---

How to use:

Lets ban an account. It detects if the account is already banned:

$ ejabberdctl ban_account user2 localhost largo_de_aqui
$ ejabberdctl ban_account user2 localhost largo_de_aqui
Error: account_was_already_banned

Lets get ban information of an account

$ ejabberdctl get_ban_details user2 localhost
reason  largo_de_aqui
bandate 2024-04-22T09:16:47.975312Z
lastdate        2024-04-22T08:39:12Z
lastreason      Registered but didn't login

And finally, lets revert the banning. It detects if the account is not banned:

$ ejabberdctl unban_account user2 localhost
$ ejabberdctl unban_account user2 localhost
Error: account_was_not_banned

The old command implementation is still available, in case somebody still wants to use it:

$ ejabberdctl --version 0 ban_account user2 localhost largo_de_aqui

---

It would be great if you can test the feature and report your results.

There are binary installers and the container image.

[Dunedan]

Thanks a lot for addressing this issue. I played around with your changes a bit and believe there is a major flaw in the current implementation: Users with knowledge about the implementation can make themselves "unbannable". That's because the implementation uses a user accessible storage to store the ban details. So a regular user can simply write something to the ejabberd:banned namespace of his private XML storage, which does trick mod_admin_extra into thinking that the user is already banned, while he actually isn't. To do so, all a user has to do is issue an IQ request like this:

<iq type="set" id="123">
  <query xmlns="jabber:iq:private">
    <banned xmlns='ejabberd:banned'>
      <reason>I'm unbannable now.</reason>
      <password>super_secret_password</password>
      <lastdate>2024-01-02T03:04:05.000000Z</lastdate>
      <lastreason>ONLINE</lastreason>
      <bandate>2024-02-03T04:05:06.000000Z</bandate>
    </banned>
  </query>
</iq>

If an admin wants to ban the user later on, he'll get the message that the user is already banned, while the user is still able to log in, as his password had never been replaced with a randomly generated one:

$ docker exec -it ejabberd ejabberdctl ban_account user1 localhost test
Error: account_was_already_banned

I assume the current implementation of replacing a users password with a random one when banning is intentional to be able to confine the ban logic to mod_admin_extra. However, I believe adding an additional flag in the auth table whether a user is banned or not might be worth considering. This would not only solve this issue, but would also allow ejabberd to return another error message text when a banned user tries to log in. Right now a banned user just gets "Invalid username or password" when trying to log in, which makes it impossible for him to figure out whether he mistyped his password or whether he's banned.

One downside of the implementation utilizing a users private XML storage is also that users will loose data they have stored in the ejabberd:banned namespace when they get banned and later unbanned. That's probably a rather theoretical issue, as it's unlikely users will store something in that namespace, but technically it might cause ejabberd to not be compliant with XEP-0049, as only the users themselves should be allowed to modify their private XML storage (https://xmpp.org/extensions/xep-0049.html#sect-idm26892098754400). I have no idea how much of a concern that is though.

[badlop]

is intentional to be able to confine the ban logic to mod_admin_extra

More than that, a design goal right now for that feature is to not require fields in other tables (which would involve updating their schemas in mnesia, and all SQL), nor require a brand new table (which would involve writting code for mnesia and SQL backends).

As long as this feature is rarely used, let's try to get it working without requiring specific database changes.

---

there is a major flaw in the current implementation: Users with knowledge about the implementation can make themselves "unbannable".

Wow, right!

I've now added a hash value to the banned element, without that one the element is not taken into account. A valid hash can only be generated by ejaberd or somebody that knows the erlang's cookie (like a server administrator), regular users don't know it.

---

return another error message text when a banned user tries to log in

And the error returned could be https://xmpp.org/rfcs/rfc6120.html#sasl-errors-account-disabled

Ok, I've implemented it. It requires minor changes in the xmpp library (that could be useful in other cases in the future).

---

users will loose data they have stored in the ejabberd:banned namespace when they get banned and later unbanned.

Aha, right.

Looking for a solution, I noticed a paragraph in XEP-0049: Private XML Storage that says:

Certain namespaces are reserved in Jabber (namespaces beginning with 'jabber:' or 'http://jabber.org/', as well as 'vcard-temp'). If a user attempts to get or set jabber:iq:private data in a reserved namespace, historically some server implementations have chosen to return an error (commonly "Not Acceptable") to the sender. Such behavior is OPTIONAL, but may be encountered by clients when interacting with some existing server implementations.

This means that the clients are not expected to use reserved namespaces. ejabberd does not enforce that restriction, but anyway a client cannot expect such reserved namespaces to persist. Consequently, I think acceptable to use a reserved namespace for ejabberd, for example jabber:ejabberd:banned.

---

I've implemented all those changes in my fork, so we can test it without messing in the public repository. Once we are happy with the changes, they can get merged in the public repository.

There are binary installers and container image in case you cannot compile ejabberd yourself.

[Dunedan]

I've now added a hash value to the banned element, without that one the element is not taken into account. A valid hash can only be generated by ejaberd or somebody that knows the erlang's cookie (like a server administrator), regular users don't know it.

Sounds good and I didn't notice any problems in my tests.

And the error returned could be https://xmpp.org/rfcs/rfc6120.html#sasl-errors-account-disabled

Ok, I've implemented it. It requires minor changes in the xmpp library (that could be useful in other cases in the future).

That's much appreciated.

Looking for a solution, I noticed a paragraph in XEP-0049: Private XML Storage that says:

Certain namespaces are reserved in Jabber (namespaces beginning with 'jabber:' or 'http://jabber.org/', as well as 'vcard-temp'). If a user attempts to get or set jabber:iq:private data in a reserved namespace, historically some server implementations have chosen to return an error (commonly "Not Acceptable") to the sender. Such behavior is OPTIONAL, but may be encountered by clients when interacting with some existing server implementations.

This means that the clients are not expected to use reserved namespaces. ejabberd does not enforce that restriction, but anyway a client cannot expect such reserved namespaces to persist. Consequently, I think acceptable to use a reserved namespace for ejabberd, for example jabber:ejabberd:banned.

I must've missed that paragraph. Using a reserved namespace sounds like good solution to me too.

One minor problem I noticed is that trying to ban an account while mod_private isn't enabled results in an exception now:

Unhandled exception occurred executing the command:
** exception error: {module_not_loaded,mod_private,<<"localhost">>}
   in function  gen_mod:get_module_opts/2 (src/gen_mod.erl, line 399)
   in call from gen_mod:get_module_opt/3 (src/gen_mod.erl, line 379)
   in call from gen_mod:db_mod/3 (src/gen_mod.erl, line 413)
   in call from mod_private:set_data/4 (src/mod_private.erl, line 223)
   in call from mod_admin_extra:ban_account_v2_b/3 (src/mod_admin_extra.erl, line 1086)
   in call from ejabberd_ctl:call_command/4 (src/ejabberd_ctl.erl, line 327)
   in call from ejabberd_ctl:try_call_command/4 (src/ejabberd_ctl.erl, line 288)
   in call from ejabberd_ctl:process2/4 (src/ejabberd_ctl.erl, line 228)

Maybe it'd make sense to just print a message when the new ban commands get invoked while mod_private is disabled to inform the admin about that and point him to the ban_account command with the old API version, which still allows banning when mod_private is disabled. Not sure if it's worth the effort though.

[badlop]

The commands documentation already mentions that mod_private is required, but of course it's preferable if the error message clearly indicates this. I've added this in commit https://github.com/badlop/ejabberd/commit/dd299788e8f2c576b024fb98fd55f7efdac02316

[Dunedan]

I've no further comments. From my point of view it's ready to get merged now. :+1: