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

Potential XSS vulnerability with data-background-video attribute #3546

Closed
EastSun5566 opened this issue Dec 13, 2023 · 1 comment · Fixed by #3548
Closed

Potential XSS vulnerability with data-background-video attribute #3546

EastSun5566 opened this issue Dec 13, 2023 · 1 comment · Fixed by #3548

Comments

@EastSun5566
Copy link
Contributor

EastSun5566 commented Dec 13, 2023

Hi @hakimel

Thanks for all your hard work!

we've encountered a potential security issue related to XSS (Cross-Site Scripting) when using the data-background-video attribute in slides. It appears that the way Reveal.js handles this attribute might allow for the execution of injected scripts, even when the attribute value is HTML entity-encoded.

Consider the following example where we use an encoded string in the data-background-video attribute:

<section data-background-video="&quot;&gt;&lt;/video&gt;&lt;iframe srcdoc=&quot;&lt;script src='https://www.google.com/complete/search?client=chrome&q=123&jsonp=alert(document.domain)//'&gt;&lt;/script&gt;&quot;&gt;&lt;/iframe&gt;&lt;img src=wtf:&gt;&lt;a data-x=&quot;">
	<h2>🍦</h2>
</section>

The script will be executed despite being encoded. because getAttribute will decode entities even if they are already escaped:

backgroundVideo = slide.getAttribute( 'data-background-video' ),

And then concat in innerHTML will cause XSS:

backgroundVideo.split( ',' ).forEach( source => {
let type = getMimeTypeFromFile( source );
if( type ) {
video.innerHTML += `<source src="${source}" type="${type}">`;
}
else {
video.innerHTML += `<source src="${source}">`;
}
} );


Fix is pretty easy I think. Just replace innerHTML with attribute assignment, as shown below:

backgroundVideo.split( ',' ).forEach( source => {
    let type = getMimeTypeFromFile(source);
    let sourceElement = document.createElement('source');
    sourceElement.setAttribute('src', source);
    if( type ) {
        sourceElement.setAttribute('type', type);
    }

    video.appendChild(sourceElement);
});

Using setAttribute ensures that the source remains a plain string.

If this approach seems suitable, I would be happy to create a PR to address this. Thank you for your consideration!

@hakimel
Copy link
Owner

hakimel commented Dec 13, 2023

Thanks for reporting this @EastSun5566! Your proposed fix looks great. If you want to submit it as a PR I'll be happy to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants