Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make use trusted-types #2408

Closed
jvoisin opened this issue Feb 26, 2024 · 8 comments · Fixed by #2532
Closed

Make use trusted-types #2408

jvoisin opened this issue Feb 26, 2024 · 8 comments · Fixed by #2532

Comments

@jvoisin
Copy link
Contributor

jvoisin commented Feb 26, 2024

XSS are still somehow a plague in 2024, but luckily we have some ways to mitigate them:

The main blocker for trusted-types adoption is the pervasive usage of .innerHTML in app.js:

$ git grep -c innerHTML
internal/ui/static/js/app.js:14
$

it shouldn't be too hard™ to replace those with non-injectable sinks. I think it would make a great low-hanging/easy issue for anyone knowing javascript and wanting to contribute to miniflux.

Having trusted-types support in miniflux would prevent DOM-XSS in miniflux' own javascript code, but in its dependencies as well.

The only remaining issues are:

  1. Doing an ajax request to fetch some trusted HTML in handleFetchOriginalContent in app.js:
function handleFetchOriginalContent() {
    //[...]  
    const request = new RequestBuilder(element.dataset.fetchContentUrl);
    request.withCallback((response) => {
        element.textContent = '';
        element.appendChild(previousElement);

        response.json().then((data) => {
            if (data.hasOwnProperty("content") && data.hasOwnProperty("reading_time")) {
                document.querySelector(".entry-content").innerHTML = data.content;                                                                                                                 
                const entryReadingtimeElement = document.querySelector(".entry-reading-time");
  1. Registering the service-worker in bootstrap.js.
    if ("serviceWorker" in navigator) {                                                                                                                                                            
        let scriptElement = document.getElementById("service-worker-script");
        if (scriptElement) {
            navigator.serviceWorker.register(scriptElement.src);
        }
    }  

the src being defined in a template as <script src="{{ route "javascript" "name" "service-worker" "checksum" .sw_js_checksum }}" defer id="service-worker-script"></script>, but I don't think we want to use templating in the javascript files.

@guest20
Copy link

guest20 commented Feb 28, 2024

They're mostly of the form:

element.innerHTML = '<span class="icon-label">' + element.dataset.labelLoading + '</span>';

... so they just pull data attributes stuff from elsewhere in the dom.

You'd have to XSS a value into the data attribute for this to be an XSS.

@jvoisin
Copy link
Contributor Author

jvoisin commented Feb 28, 2024

The issue isn't that the current usage of innerHTML can be used for content injection. It's that they do prevent the usage of trusted-types :)

@guest20
Copy link

guest20 commented Feb 29, 2024

Those examples are easy enough to replace theses examples with code that uses createElement and replaceChilderen etc

@jvoisin
Copy link
Contributor Author

jvoisin commented Feb 29, 2024

You're vastly overestimating my javascript fluency: I'm a simple security engineer, those javascript vagaries confuse and frighten me.

@guest20
Copy link

guest20 commented Feb 29, 2024

I understand the desire to have a type system solve XSS, but there's a good reason math teachers make you learn first principals first.

This kind of method is also discussed in the documentation for trusted types... which makes me think you're on here telling someone to use a library when you haven't even read its docs...

(though the example for that method ... messes up the escaping in the article and shows a broken image, which is absolutely amazing to me):

Screen Shot 2024-02-29 at 15 23 28

Edit: linked to the docs

@jvoisin
Copy link
Contributor Author

jvoisin commented Feb 29, 2024

I'm a security engineer at Google, I sit right next to the people who designed trusted-types, and shipped them into Chrome :p I'm pretty well versed in its threat model and deployment, it's just that I do suck at javascript.

But anyway, once I'm done rampaging through the go codebase, odds are that I'll tackle this issue myself, if nobody else does it before.

@jvoisin jvoisin mentioned this issue Feb 29, 2024
2 tasks
@guest20
Copy link

guest20 commented Feb 29, 2024

@jvoisin That's handy! Can you let them know about that doc bug i mentioned? I couldn't find a way of reporting it without a google account :(

@jvoisin
Copy link
Contributor Author

jvoisin commented Feb 29, 2024

Sure: @koto @mikispag, can you PTAL? <3

@fguillot fguillot linked a pull request Mar 21, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants