Security vulnerabilities and mitigations around users

Security vulnerabilities and mitigations around users

Hello folks.

We’ve been both audited AND attacked on moqui instances. I am listing below what I found so far, and I’m also providing PRs for fixes to some of these problems. We might require CVE declarations, or not depending on how you wish to handle this.

Brute force user info extraction

A successful attack was made for gathering user information. The attack is a combination of:

  1. brute force login end-point, the end-point differentiates between non existent users and those with incorrect password.
  2. find email addresses of identified existing users by calling resetPassword from Login.xml

Now although we can tarpit the resetPassword end-point as it calls a service, we cannot tarpit the login end-points because they are not services and upon testing it seems tarpits only apply on services, not screens or transitions.

XSS in all render modes

go to any text field in vuet, qvt or regular apps render mode and write something like the following

{{constructor.constructor('alert(1)')()}}

you will notice that the alert window will popup. We need to sanitize text fields in all render modes.

PRs to fix some of these vulnerabilities

  • First, create a unified login service in UserServices, framework PR here and in the same PR also hide email address from resetPassword end-point
  • Rewire login end-points in runtime to use above service runtime PR here

I have more vulnerabilities to address later on, but these seem to be the most critical for now. I look forward to hearing your thoughts on them.

4 Likes

Kindly also note we’ll probably need to work on PopRestStore to fix similar kinds of vulnerabilities around user access issues

1 Like

To protect login from being abused, I just come up with a simple method:

Write a filter for jetty server which will read a txt file containing a list of ip address allowed to have access to login page under webroot. This text file will be reloaded automatically if its content being changed.

This is just a hack. Not a well thought method.

PopRestStore, I did not do anything with it. Because it is open for the public. Meaning that login rest service is exposed to public as it is.

Looking forward to see others opinions.

1 Like

So if something is open to the public (rest.xml or PopRestStore) does not mean you don’t secure it against brute-force attacks. You must at a minimum throttle down the frequency or “tarpit” the attack, otherwise people can run scripts against your server all day to try and expose either usernames or weak passwords.

So my patches above, convert both Login.xml and rest.xml to utilize a new service that I created in UserServices.xml that is a centralized login service. The benefit of this design is that first, we fix all login problems in one place, second, we can now tarpit this service and thus limit the rate of attack.

PopRestStore should also utilize this service and only add the additional logic related to e-commerce. This way everything depends on a single service that is well maintained and does everything for consumers. Note that only login is not a service, everything else is actually a service like resetPassword, updatePassword and so on. So my focus was on fixing login.

Next, I also changed the error message of resetPassword not to expose the email that the reset password is being sent to. This prevents attackers from building a database of users (username and email) by combining the above attacks.

1 Like

On the XSS issue, or maybe better described as a template injection attack, here is an issue where this came up a few months ago:

There is a commit in moqui-runtime mentioned there, and a few follow up commits from more testing and fixing.

When you found this issue was it a version of moqui-runtime before or after these commits? After those changes I did consider that filtering input is probably worth it, but there are a few issues with that. One of them is the general principles of changing something in one place (input) to fix an issue in another place (output rendering). Part of the problem in this case is if something gets into the database anyway then the input filtering won’t matter. Another part of that problem for this case is not all output for Moqui uses Vue JS for rendering, or other {{}} based string expansion.

BTW, you mentioned regular apps render mode, plain HTML… do you have a test case for that? This sort of template interpolation is only done by Vue JS, which is only used in the vuet and qvt render modes (ie under /vapps and /qapps).

The main question I have is whether you found this issue with a version of moqui-runtime from before or after the changes mentioned above. If you found it with those changes in place then there is still a hole somewhere, so more details would be helpful.

Other may disagree, but FWIW I this is fine to discuss in the open. It is a somewhat minor issue and more importantly has already been discussed in the open and already has a mitigation in place… or a partial mitigation if more holes are found!

I’ll reply separately about the other issue.

On the two part issue of brute force valid username determination and getting an email address from a username, I think it is worth changing the message to not include the email address. That is likely to cause some user support challenges as users don’t always know which email address to check and the email address associated with their account may not be one they expect, but unless someone actually finds that their helpdesk load for forgotten passwords increases because of this then it’s probably safe to guess that it would only cause minor and infrequent issues for users and helpdesk personnel.

On the brute force valid username discovery, I had a chat with Michael about that to think it over (this thread was one topic that came up on our call today). The only solution we role played that is likely to work is client IP based velocity limits.

The current velocity limits in Moqui will not help. The wrong password time delay for incorrect password on a correct account is per user, and so are artifact tarpits (which can be used for screens and transitions, but not for entity ops, IIRC). In other words even existing artifact tarpits would not help with this because they are tarpits for authenticated users, not for pre-auth requests.

The idea of dumbing down the message to not distinguish between invalid username and incorrect password would also not help, that just slightly changes the approach needed for a brute force attack. With an incorrect password temporary account lockout all one has to do is try each candidate username with X different passwords, X being the number of login attempts before account lock which can be determined by manual testing, and if it does lock you out you know the username is valid.

Are there any other approaches that anyone would like to propose? The best one seems to be client IP address based velocity limits, which is a bit of a pain to build and there are ways to partly workaround even that, but even the most determined attacker can only get access to a limited number of client IP addresses (okay, a large group that is determined enough can get millions by routing traffic through botnets of compromised machines, but I don’t think that is so common).

I’ll take a closer look at the PR you submitted and comment on it over there on GitHub.

Hi @jonesde

OK a few updates after testing:

First of all, you’re right “/apps” is not affected I misspoke it’s only qvt and vuet modes. With the latest version of the framework and runtime components I still get the popup when executing {{constructor.constructor('alert(1)')()}} into any text field.

Regarding the email address you mentioned that it might cause some confusion. In this, how about obfuscating the email, something like a********b@c*****d, that could perhaps strike a balance between user confusion vs security and information collection.

It’s unfortunate indeed that we don’t have tarpits on guest accounts or visits or IP addresses. This means my only option until we implement this is to perhaps secure login, signup, resetPassword to be all behind something like google recaptcha. But this means now that I need to override Login.xml, rest.xml and UserServices.xml to wire in captcha to all of those, and I have to maintain a synched version with the community.

So how about this? let’s override all these entry points to accept an optional recaptcha token, with some kind of setting to the framework, such that if captcha is enabled then it will block all these end-points. I already have recaptcha enforced on signup for one of the systems

Oh and one last thing, I was not able to activate tarpits on screens or transitions. Maybe I’m doing something wrong?

1 Like

Here is an exact repeat that showcases the vulnerability on latest version of moqui and runtime

git clone https://github.com/moqui/moqui-framework.git
cd moqui-framework
./gradlew getComponentSet -PcomponentSet=demo -PlocationType=release
./gradlew downloadOpenSearch
./gradlew startElasticSearch
./gradlew load
./gradlew run

Now visit any screen in the system, for example:

  • go to find supplier
  • click on “Find Options” (vapps or qapps)
  • input {{constructor.constructor('alert(1)')()}} into any of the text fields
  • observe the popup executing

So, latest version of moqui and runtime is definitely still exposed. I will try to check the macros to see how to fix it, but this repeat at least confirms on vanilla moqui system the vulnerability persists.

1 Like

Thanks for the details @taher

I was able to reproduce this. The Find Options inputs were being displayed in 2 places in the qvt form-list header, and 1 place in the vuet one. Here are the changes to add v-pre in these 3 places:

Hopefully the scan you did was more thorough that the manual testing that Michael and I did for the first set of changes for this a while back, and that this is the last of the places that need v-pre. Chances are there are more… lots of code in there.

2 Likes

Why wasn’t this merged: rewrite login end-points to use unified service by pythys · Pull Request #226 · moqui/moqui-runtime · GitHub? In the comments I see mentioning that there are functionality changes, but I don’t see any. The code in UserServices.login#UserAccount seems to be just a copy from Login.xml…

So currently there are 3 places for login functionality: Login.xml, rest.xml and UserServices.login#UserAccount. Actually 4 if we also count CustomerServices.login#Customer in PopRestStore. IMO we should merge these into one, at least the ones from the framework in the PR.

Also, are there no tests for login functionality? I couldn’t find any. Security features I think should have at least some basic unit tests. This might also be one of the issues why the PR wasn’t merged yet.

Also there is another problem I see with current functionality, and that is that for a number of wrong password attempts the user is disabled. This is wrong in my opinion, because if someone tries to bruteforce my account, I will be locked out of my account(and I am innocent, I did nothing wrong) and the attacker will still be free to bruteforce other accounts. IMO the disabling should be IP-based, not user based. This is also related to what you said above, about the IP-based tarpitting. You said it is hard to implement and takes a bit of time, but why do we need to do it in Moqui. If Moqui already stands behind a reverse proxy(which it should) why not use a log parsing solution to read access attempts and ban them, like fail2ban for example. This solves this problem.

Quick note here: I’ve recently setup Crowdsec for my server and it banned my own IP because it believed some app I used was trying to scan the server for endpoints, so you might try to relax the banning rules if you setup such a system.

Okay I did some tests and here are the results. As a side note, it seems Moqui is inherently session based, and by this I mean that even for REST calls that are technically stateless, Moqui will create a session with a corresponding file in the sessions folder and return a JSESSIONID for each call. You could of course store the JSESSIONID and send it in a cookie with subsequent requests so then new sessions would not be created, and also you don’t need to authenticate again.

Also there are some other security problems, not related to this PR, respectively:

  • there is no validation after adding a 2FA method, this means you can lock yourself out of your account by: adding Email Factor with no email set on the UserAccount, or an invalid email, or no EmailServer configured; adding an authenticator app and not verifiying it(aka not adding the secret in the authenticator app)
  • if the user is added to a group that has requireAuthcFactor='Y' and the user doesn’t have a valid email address or no EmailServer is configured, the user will again be locked out of his account

In my opinion, after these test results we should merge Taher’s PR and focus on testing and implementing the scenario of setting up a 2FA method for a user that doesn’t have one when 2FA is enforced.

Stock Moqui
UI Tests
Request: login non-existent user
Result: No account found for username notauser
Request: login john.doe:moqui
Response: success

Activate 2FA with single use code
Request: login john.doe:moqui
Response: An authentication code is required for your account, you have these options: Single Use Code
Request: fill wrong 2FA code
Response: Authentication code is not valid
Request: fill correct 2FA code
Response: success
Request: logout and login again
Response: success, 2FA is not required anymore

REST Login Tests

  1. POST rest/login
    Response code 500
{
  "errors": "No username specified\n",
  "loggedIn": false
}
  1. POST rest/login
{
  "username": "john.doe"
}

Response 500

{
  "errors": "No password specified\n",
  "loggedIn": false
}
  1. POST rest/login
{
  "username": "notauser",
  "password": "moqui"
}

Response 500

{
  "errors": "No account found for username notauser\n",
  "loggedIn": false
}
  1. POST rest/login
{
  "username": "john.doe",
  "password": "moqui"
}

Response 200

{
  "loggedIn": true
}
  1. Activate 2FA with single use code
  2. POST rest/login
{
  "username": "john.doe",
  "password": "moqui"
}

Response 200

{
  "sendableFactors": [],
  "secondFactorRequired": true,
  "factorTypeEnumIds": [
    "UafSingleUse"
  ],
  "factorTypeDescriptions": [
    "Single Use Code"
  ]
}
  1. POST rest/login
{
  "username": "john.doe",
  "password": "moqui",
  "code": "wrongcode"
}

Response 200

{
  "secondFactorRequired": true,
  "factorTypeEnumIds": [
    "UafSingleUse"
  ],
  "messages": "Authentication code is not valid\n",
  "sendableFactors": [],
  "factorTypeDescriptions": [
    "Single Use Code"
  ]
}
  1. POST rest/login
{
  "username": "john.doe",
  "password": "moqui",
  "code": {{singleUseCode}}
}

Response 200

No response body

After applying Taher’s above PR #226 to runtime
UI Tests
Request: login non-existent user
Result: No account found for username notauser
Request: login john.doe:moqui
Response: success

Activate 2FA with single use code
Request: login john.doe:moqui
Response: An authentication code is required for your account, you have these options: Single Use Code
Request: fill wrong 2FA code
Response: [different] the error Authentication code is not valid doesn’t appear anymore on the UI, the code field is just cleared
Request: fill correct 2FA code
Response: success
Request: logout and login again
Response: success, 2FA is not required anymore

REST Login Tests

  1. POST rest/login
    Response code [different] 200
{
  [different] "messages": "No username specified\n",
  "loggedIn": false
}
  1. POST rest/login
{
  "username": "john.doe"
}

Response [different] 200

{
  [different] "messages": "No password specified\n",
  "loggedIn": false
}
  1. POST rest/login
{
  "username": "notauser",
  "password": "moqui"
}

Response [different] 200

{
  [different] "messages": "No account found for username notauser\n",
  "loggedIn": false
}
  1. POST rest/login
{
  "username": "john.doe",
  "password": "moqui"
}

Response 200

{
  "loggedIn": true
}
  1. Activate 2FA with single use code
  2. POST rest/login
{
  "username": "john.doe",
  "password": "moqui"
}

Response 200

{
  "sendableFactors": [],
  [different] "loggedIn": false,
  "secondFactorRequired": true,
  "factorTypeEnumIds": [
    "UafSingleUse"
  ],
  "factorTypeDescriptions": [
    "Single Use Code"
  ]
}
  1. POST rest/login
{
  "username": "john.doe",
  "password": "moqui",
  "code": "wrongcode"
}

Response 200

{ [different]
  "messages": "Authentication code is not valid\n",
  "loggedIn": false
} [different]
  1. POST rest/login
{
  "username": "john.doe",
  "password": "moqui",
  "code": {{singleUseCode}}
}

Response 200

{
  [different] "loggedIn": true
}

Some other REST tests that have the same functionality after applying the PR

  1. GET rest/s1/moqui/users
    Response 403
{
  "errorCode": 403,
  "errors": "User [No User] is not authorized for View on REST Path /moqui/users"
}
  1. GET rest/s1/moqui/users
BASIC wronguser:moqui

Response 401

{
  "errorCode": 401,
  "errors": "No account found for username wronguser\n"
}
  1. GET rest/s1/moqui/users
BASIC john.doe:notMyPassword

Response 401

{
  "errorCode": 401,
  "errors": "Password incorrect for username john.doe\n"
}
  1. GET rest/s1/moqui/users
BASIC john.doe:moqui

Response 200 OK
5. POST /rest/login and save JSESSIONID and csrf token.
6. Enable 2FA with single use code.
7. GET rest/s1/moqui/users

BASIC john.doe:moqui

Response 403

{
  "errorCode": 403,
  "errors": "User [No User] is not authorized for View on REST Path /moqui/users"
}
  1. GET rest/s1/moqui/users
COOKIE JSESSIONID="someJID"

Response 403

{
  "errorCode": 403,
  "errors": "User [No User] is not authorized for View on REST Path /moqui/users"
}
  1. GET rest/s1/moqui/users
COOKIE JSESSIONID={{JSESSIONID}}

Response 200 OK
10. PATCH rest/s1/moqui/users/EX_JOHN_DOE

COOKIE JSESSIONID={{JSESSIONID}}

Response 401

401 Session token required (in X-CSRF-Token) for URL http://localhost:8085/rest/s1/moqui/users/EX_JOHN_DOE /rest/s1/moqui/users/EX_JOHN_DOE
  1. PATCH rest/s1/moqui/users/EX_JOHN_DOE
COOKIE JSESSIONID={{JSESSIONID}}
HEADER x-csrf-token={{csrf}}
HEADER moquisessiontoken={{csrf}}

Response 200

{}