On the staff authentication page, there is this block of JS code: if ( document.location.hash ) { $( '#loginform' ).append( '<input name="auth_forwarded_hash" type="hidden" value="' + document.location.hash + '"/>' ); } document.location.hash is not HTML-escaped This has been brought up by a security audit but I'm not sure if it's a real issue, as document.location.hash is (according to MDN) always URI-encoded, so characters like ", < or > are percent-encoded I haven't been able to inject javascript in the page using that yet. What do you think ? Maybe we should use jQuery's .val(), just to be extra safe.
(In reply to Julian Maurice from comment #0) > Maybe we should use jQuery's .val(), just to be extra safe. Sounds good to me. I think using .val() would also look cleaner than a concatenated string, so win-win.
Created attachment 161712 [details] [review] Bug 35960: Use .val() instead of string concat to prevent potential XSS Test plan: 1. Log out 2. Go to /cgi-bin/koha/mainpage.pl#somestring"with<html>char 3. Open the brower's inspector and find "auth_forwarded_hash" input 4. Make sure the value attribute is there and corresponds to the URL's fragment. It should be URI-encoded.
Created attachment 162149 [details] [review] Bug 35960: Use .val() instead of string concat to prevent potential XSS Test plan: 1. Log out 2. Go to /cgi-bin/koha/mainpage.pl#somestring"with<html>char 3. Open the brower's inspector and find "auth_forwarded_hash" input 4. Make sure the value attribute is there and corresponds to the URL's fragment. It should be URI-encoded. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 162526 [details] [review] Bug 35960: Use .val() instead of string concat to prevent potential XSS Test plan: 1. Log out 2. Go to /cgi-bin/koha/mainpage.pl#somestring"with<html>char 3. Open the brower's inspector and find "auth_forwarded_hash" input 4. Make sure the value attribute is there and corresponds to the URL's fragment. It should be URI-encoded. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
> This has been brought up by a security audit but I'm not sure if it's a real issue, as document.location.hash is (according to MDN) always URI-encoded Seems about right. https://security.stackexchange.com/questions/228925/dom-based-xss-using-window-location-hash/228926#228926 Still a nice to have cleanup and will prevent automated tools to give false positives. Works, makes sense, QA script happy, code looks good, passing QA :) OPAC login doesn't seem to have a similar feature. --- Should this be reclassified as a non security ticket? Or do we keep it there and not handle it as an actual security issue? What's looks certain is that it's totally not worth it to include in a security release. They are already enough a pain without more tickets.
Can I get another opinion here on whether this should be classified as a security bug or not, please? (see comment#5 from Victor)
We have a security release to schedule anyway. In doubt I'd say yes.
Pushed to security repository.
+ const input = $('<input name="auth_forwarded_hash" type="hidden">') + input.val(document.location.hash); + $( '#loginform' ).append( input ); There is a ";" missing in first line no ? And maybe keep XHTML syntax "<input ... />" ?
(In reply to Fridolin Somers from comment #9) > + const input = $('<input name="auth_forwarded_hash" > type="hidden">') > + input.val(document.location.hash); > + $( '#loginform' ).append( input ); > > There is a ";" missing in first line no ? Semicolons are optional here (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#automatic_semicolon_insertion) but could be added for consistency. Is there a coding guideline rule for this ? I didn't find one. > And maybe keep XHTML syntax "<input ... />" ? Coding guidelines says "HTML5 does not strictly require closing tags, but Koha templates should continue to use XHTML-style syntax tag closing, including self-closing tags." which is a little misleading as the HTML spec says it "is unnecessary and has no effect of any kind" and "it should be used only with caution — especially since, if directly preceded by an unquoted attribute value, it becomes part of the attribute value rather than being discarded by the parser." So, per the coding guidelines, you're right. But are the coding guidelines right ? ;)
I was just looking at this one too, thanks for explaining Julian. I will not push a follow-up for now.
Thanks a lot for explaining, I always learn here ;) I think adding semicolon would be better for consistency. Since this is important code better make it strong.
Thanks Fridolin for raising these points that I missed. +1 for consistency for the semi colon. And to stay consistent with the code that follows the guideline about void/self closing tag. (In reply to Julian Maurice from comment #10) > the HTML spec says it "is unnecessary and > has no effect of any kind" and "it should be used only with caution — > especially since, if directly preceded by an unquoted attribute value, it > becomes part of the attribute value rather than being discarded by the > parser." > So, per the coding guidelines, you're right. But are the coding guidelines > right ? ;) There doesn't seems to be a need to worry about unquoted attribute values. Otherwise yes that would be a good foolproof choice to everywhere avoid the self-closing /. So the current guideline seems on point just to keep consistency even if there is not functional of foolproof reason. Like a part of stuff enforced by perltidy. Thanks, that was an occasion to learn more about this.
Backported to 22.05.x-security for 22.05.20