Add quick scan report summary
Compare changes
content/security-quick-scan.md
0 → 100644
+ 72
− 0
Due to the [change of maintainers]({filename}/funkwhale-is-looking-for-new-maintainers.md) and the subsequent handover of duties, we've not had time to publish the results of this report and the actions we took to address the issues raised. Since things have calmed down a bit, let's go through these here!
NLNet's security report involved a quick penetration test performed by an independent security company. Its goal was to point out common exploits present in our app. The report highlighted **4** issues. Of these, **2** are **Resolved**. The other **2** we have decided we are **Not doing**. Let's get into the details.
After some investigation we determined that the lack of security in this situation is actually a quirk of federated services. The endpoint that was highlighted by the report is one used to search for remote libraries. This endpoint passes a URL to the Funkwhale API which then sends a request. Since there is no centralized database for us to validate URLs against, there is no way of knowing whether the domain name is a valid Funkwhale URL. The only mitigation would be to implement URL pattern checks, but this would not prevent an attack.
NLNet noted that the `/login` screen accepts a `next` parameter. This is used to redirect users to the page they were trying to access after a successful login. However, NLNet noted that this left the user vulnerable to an open redirect, where an attacker could put any URL in this parameter to forward users to a hostile domain.
This was a good catch! We decided to introduce a mechanism that checks the URL in the `next` parameter to ensure it is an internal URL. This way, the user can only be forwarded to screens in the same app they came from. We do this by checking the URL against the list of valid routes in the application, and routing the user to the home page if the URL is not in the list.
NLNet highlighted that moderation endpoints such as `/manage/moderation` and `/manage/users` were not protected by user access controls. While conditional rendering meant that no content could be accessed by an unauthenticated or unprivileged user, it is best practice to lock the screen itself down.
We've implemented several improvements to our Nginx config since this report was done. Notably, the suggested header (`"Strict-Transport-Security: max-age=31536000"`) is present in our config already. We do not use the `includeSubDomains` option as we don't think this is a choice we should make for our users. Users can add this to their config if they want a little extra security!