Security Fix for Cross-site Scripting (XSS) - huntr.dev

[huntr-helper]

https://huntr.dev/users/alromh87 has fixed the Cross-site Scripting (XSS) vulnerability 🔨. alromh87 has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#2
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/other/monica/2/README.md

User Comments:

Metadata *

Bounty URL: https://www.huntr.dev/bounties/2-other-monica

⚙️ Description *

XSS queries being triggered from people info page by the audit log at the settings, fix sanitizes content before showing to user.

Technical Description *

Even tough Laravel has inbuilt protection for XSS it has been disabled when presenting log: {!! $log['description'] !!} to enable embeding contact links, leading to XSS, htmlentities was used to sanitize data before showing to user, efectively avoiding new as well as already stored XSS.

Edit

Looking further into the code I realized the XSS protection have been disabled in many views so I added a Middleware that will strip html tags using strip_tags, and since strip_tags can be tricked with malformed markup reamaning input is encoded using htmlentities.

Middleware is at app/Http/Middleware/SanitizeInput.php and can be tunned for specific keys adding them to the $except array:

    /**
     * The names of the attributes that should not be trimmed.
     *
     * @var array
     */
    protected $except = [
        'password',
        'password_confirmation',
        '_token',
        ''
    ];

Proof of Concept (PoC) *

1. Download and setup monica
2. Create new contact, introducing payload as name:
   < <svg/onload=alert("firstname1")><script> alert("firstname2_xss")</script> <script> alert("midname_xss")</script> <script> alert("Lname_xss")</script><svg/onload=alert(1)> (<svg/onload=alert("nickie1")>)
3. Go to Settings -> Audit logs
4. XSS is triggerd

Proof of Fix (PoF) *

After fix introduced data is displayed as text and no XSS is executed

Stored XSS will be also stripped out and encoded after contact is edited

User Acceptance Testing (UAT)

Functionallity is unafected, contact link works as usual

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ Mik317
✅ alromh87
✅ JamieSlome
✅ asbiin
❌ Raj

---

Raj seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[JamieSlome]

@alromh87 - are you able to sign the CLA? Let me know if you have any questions! 🍰

[alromh87]

Sure 😉

[asbiin]

Hi @huntr-helper @JamieSlome @alromh87
Thank you for your submit.
However, I cannot merge this change.
htmlentities is not to be used directly, as Laravel already handle this kind of security checks (but I don't know yet why it's not doing it in this case).

Anyway, if you want to consider it, please remove the first changes done in the commit 3871537, because it has been superseeded by another fix here.

[alromh87]

I will update tomorrow, I found in many places protection was disabled due to using {!! $log['description'] !!}

[alromh87]

@asbiin Updated as per requested and updated with upstream, use of htmlentities was removed when presenting information, only middleware for sanitizing before storage is preserved.

[asbiin]

@alromh87 any update on that fix? Current changes still uses htmlentities(), and it should never been required.

[asbiin]

This PR has been on hold for a long time.
I'll close it for now, don't hesitate to reopen if needed.

[github-actions]

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.