Bug 35960 - XSS in staff login form
Summary: XSS in staff login form
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-31 13:53 UTC by Julian Maurice
Modified: 2024-04-03 12:08 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10,22.11.16,22.05.20


Attachments
Bug 35960: Use .val() instead of string concat to prevent potential XSS (1.41 KB, patch)
2024-02-01 08:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35960: Use .val() instead of string concat to prevent potential XSS (1.46 KB, patch)
2024-02-14 15:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35960: Use .val() instead of string concat to prevent potential XSS (1.52 KB, patch)
2024-02-28 06:44 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2024-01-31 13:53:44 UTC
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.
Comment 1 David Cook 2024-01-31 22:08:02 UTC
(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.
Comment 2 Julian Maurice 2024-02-01 08:22:18 UTC
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.
Comment 3 Owen Leonard 2024-02-14 15:19:59 UTC
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>
Comment 4 Victor Grousset/tuxayo 2024-02-28 06:44:53 UTC
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>
Comment 5 Victor Grousset/tuxayo 2024-02-28 06:57:31 UTC
> 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.
Comment 6 Katrin Fischer 2024-03-15 15:04:53 UTC
Can I get another opinion here on whether this should be classified as a security bug or not, please? (see comment#5 from Victor)
Comment 7 Jonathan Druart 2024-03-15 15:29:03 UTC
We have a security release to schedule anyway. In doubt I'd say yes.
Comment 8 Katrin Fischer 2024-03-15 16:25:15 UTC
Pushed to security repository.
Comment 9 Fridolin Somers 2024-03-18 09:36:18 UTC
+                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 ... />" ?
Comment 10 Julian Maurice 2024-03-18 10:17:29 UTC
(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 ? ;)
Comment 11 Katrin Fischer 2024-03-18 10:22:15 UTC
I was just looking at this one too, thanks for explaining Julian. I will not push a follow-up for now.
Comment 12 Fridolin Somers 2024-03-18 15:56:54 UTC
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.
Comment 13 Victor Grousset/tuxayo 2024-03-20 05:17:11 UTC
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.
Comment 14 wainuiwitikapark 2024-03-20 22:23:49 UTC
Backported to 22.05.x-security for 22.05.20