Strip HTML tags for gist id to avoid stored XSS on showing error [Security Issue]

[jackycute]

Fix #1689

cc @lbherrera

Signed-off-by: Max Wu [email protected]

[lbherrera]

Hi @jackycute,

I think the fix contained on 85c76f5 is incomplete as the code located in https://github.com/hackmdio/codimd/blob/develop/public/js/extra.js#L332 looks for elements that have a code tag with a data-gist-id attribute and then sends a reference of it to the gist function of the gist's embed library.

So if a user created the following note, they would still be able to reach the vulnerable library and get XSS:

<code data-gist-id="payload"></code>

I think that to fix this issue you will have to make sure that all the attributes of code tags that have a data-gist-id attribute are escaped before calling $(value).gist(window.viewAjaxCallback).

[jackycute]

Hi @jackycute,

I think the fix contained on 85c76f5 is incomplete as the code located in https://github.com/hackmdio/codimd/blob/develop/public/js/extra.js#L332 looks for elements that have a code tag with a data-gist-id attribute and then sends a reference of it to the gist function of the gist's embed library.

So if a user created the following note, they would still be able to reach the vulnerable library and get XSS:

<code data-gist-id="payload"></code>

I think that to fix this issue you will have to make sure that all the attributes of code tags that have a data-gist-id attribute are escaped before calling $(value).gist(window.viewAjaxCallback).

Thanks for heads up @lbherrera.
I've force pushed a commit that set attribute with stripped tags gist id before applying gist embed to the element.
Please take a look if that works, thanks.

[lbherrera]

I think that works, but there are other attributes that get used by gist embed that also seem to be reflected into error messages and need to be sanitized as well.

e = f.data("gist-id") || "";
h = f.data("gist-file");
k = f.data("gist-hide-footer") === true;
d = f.data("gist-hide-line-numbers") === true;
n = f.data("gist-line");
l = f.data("gist-highlight-line");
m = f.data("gist-show-spinner") === true;

[...]

g = f.data("gist-show-loading") !== undefined ? f.data("gist-show-loading") : true

I don't see this being exploitable anymore if you also sanitize the data-gist-file, data-gist-line, data-gist-show-loading and data-gist-highlight-line attributes.

[jackycute]

I think that works, but there are other attributes that get used by gist embed that also seem to be reflected into error messages and need to be sanitized as well.

e = f.data("gist-id") || "";
h = f.data("gist-file");
k = f.data("gist-hide-footer") === true;
d = f.data("gist-hide-line-numbers") === true;
n = f.data("gist-line");
l = f.data("gist-highlight-line");
m = f.data("gist-show-spinner") === true;

[...]

g = f.data("gist-show-loading") !== undefined ? f.data("gist-show-loading") : true

I don't see this being exploitable anymore if you also sanitize the data-gist-file, data-gist-line, data-gist-show-loading and data-gist-highlight-line attributes.

Thanks for the reminder.
I've pushed commit that also sanitize those attributes, please take a look.